[PATCH v0 0/2] IMA: Deferred measurement of keys

[PATCH v0 0/2] IMA: Deferred measurement of keys

[Lakshmi Ramasubramanian]

This patchset extends the previous version[1] by adding support for
deferred processing of keys.

With the patchset referenced above, the IMA subsystem supports
measuring keys when the key is created or updated. But the keys
created or updated before IMA subsystem is initialized are not
handled. This includes keys added to, for instance,
.builtin_trusted_keys which happens very early in the boot process.

This change adds support for queuing keys when IMA is not ready
and process the keys (including queued keys) when IMA is initialized.

[1] https://lore.kernel.org/linux-integrity/20191127015654.3744-1-nramas@linux.microsoft.com/

Testing performed:

  * Booted the kernel with this change.
  * Added .builtin_trusted_keys in "keyrings=" option in
    the IMA policy and verified the keys added to this
    keyring are measured.
  * Specified only func=KEY_CHECK and not "keyrings=" option,
    and verified the keys added to builtin_trusted_keys keyring
    are processed.
  * Added keys at runtime and verified they are measured
    if the IMA policy permitted.
      => For example, added keys to .ima keyring and verified.

Changelog:

  v0

  => Based changes on v5.4-rc8
  => The following patchsets should be applied in that order
     https://lore.kernel.org/linux-integrity/1572492694-6520-1-git-send-email-zohar@linux.ibm.com
     https://lore.kernel.org/linux-integrity/20191127015654.3744-1-nramas@linux.microsoft.com/
  => Added functions to queue and dequeue keys, and process
     the queued keys when custom IMA policies are applied.

Lakshmi Ramasubramanian (2):
  IMA: Defined queue functions
  IMA: Call queue and dequeue functions to measure keys

 security/integrity/ima/ima.h                 |  15 ++
 security/integrity/ima/ima_asymmetric_keys.c | 151 ++++++++++++++++++-
 security/integrity/ima/ima_policy.c          |  12 ++
 3 files changed, 174 insertions(+), 4 deletions(-)

-- 
2.17.1

[PATCH v0 1/2] IMA: Defined queue functions

[Lakshmi Ramasubramanian]

Keys created or updated in the system before IMA is initialized
should be queued up. And, keys (including any queued ones)
should be processed when IMA initialization is completed.

This patch defines functions to queue and dequeue keys for
measurement. A flag namely ima_process_keys_for_measurement
is used to check if the key should be queued or should be
processed immediately.

ima_policy_flag cannot be relied upon to make queuing decision
because ima_policy_flag will be set to 0 when either IMA is
not initialized or when the IMA policy itself is empty.

Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
 security/integrity/ima/ima.h                 |  15 +++
 security/integrity/ima/ima_asymmetric_keys.c | 135 ++++++++++++++++++-
 2 files changed, 146 insertions(+), 4 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index f06238e41a7c..f86371647707 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -205,6 +205,21 @@ extern const char *const func_tokens[];
 
 struct modsig;
 
+#ifdef CONFIG_ASYMMETRIC_PUBLIC_KEY_SUBTYPE
+/*
+ * To track keys that need to be measured.
+ */
+struct ima_measure_key_entry {
+	struct list_head list;
+	void *payload;
+	size_t payload_len;
+	char *keyring_name;
+};
+void ima_process_queued_keys_for_measurement(void);
+#else
+static inline void ima_process_queued_keys_for_measurement(void) {}
+#endif /* CONFIG_ASYMMETRIC_PUBLIC_KEY_SUBTYPE */
+
 /* LIM API function definitions */
 int ima_get_action(struct inode *inode, const struct cred *cred, u32 secid,
 		   int mask, enum ima_hooks func, int *pcr,
diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c
index ca895f9a6504..10deb77b22a0 100644
--- a/security/integrity/ima/ima_asymmetric_keys.c
+++ b/security/integrity/ima/ima_asymmetric_keys.c
@@ -14,12 +14,139 @@
 #include <keys/asymmetric-type.h>
 #include "ima.h"
 
+/*
+ * Flag to indicate whether a key can be processed
+ * right away or should be queued for processing later.
+ */
+bool ima_process_keys_for_measurement;
+
+/*
+ * To synchronize access to the list of keys that need to be measured
+ */
+static DEFINE_MUTEX(ima_measure_keys_mutex);
+static LIST_HEAD(ima_measure_keys);
+
+static void ima_free_measure_key_entry(struct ima_measure_key_entry *entry)
+{
+	if (entry) {
+		kfree(entry->payload);
+		kfree(entry->keyring_name);
+		kfree(entry);
+	}
+}
+
+static struct ima_measure_key_entry *ima_alloc_measure_key_entry(
+	struct key *keyring,
+	const void *payload, size_t payload_len)
+{
+	int rc = 0;
+	struct ima_measure_key_entry *entry = NULL;
+
+	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
+	if (entry) {
+		entry->payload =
+			kmemdup(payload, payload_len, GFP_KERNEL);
+		entry->keyring_name =
+			kstrdup(keyring->description, GFP_KERNEL);
+		entry->payload_len = payload_len;
+	}
+
+	if ((entry == NULL) || (entry->payload == NULL) ||
+	    (entry->keyring_name == NULL)) {
+		rc = -ENOMEM;
+		goto out;
+	}
+
+	INIT_LIST_HEAD(&entry->list);
+	rc = 0;
+
+out:
+	if (rc) {
+		ima_free_measure_key_entry(entry);
+		entry = NULL;
+	}
+
+	return entry;
+}
+
+bool ima_queue_key_for_measurement(struct key *keyring,
+				   const void *payload, size_t payload_len)
+{
+	bool queued = false;
+	struct ima_measure_key_entry *entry = NULL;
+
+	entry = ima_alloc_measure_key_entry(keyring, payload, payload_len);
+	if (entry) {
+		/*
+		 * ima_measure_keys_mutex should be taken before checking
+		 * ima_process_keys_for_measurement flag to avoid the race
+		 * condition between the IMA hook checking this flag and
+		 * calling ima_queue_key_for_measurement() to queue
+		 * the key and ima_process_queued_keys_for_measurement()
+		 * setting this flag.
+		 */
+		mutex_lock(&ima_measure_keys_mutex);
+		if (!ima_process_keys_for_measurement) {
+			list_add_tail(&entry->list, &ima_measure_keys);
+			queued = true;
+		}
+		mutex_unlock(&ima_measure_keys_mutex);
+
+		if (!queued)
+			ima_free_measure_key_entry(entry);
+	}
+
+	return queued;
+}
+
+void ima_process_queued_keys_for_measurement(void)
+{
+	struct ima_measure_key_entry *entry, *tmp;
+	LIST_HEAD(temp_ima_measure_keys);
+
+	if (ima_process_keys_for_measurement)
+		return;
+
+	/*
+	 * Any queued keys will be processed now. From here on
+	 * keys should be processed right away.
+	 */
+	ima_process_keys_for_measurement = true;
+
+	/*
+	 * To avoid holding the mutex when processing queued keys,
+	 * transfer the queued keys with the mutex held to a temp list,
+	 * release the mutex, and then process the queued keys from
+	 * the temp list.
+	 *
+	 * Since ima_process_keys_for_measurement is set to true above,
+	 * any new key will be processed immediately and not be queued.
+	 */
+	INIT_LIST_HEAD(&temp_ima_measure_keys);
+	mutex_lock(&ima_measure_keys_mutex);
+	list_for_each_entry_safe(entry, tmp, &ima_measure_keys, list) {
+		list_move_tail(&entry->list, &temp_ima_measure_keys);
+	}
+	mutex_unlock(&ima_measure_keys_mutex);
+
+	list_for_each_entry_safe(entry, tmp,
+				 &temp_ima_measure_keys, list) {
+		process_buffer_measurement(entry->payload,
+					   entry->payload_len,
+					   entry->keyring_name,
+					   KEY_CHECK, 0,
+					   entry->keyring_name);
+		list_del(&entry->list);
+		ima_free_measure_key_entry(entry);
+	}
+}
+
 /**
  * ima_post_key_create_or_update - measure asymmetric keys
  * @keyring: keyring to which the key is linked to
  * @key: created or updated key
  * @payload: The data used to instantiate or update the key.
- * @plen: The length of @payload.
+ * @payload_len: The length of @payload.
  * @flags: key flags
  * @create: flag indicating whether the key was created or updated
  *
@@ -27,14 +154,14 @@
  * The payload data used to instantiate or update the key is measured.
  */
 void ima_post_key_create_or_update(struct key *keyring, struct key *key,
-				   const void *payload, size_t plen,
+				   const void *payload, size_t payload_len,
 				   unsigned long flags, bool create)
 {
 	/* Only asymmetric keys are handled by this hook. */
 	if (key->type != &key_type_asymmetric)
 		return;
 
-	if (!payload || (plen == 0))
+	if (!payload || (payload_len == 0))
 		return;
 
 	/*
@@ -52,7 +179,7 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
 	 * if the IMA policy is configured to measure a key linked
 	 * to the given keyring.
 	 */
-	process_buffer_measurement(payload, plen,
+	process_buffer_measurement(payload, payload_len,
 				   keyring->description, KEY_CHECK, 0,
 				   keyring->description);
 }
-- 
2.17.1

[PATCH v0 2/2] IMA: Call queue functions to measure keys

[Lakshmi Ramasubramanian]

Keys should be queued for measurement if custom IMA policies have
not yet been applied. Keys queued for measurement, if any, need to be
processed when custom IMA policies have been applied.

This patch adds the call to ima_queue_key_for_measurement() in
the IMA hook function if ima_process_keys_for_measurement flag is set
to false. And, the call to ima_process_queued_keys_for_measurement()
when custom IMA policies have been applied in ima_update_policy().

NOTE:
If the kernel is built with CONFIG_ASYMMETRIC_PUBLIC_KEY_SUBTYPE
enabled then the IMA policy should be applied as custom IMA policies.

Keys will be queued up until custom policies are applied and processed
when custom policies have been applied.

Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
 security/integrity/ima/ima_asymmetric_keys.c | 16 ++++++++++++++++
 security/integrity/ima/ima_policy.c          | 12 ++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c
index 10deb77b22a0..adb7a307190f 100644
--- a/security/integrity/ima/ima_asymmetric_keys.c
+++ b/security/integrity/ima/ima_asymmetric_keys.c
@@ -157,6 +157,8 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
 				   const void *payload, size_t payload_len,
 				   unsigned long flags, bool create)
 {
+	bool key_queued = false;
+
 	/* Only asymmetric keys are handled by this hook. */
 	if (key->type != &key_type_asymmetric)
 		return;
@@ -164,6 +166,20 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
 	if (!payload || (payload_len == 0))
 		return;
 
+	if (!ima_process_keys_for_measurement)
+		key_queued = ima_queue_key_for_measurement(keyring,
+							   payload,
+							   payload_len);
+
+	/*
+	 * Need to check again if the key was queued or not because
+	 * ima_process_keys_for_measurement could have flipped from
+	 * false to true after it was checked above, but before the key
+	 * could be queued by ima_queue_key_for_measurement().
+	 */
+	if (key_queued)
+		return;
+
 	/*
 	 * keyring->description points to the name of the keyring
 	 * (such as ".builtin_trusted_keys", ".ima", etc.) to
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 78b25f083fe1..a2e30a90f97d 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -812,6 +812,18 @@ void ima_update_policy(void)
 		kfree(arch_policy_entry);
 	}
 	ima_update_policy_flag();
+
+	/*
+	 * Custom IMA policies have been setup.
+	 * Process key(s) queued up for measurement now.
+	 *
+	 * NOTE:
+	 *   Custom IMA policies always overwrite builtin policies
+	 *   (policies compiled in code). If one wants measurement
+	 *   of asymmetric keys then it has to be configured in
+	 *   custom policies and updated here.
+	 */
+	ima_process_queued_keys_for_measurement();
 }
 
 /* Keep the enumeration in sync with the policy_tokens! */
-- 
2.17.1

Re: [PATCH v0 1/2] IMA: Defined queue functions

[Mimi Zohar]

Hi Lakshmi,

Janne Karhunen is defining an IMA workqueue in order to more
frequently update the on disk security xattrs.  The Subject line on
this patch needs to be more explicit (eg. define workqueue for early
boot "key" measurements).

On Tue, 2019-11-26 at 18:52 -0800, Lakshmi Ramasubramanian wrote:
> Keys created or updated in the system before IMA is initialized

Keys created or updated before a custom policy is loaded are currently
not measured.

> should be queued up. And, keys (including any queued ones)
> should be processed when IMA initialization is completed.
> 
> This patch defines functions to queue and dequeue keys for
> measurement. A flag namely ima_process_keys_for_measurement
> is used to check if the key should be queued or should be
> processed immediately.
> 
> ima_policy_flag cannot be relied upon to make queuing decision
> because ima_policy_flag will be set to 0 when either IMA is
> not initialized or when the IMA policy itself is empty.

I'm not sure why you want to differentiate between IMA being
initialized vs. an empty policy.  I would think you would want to know
when a custom policy has been loaded.

Until ima_update_policy() is called, "ima_rules" points to the
architecture specific and configured policy rules, which are
persistent, and the builtin policy rules.  Once a custom policy is
loaded, "ima_rules" points to the architecture specific, configured,
and custom policy rules.

I would define a function that determines whether or not a custom
policy has been loaded.

(I still need to review adding/removing from the queue.)

> 
> @@ -27,14 +154,14 @@
>   * The payload data used to instantiate or update the key is measured.
>   */
>  void ima_post_key_create_or_update(struct key *keyring, struct key *key,
> -				   const void *payload, size_t plen,
> +				   const void *payload, size_t payload_len,
>  				   unsigned long flags, bool create)

This "hunk" and subsequent one seem to be just a variable name change.
 It has nothing to do with queueing "key" measurements and shouldn't
be included in this patch.

Mimi

>  {
>  	/* Only asymmetric keys are handled by this hook. */
>  	if (key->type != &key_type_asymmetric)
>  		return;
>  
> -	if (!payload || (plen == 0))
> +	if (!payload || (payload_len == 0))
>  		return;
>  
>  	/*
> @@ -52,7 +179,7 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
>  	 * if the IMA policy is configured to measure a key linked
>  	 * to the given keyring.
>  	 */
> -	process_buffer_measurement(payload, plen,
> +	process_buffer_measurement(payload, payload_len,
>  				   keyring->description, KEY_CHECK, 0,
>  				   keyring->description);
>  }

Re: [PATCH v0 1/2] IMA: Defined queue functions

[Lakshmi Ramasubramanian]

On 11/27/19 12:38 PM, Mimi Zohar wrote:

> Hi Lakshmi,
> 
> Janne Karhunen is defining an IMA workqueue in order to more
> frequently update the on disk security xattrs.  

Has the above patch set been posted for review? I'll take a look and see 
if that one can be used for queuing keys.

The Subject line on
> this patch needs to be more explicit (eg. define workqueue for early
> boot "key" measurements).

Will update the subject line.
I was trying to keep the subject line short and have more details in the 
patch description.

> I'm not sure why you want to differentiate between IMA being
> initialized vs. an empty policy.  I would think you would want to know
> when a custom policy has been loaded.

You are right - When custom ima policy rules are loaded (in 
ima_update_policy() function), ima_process_queued_keys_for_measurement() 
function is called to process queued keys.

The flag ima_process_keys_for_measurement is set to true in 
ima_process_queued_keys_for_measurement(). And, subsequent keys are 
processed immediately.

Please take a look at ima_process_queued_keys_for_measurement() in this 
patch (v0 1/2) and the ima_update_policy() change in "PATCH v0 2/2".

> 
> I would define a function that determines whether or not a custom
> policy has been loaded.

The queued keys need to be processed once when the custom policy is 
loaded. Subsequently, keys are processed immediately (not queued).

Do you still think there is a need to have a function to determine if 
custom policy has been loaded? Wouldn't the flag 
ima_process_keys_for_measurement be sufficient?

Please take a look at "PATCH v0 2/2" and let me know if you disagree.

> (I still need to review adding/removing from the queue.)
> 
>>
>> @@ -27,14 +154,14 @@
>>    * The payload data used to instantiate or update the key is measured.
>>    */
>>   void ima_post_key_create_or_update(struct key *keyring, struct key *key,
>> -				   const void *payload, size_t plen,
>> +				   const void *payload, size_t payload_len,
>>   				   unsigned long flags, bool create)
> 
> This "hunk" and subsequent one seem to be just a variable name change.
>   It has nothing to do with queueing "key" measurements and shouldn't
> be included in this patch.
> 
> Mimi

I'll remove this change.

thanks,
  -lakshmi

Re: [PATCH v0 1/2] IMA: Defined queue functions

[Mimi Zohar]

On Wed, 2019-11-27 at 13:11 -0800, Lakshmi Ramasubramanian wrote:
> On 11/27/19 12:38 PM, Mimi Zohar wrote:

> > I'm not sure why you want to differentiate between IMA being
> > initialized vs. an empty policy.  I would think you would want to know
> > when a custom policy has been loaded.
> 
> You are right - When custom ima policy rules are loaded (in 
> ima_update_policy() function), ima_process_queued_keys_for_measurement() 
> function is called to process queued keys.
> 
> The flag ima_process_keys_for_measurement is set to true in 
> ima_process_queued_keys_for_measurement(). And, subsequent keys are 
> processed immediately.
> 
> Please take a look at ima_process_queued_keys_for_measurement() in this 
> patch (v0 1/2) and the ima_update_policy() change in "PATCH v0 2/2".

ima_update_policy() is called from multiple places.  Initially, it is
called before a custom policy has been loaded.  The call to
ima_process_queued_keys_for_measurement() needs to be moved to within 
the test, otherwise it runs the risk of dropping "key" measurements.

All the queued keys need to be processed at the same time.  Afterwards
the queue should be deleted.  Unfortunately, the current queue locking
assumes ima_process_queued_keys_for_measurement() is called multiple
times.

Perhaps using the RCU method of walking lists would help.  I need to
think about it some more.

Mimi

Re: [PATCH v0 1/2] IMA: Defined queue functions

[Lakshmi Ramasubramanian]

On 12/2/19 10:00 AM, Mimi Zohar wrote:

> 
> ima_update_policy() is called from multiple places.  Initially, it is
> called before a custom policy has been loaded.  The call to
> ima_process_queued_keys_for_measurement() needs to be moved to within
> the test, otherwise it runs the risk of dropping "key" measurements.

static const struct file_operations ima_measure_policy_ops = {
	.release = ima_release_policy,
};

ima_update_policy() is called from ima_release_policy() function.

On my test machine I have the IMA policy in /etc/ima/ima-policy file. 
When IMA policy is setup from this file, I see ima_release_policy() 
called (which in turn calls ima_update_policy()).

How can I have ima_update_policy() called before a custom policy is loaded?

If CONFIG_IMA_WRITE_POLICY is enabled, IMA policy can be updated at 
runtime - which would invoke ima_update_policy().

Is there any other way ima_update_policy() can get called?

> All the queued keys need to be processed at the same time.  Afterwards
> the queue should be deleted.  Unfortunately, the current queue locking
> assumes ima_process_queued_keys_for_measurement() is called multiple
> times.

When ima_process_queued_keys_for_measurement() is called the first time, 
the flag ima_process_keys_for_measurement is set to true and all queued 
keys are processed at that time.

The queue is not used after this point - In the IMA hook the key is 
processed immediately when ima_process_keys_for_measurement is set to true.

ima_process_queued_keys_for_measurement() can be called multiple times, 
but on 2nd and subsequent calls it will be a NOP.

> 
> Perhaps using the RCU method of walking lists would help.  I need to
> think about it some more.
> 
> Mimi

Please let me know how using RCU method would help.

thanks,
  -lakshmi

Re: [PATCH v0 1/2] IMA: Defined queue functions

[Mimi Zohar]

On Mon, 2019-12-02 at 10:39 -0800, Lakshmi Ramasubramanian wrote:
> On 12/2/19 10:00 AM, Mimi Zohar wrote:
> 
> > 
> > ima_update_policy() is called from multiple places.  Initially, it is
> > called before a custom policy has been loaded.  The call to
> > ima_process_queued_keys_for_measurement() needs to be moved to within
> > the test, otherwise it runs the risk of dropping "key" measurements.
> 
> static const struct file_operations ima_measure_policy_ops = {
> 	.release = ima_release_policy,
> };
> 
> ima_update_policy() is called from ima_release_policy() function.
> 
> On my test machine I have the IMA policy in /etc/ima/ima-policy file. 
> When IMA policy is setup from this file, I see ima_release_policy() 
> called (which in turn calls ima_update_policy()).
> 
> How can I have ima_update_policy() called before a custom policy is loaded?

Oops, you're right.  My concern was ima_init_policy(), but it calls
ima_update_policy_flag() directly.

Mimi

Re: [PATCH v0 1/2] IMA: Defined queue functions

[Lakshmi Ramasubramanian]

On 12/2/19 11:11 AM, Mimi Zohar wrote:

>> How can I have ima_update_policy() called before a custom policy is loaded?
> 
> Oops, you're right.  My concern was ima_init_policy(), but it calls
> ima_update_policy_flag() directly.
> 
> Mimi

Thanks Mimi.

Please let me know if you have any concerns with respect to the deferred 
key processing implementation in this patch set.

Also, if you think Janne Karhunen work queue implementation can be used 
for deferred key measurement also, please post the patch set. I'll take 
a look.

thanks,
  -lakshmi

Re: [PATCH v0 1/2] IMA: Defined queue functions

[Mimi Zohar]

Hi Lakshmi,

On Tue, 2019-11-26 at 18:52 -0800, Lakshmi Ramasubramanian wrote:
> Keys created or updated in the system before IMA is initialized
> should be queued up. And, keys (including any queued ones)
> should be processed when IMA initialization is completed.
> 
> This patch defines functions to queue and dequeue keys for
> measurement. A flag namely ima_process_keys_for_measurement
> is used to check if the key should be queued or should be
> processed immediately.
> 
> ima_policy_flag cannot be relied upon to make queuing decision
> because ima_policy_flag will be set to 0 when either IMA is
> not initialized or when the IMA policy itself is empty.

Already commented on the patch description.  Some additional minor
comments below.

> 
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> ---
>  security/integrity/ima/ima.h                 |  15 +++
>  security/integrity/ima/ima_asymmetric_keys.c | 135 ++++++++++++++++++-
>  2 files changed, 146 insertions(+), 4 deletions(-)
> 
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index f06238e41a7c..f86371647707 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -205,6 +205,21 @@ extern const char *const func_tokens[];
>  
>  struct modsig;
>  
> +#ifdef CONFIG_ASYMMETRIC_PUBLIC_KEY_SUBTYPE
> +/*
> + * To track keys that need to be measured.
> + */
> +struct ima_measure_key_entry {
> +	struct list_head list;
> +	void *payload;
> +	size_t payload_len;
> +	char *keyring_name;
> +};
> +void ima_process_queued_keys_for_measurement(void);
> +#else
> +static inline void ima_process_queued_keys_for_measurement(void) {}
> +#endif /* CONFIG_ASYMMETRIC_PUBLIC_KEY_SUBTYPE */
> +
>  /* LIM API function definitions */
>  int ima_get_action(struct inode *inode, const struct cred *cred, u32 secid,
>  		   int mask, enum ima_hooks func, int *pcr,
> diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c
> index ca895f9a6504..10deb77b22a0 100644
> --- a/security/integrity/ima/ima_asymmetric_keys.c
> +++ b/security/integrity/ima/ima_asymmetric_keys.c
> @@ -14,12 +14,139 @@
>  #include <keys/asymmetric-type.h>
>  #include "ima.h"
>  
> +/*
> + * Flag to indicate whether a key can be processed
> + * right away or should be queued for processing later.
> + */
> +bool ima_process_keys_for_measurement;
> +
> +/*
> + * To synchronize access to the list of keys that need to be measured
> + */
> +static DEFINE_MUTEX(ima_measure_keys_mutex);
> +static LIST_HEAD(ima_measure_keys);
> +
> +static void ima_free_measure_key_entry(struct ima_measure_key_entry *entry)
> +{
> +	if (entry) {
> +		kfree(entry->payload);
> +		kfree(entry->keyring_name);
> +		kfree(entry);
> +	}
> +}
> +
> +static struct ima_measure_key_entry *ima_alloc_measure_key_entry(
> +	struct key *keyring,
> +	const void *payload, size_t payload_len)
> +{
> +	int rc = 0;
> +	struct ima_measure_key_entry *entry = NULL;
> +
> +	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> +	if (entry) {
> +		entry->payload =
> +			kmemdup(payload, payload_len, GFP_KERNEL);
> +		entry->keyring_name =
> +			kstrdup(keyring->description, GFP_KERNEL);

No need to break up the assignments like this.  Neither line when
joined is greater than 80 bytes.

> +		entry->payload_len = payload_len;
> +	}
> +
> +	if ((entry == NULL) || (entry->payload == NULL) ||
> +	    (entry->keyring_name == NULL)) {
> +		rc = -ENOMEM;
> +		goto out;
> +	}
> +
> +	INIT_LIST_HEAD(&entry->list);
> +	rc = 0;

"rc" was initialized in the declaration.

> +
> +out:
> +	if (rc) {
> +		ima_free_measure_key_entry(entry);
> +		entry = NULL;
> +	}
> +
> +	return entry;
> +}
> +
> +bool ima_queue_key_for_measurement(struct key *keyring,
> +				   const void *payload, size_t payload_len)
> +{
> +	bool queued = false;
> +	struct ima_measure_key_entry *entry = NULL;

No need to initialize ima_measure_key_entry here, as the first
statement sets it.
> +
> +	entry = ima_alloc_measure_key_entry(keyring, payload, payload_len);
> +	if (entry) {

If we're ignoring failure to allocate memory, why not just return?

> +		/*
> +		 * ima_measure_keys_mutex should be taken before checking
> +		 * ima_process_keys_for_measurement flag to avoid the race
> +		 * condition between the IMA hook checking this flag and
> +		 * calling ima_queue_key_for_measurement() to queue
> +		 * the key and ima_process_queued_keys_for_measurement()
> +		 * setting this flag.
> +		 */
> +		mutex_lock(&ima_measure_keys_mutex);
> +		if (!ima_process_keys_for_measurement) {
> +			list_add_tail(&entry->list, &ima_measure_keys);
> +			queued = true;
> +		}
> +		mutex_unlock(&ima_measure_keys_mutex);
> +
> +		if (!queued)
> +			ima_free_measure_key_entry(entry);
> +	}
> +
> +	return queued;
> +}
> +
> +void ima_process_queued_keys_for_measurement(void)
> +{
> +	struct ima_measure_key_entry *entry, *tmp;
> +	LIST_HEAD(temp_ima_measure_keys);
> +
> +	if (ima_process_keys_for_measurement)
> +		return;
> +
> +	/*
> +	 * Any queued keys will be processed now. From here on
> +	 * keys should be processed right away.
> +	 */
> +	ima_process_keys_for_measurement = true;
> +
> +	/*
> +	 * To avoid holding the mutex when processing queued keys,
> +	 * transfer the queued keys with the mutex held to a temp list,
> +	 * release the mutex, and then process the queued keys from
> +	 * the temp list.
> +	 *
> +	 * Since ima_process_keys_for_measurement is set to true above,
> +	 * any new key will be processed immediately and not be queued.
> +	 */
> +	INIT_LIST_HEAD(&temp_ima_measure_keys);
> +	mutex_lock(&ima_measure_keys_mutex);
> +	list_for_each_entry_safe(entry, tmp, &ima_measure_keys, list) {
> +		list_move_tail(&entry->list, &temp_ima_measure_keys);
> +	}

Parenthesis not needed.

> +	mutex_unlock(&ima_measure_keys_mutex);
> +
> +	list_for_each_entry_safe(entry, tmp,
> +				 &temp_ima_measure_keys, list) {

No need to break up this line either.  The joined line is not greater
than 80 bytes.

> +		process_buffer_measurement(entry->payload,
> +					   entry->payload_len,
> +					   entry->keyring_name,
> +					   KEY_CHECK, 0,
> +					   entry->keyring_name);
> +		list_del(&entry->list);
> +		ima_free_measure_key_entry(entry);
> +	}
> +}
> +

Re: [PATCH v0 2/2] IMA: Call queue functions to measure keys

[Mimi Zohar]

Hi Lakshmi,

On Tue, 2019-11-26 at 18:52 -0800, Lakshmi Ramasubramanian wrote:
> Keys should be queued for measurement if custom IMA policies have
> not yet been applied. Keys queued for measurement, if any, need to be
> processed when custom IMA policies have been applied.

Please start with the problem description.  For example, measuring
keys requires loading a custom IMA policy.

> 
> This patch adds the call to ima_queue_key_for_measurement() in
> the IMA hook function if ima_process_keys_for_measurement flag is set
> to false. And, the call to ima_process_queued_keys_for_measurement()
> when custom IMA policies have been applied in ima_update_policy().

This reads like pseudo code.  Please summarize the purpose of this
patch.

> 
> NOTE:
> If the kernel is built with CONFIG_ASYMMETRIC_PUBLIC_KEY_SUBTYPE
> enabled then the IMA policy should be applied as custom IMA policies.
> 
> Keys will be queued up until custom policies are applied and processed
> when custom policies have been applied.
> 
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> ---
>  security/integrity/ima/ima_asymmetric_keys.c | 16 ++++++++++++++++
>  security/integrity/ima/ima_policy.c          | 12 ++++++++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c
> index 10deb77b22a0..adb7a307190f 100644
> --- a/security/integrity/ima/ima_asymmetric_keys.c
> +++ b/security/integrity/ima/ima_asymmetric_keys.c
> @@ -157,6 +157,8 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
>  				   const void *payload, size_t payload_len,
>  				   unsigned long flags, bool create)
>  {
> +	bool key_queued = false;
> +
>  	/* Only asymmetric keys are handled by this hook. */
>  	if (key->type != &key_type_asymmetric)
>  		return;
> @@ -164,6 +166,20 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
>  	if (!payload || (payload_len == 0))
>  		return;
>  
> +	if (!ima_process_keys_for_measurement)
> +		key_queued = ima_queue_key_for_measurement(keyring,
> +							   payload,
> +							   payload_len);
> +
> +	/*
> +	 * Need to check again if the key was queued or not because
> +	 * ima_process_keys_for_measurement could have flipped from
> +	 * false to true after it was checked above, but before the key
> +	 * could be queued by ima_queue_key_for_measurement().
> +	 */

You're describing a race condition.

> +	if (key_queued)
> +		return;
> +
>  	/*
>  	 * keyring->description points to the name of the keyring
>  	 * (such as ".builtin_trusted_keys", ".ima", etc.) to
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 78b25f083fe1..a2e30a90f97d 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -812,6 +812,18 @@ void ima_update_policy(void)
>  		kfree(arch_policy_entry);
>  	}
>  	ima_update_policy_flag();
> +
> +	/*
> +	 * Custom IMA policies have been setup.
> +	 * Process key(s) queued up for measurement now.
> +	 *
> +	 * NOTE:
> +	 *   Custom IMA policies always overwrite builtin policies
> +	 *   (policies compiled in code). If one wants measurement
> +	 *   of asymmetric keys then it has to be configured in
> +	 *   custom policies and updated here.
> +	 */

The "NOTE" is over commenting the code and belongs in the patch
description.

> +	ima_process_queued_keys_for_measurement();

Overwriting the initial policy is highly recommended, but not everyone
defines a custom policy.  Should there be a time limit or some other
criteria before deleting the key measurement queue?

Mimi

>  }
>  
>  /* Keep the enumeration in sync with the policy_tokens! */

Re: [PATCH v0 2/2] IMA: Call queue functions to measure keys

[Lakshmi Ramasubramanian]

Thanks for reviewing the changes Mimi. I'll address your comments in the 
next update.

> 
> Overwriting the initial policy is highly recommended, but not everyone
> defines a custom policy.  Should there be a time limit or some other
> criteria before deleting the key measurement queue?
> 
> Mimi

For the above, I feel checking for the presence of custom policy, if 
that is possible, would be a more deterministic approach compared to 
having a time limit.

On my machine, systemd loads the custom IMA policy from 
/etc/ima/ima-policy if that file is present. Is this the recommended way 
to configure custom IMA policy? If yes, can the IMA initialization 
function check for the presence of the above file?

Another way to address this issue is to define a new CONFIG parameter to 
determine whether or not to support deferred processing of keys. If this 
config is chosen, custom IMA policy must be defined.

Least preferred option would be to leave the queued keys as is if custom 
policy is not defined - at the cost of, maybe, a non-trivial amount of 
kernel memory consumed.

If detection of custom policy is not possible, then define a timer to 
drain the key measurement queue.

Please let me know which approach you think is optimal.

thanks,
  -lakshmi