* [PATCH v5 1/2] IMA: Define workqueue for early boot "key" measurements
2019-12-18 16:44 [PATCH v5 0/2] IMA: Deferred measurement of keys Lakshmi Ramasubramanian
@ 2019-12-18 16:44 ` Lakshmi Ramasubramanian
2019-12-19 13:11 ` Mimi Zohar
2019-12-18 16:44 ` [PATCH v5 2/2] IMA: Call workqueue functions to measure queued keys Lakshmi Ramasubramanian
2019-12-20 19:01 ` [PATCH v5 0/2] IMA: Deferred measurement of keys Mimi Zohar
2 siblings, 1 reply; 10+ messages in thread
From: Lakshmi Ramasubramanian @ 2019-12-18 16:44 UTC (permalink / raw)
To: zohar, James.Bottomley, linux-integrity
Cc: eric.snowberg, dhowells, mathew.j.martineau, matthewgarrett,
sashal, jamorris, linux-kernel, keyrings
Measuring keys requires a custom IMA policy to be loaded.
Keys created or updated before a custom IMA policy is loaded should
be queued and the keys should be processed after a custom policy
is loaded.
This patch defines workqueue for queuing keys when a custom IMA policy
has not yet been loaded.
A flag namely ima_process_keys is used to check if the key should be
queued or should be processed immediately.
Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
security/integrity/ima/ima.h | 15 +++
security/integrity/ima/ima_asymmetric_keys.c | 115 +++++++++++++++++++
2 files changed, 130 insertions(+)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index f06238e41a7c..97f8a4078483 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_key_entry {
+ struct list_head list;
+ void *payload;
+ size_t payload_len;
+ char *keyring_name;
+};
+void ima_process_queued_keys(void);
+#else
+static inline void ima_process_queued_keys(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 fea2e7dd3b09..d520a67180d8 100644
--- a/security/integrity/ima/ima_asymmetric_keys.c
+++ b/security/integrity/ima/ima_asymmetric_keys.c
@@ -14,6 +14,121 @@
#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.
+ */
+static bool ima_process_keys;
+
+/*
+ * To synchronize access to the list of keys that need to be measured
+ */
+static DEFINE_MUTEX(ima_keys_mutex);
+static LIST_HEAD(ima_keys);
+
+static void ima_free_key_entry(struct ima_key_entry *entry)
+{
+ if (entry) {
+ kfree(entry->payload);
+ kfree(entry->keyring_name);
+ kfree(entry);
+ }
+}
+
+static struct ima_key_entry *ima_alloc_key_entry(
+ struct key *keyring,
+ const void *payload, size_t payload_len)
+{
+ int rc = 0;
+ struct ima_key_entry *entry;
+
+ 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);
+
+out:
+ if (rc) {
+ ima_free_key_entry(entry);
+ entry = NULL;
+ }
+
+ return entry;
+}
+
+bool ima_queue_key(struct key *keyring, const void *payload,
+ size_t payload_len)
+{
+ bool queued = false;
+ struct ima_key_entry *entry;
+
+ entry = ima_alloc_key_entry(keyring, payload, payload_len);
+ if (!entry)
+ return false;
+
+ mutex_lock(&ima_keys_mutex);
+ if (!ima_process_keys) {
+ list_add_tail(&entry->list, &ima_keys);
+ queued = true;
+ }
+ mutex_unlock(&ima_keys_mutex);
+
+ if (!queued)
+ ima_free_key_entry(entry);
+
+ return queued;
+}
+
+/*
+ * ima_process_queued_keys() - process keys queued for measurement
+ *
+ * This function sets ima_process_keys to true and processes queued keys.
+ * From here on keys will be processed right away (not queued).
+ */
+void ima_process_queued_keys(void)
+{
+ struct ima_key_entry *entry, *tmp;
+ bool process = false;
+
+ if (ima_process_keys)
+ return;
+
+ /*
+ * Since ima_process_keys is set to true, any new key will be
+ * processed immediately and not be queued to ima_keys list.
+ * First one setting the ima_process_keys flag to true will
+ * process the queued keys.
+ */
+ mutex_lock(&ima_keys_mutex);
+ if (!ima_process_keys) {
+ ima_process_keys = true;
+ process = true;
+ }
+ mutex_unlock(&ima_keys_mutex);
+
+ if (!process)
+ return;
+
+ list_for_each_entry_safe(entry, tmp, &ima_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_key_entry(entry);
+ }
+}
+
/**
* ima_post_key_create_or_update - measure asymmetric keys
* @keyring: keyring to which the key is linked to
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v5 1/2] IMA: Define workqueue for early boot "key" measurements
2019-12-18 16:44 ` [PATCH v5 1/2] IMA: Define workqueue for early boot "key" measurements Lakshmi Ramasubramanian
@ 2019-12-19 13:11 ` Mimi Zohar
2019-12-19 16:55 ` Lakshmi Ramasubramanian
0 siblings, 1 reply; 10+ messages in thread
From: Mimi Zohar @ 2019-12-19 13:11 UTC (permalink / raw)
To: Lakshmi Ramasubramanian, James.Bottomley, linux-integrity
Cc: eric.snowberg, dhowells, mathew.j.martineau, matthewgarrett,
sashal, jamorris, linux-kernel, keyrings
On Wed, 2019-12-18 at 08:44 -0800, Lakshmi Ramasubramanian wrote:
> +/*
> + * ima_process_queued_keys() - process keys queued for measurement
> + *
> + * This function sets ima_process_keys to true and processes queued keys.
> + * From here on keys will be processed right away (not queued).
> + */
> +void ima_process_queued_keys(void)
> +{
> + struct ima_key_entry *entry, *tmp;
> + bool process = false;
> +
> + if (ima_process_keys)
> + return;
> +
> + /*
> + * Since ima_process_keys is set to true, any new key will be
> + * processed immediately and not be queued to ima_keys list.
> + * First one setting the ima_process_keys flag to true will
> + * process the queued keys.
> + */
> + mutex_lock(&ima_keys_mutex);
> + if (!ima_process_keys) {
> + ima_process_keys = true;
> + process = true;
> + }
> + mutex_unlock(&ima_keys_mutex);
> +
> + if (!process)
> + return;
> +
> + list_for_each_entry_safe(entry, tmp, &ima_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_key_entry(entry);
> + }
> +}
> +
Getting rid of the temporary list is definitely a big improvement. As
James suggested, using test_and_set_bit() and test_bit() would improve
this code even more. I think, James correct me if I'm wrong, you
would be able to get rid of both the mutex and "process".
Mimi
> /**
> * ima_post_key_create_or_update - measure asymmetric keys
> * @keyring: keyring to which the key is linked to
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 1/2] IMA: Define workqueue for early boot "key" measurements
2019-12-19 13:11 ` Mimi Zohar
@ 2019-12-19 16:55 ` Lakshmi Ramasubramanian
2019-12-20 12:53 ` Mimi Zohar
0 siblings, 1 reply; 10+ messages in thread
From: Lakshmi Ramasubramanian @ 2019-12-19 16:55 UTC (permalink / raw)
To: Mimi Zohar, James.Bottomley, linux-integrity
Cc: eric.snowberg, dhowells, mathew.j.martineau, matthewgarrett,
sashal, jamorris, linux-kernel, keyrings
On 12/19/19 5:11 AM, Mimi Zohar wrote:
>
> Getting rid of the temporary list is definitely a big improvement. As
> James suggested, using test_and_set_bit() and test_bit() would improve
> this code even more. I think, James correct me if I'm wrong, you
> would be able to get rid of both the mutex and "process".
>
> Mimi
I am not sure if the mutex can be removed.
In ima_queue_key() we need to test the flag and add the key to the list
as an atomic operation:
if (!test_bit())
insert_key_to_list
Suppose the if condition is true, but before we could insert the key to
the list, ima_process_queued_keys() runs and processes queued keys we'll
add the key to the list and never process it.
Is there an API in the kernel to test and add an entry to a list
atomically?
thanks,
-lakshmi
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 1/2] IMA: Define workqueue for early boot "key" measurements
2019-12-19 16:55 ` Lakshmi Ramasubramanian
@ 2019-12-20 12:53 ` Mimi Zohar
0 siblings, 0 replies; 10+ messages in thread
From: Mimi Zohar @ 2019-12-20 12:53 UTC (permalink / raw)
To: Lakshmi Ramasubramanian, James.Bottomley, linux-integrity
Cc: eric.snowberg, dhowells, mathew.j.martineau, matthewgarrett,
sashal, jamorris, linux-kernel, keyrings
On Thu, 2019-12-19 at 08:55 -0800, Lakshmi Ramasubramanian wrote:
> I am not sure if the mutex can be removed.
>
> In ima_queue_key() we need to test the flag and add the key to the list
> as an atomic operation:
>
> if (!test_bit())
> insert_key_to_list
>
> Suppose the if condition is true, but before we could insert the key to
> the list, ima_process_queued_keys() runs and processes queued keys we'll
> add the key to the list and never process it.
>
> Is there an API in the kernel to test and add an entry to a list
> atomically?
Ok, using test_and_set_bit() and test_bit() only helps, if we can get
rid of the mutex. I'll queue these patches.
thanks,
Mimi
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v5 2/2] IMA: Call workqueue functions to measure queued keys
2019-12-18 16:44 [PATCH v5 0/2] IMA: Deferred measurement of keys Lakshmi Ramasubramanian
2019-12-18 16:44 ` [PATCH v5 1/2] IMA: Define workqueue for early boot "key" measurements Lakshmi Ramasubramanian
@ 2019-12-18 16:44 ` Lakshmi Ramasubramanian
2019-12-20 19:01 ` [PATCH v5 0/2] IMA: Deferred measurement of keys Mimi Zohar
2 siblings, 0 replies; 10+ messages in thread
From: Lakshmi Ramasubramanian @ 2019-12-18 16:44 UTC (permalink / raw)
To: zohar, James.Bottomley, linux-integrity
Cc: eric.snowberg, dhowells, mathew.j.martineau, matthewgarrett,
sashal, jamorris, linux-kernel, keyrings
Measuring keys requires a custom IMA policy to be loaded.
Keys should be queued for measurement if a custom IMA policy
is not yet loaded. Keys queued for measurement, if any, should be
processed when a custom IMA policy is loaded.
This patch updates the IMA hook function ima_post_key_create_or_update()
to queue the key if a custom IMA policy has not yet been loaded.
And, ima_update_policy() function, which is called when
a custom IMA policy is loaded, is updated to process queued keys.
Sample "key" measurement rule in the IMA policy:
measure func=KEY_CHECK uid=0 keyrings=.ima|.builtin_trusted_keys template=ima-buf
If the kernel is built with one or more built-in trusted certificates,
IMA measurement should list all the keys imported from those certificates.
Display "key" measurement in the IMA measurement list:
cat /sys/kernel/security/ima/ascii_runtime_measurements
10 faf3...e702 ima-buf sha256:27c915b8ddb9fae7214cf0a8a7043cc3eeeaa7539bcb136f8427067b5f6c3b7b .builtin_trusted_keys 308202863082...4aee
Verify "key" measurement data for a key added to ".builtin_trusted_keys" keyring:
cat /sys/kernel/security/integrity/ima/ascii_runtime_measurements | grep -m 1 "\.builtin_trusted_keys" | cut -d' ' -f 6 | xxd -r -p |tee btk-cert.der | sha256sum | cut -d' ' -f 1
The output of the above command should match the template hash
of the first "key" measurement entry in the IMA measurement list for
the key added to ".builtin_trusted_keys" keyring.
The file namely "btk-cert.der" generated by the above command
should be a valid x509 certificate (in DER format) and should match
the one that was used to import the key to the ".builtin_trusted_keys" keyring.
The certificate file can be verified using openssl tool.
Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
security/integrity/ima/ima_asymmetric_keys.c | 8 ++++++++
security/integrity/ima/ima_policy.c | 3 +++
2 files changed, 11 insertions(+)
diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c
index d520a67180d8..4124f10ff0c2 100644
--- a/security/integrity/ima/ima_asymmetric_keys.c
+++ b/security/integrity/ima/ima_asymmetric_keys.c
@@ -145,6 +145,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 queued = false;
+
/* Only asymmetric keys are handled by this hook. */
if (key->type != &key_type_asymmetric)
return;
@@ -152,6 +154,12 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
if (!payload || (payload_len == 0))
return;
+ if (!ima_process_keys)
+ queued = ima_queue_key(keyring, payload, payload_len);
+
+ if (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 a4dde9d575b2..04b9c6c555de 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -807,6 +807,9 @@ void ima_update_policy(void)
kfree(arch_policy_entry);
}
ima_update_policy_flag();
+
+ /* Custom IMA policy has been loaded */
+ ima_process_queued_keys();
}
/* Keep the enumeration in sync with the policy_tokens! */
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v5 0/2] IMA: Deferred measurement of keys
2019-12-18 16:44 [PATCH v5 0/2] IMA: Deferred measurement of keys Lakshmi Ramasubramanian
2019-12-18 16:44 ` [PATCH v5 1/2] IMA: Define workqueue for early boot "key" measurements Lakshmi Ramasubramanian
2019-12-18 16:44 ` [PATCH v5 2/2] IMA: Call workqueue functions to measure queued keys Lakshmi Ramasubramanian
@ 2019-12-20 19:01 ` Mimi Zohar
2019-12-20 19:25 ` Lakshmi Ramasubramanian
2 siblings, 1 reply; 10+ messages in thread
From: Mimi Zohar @ 2019-12-20 19:01 UTC (permalink / raw)
To: Lakshmi Ramasubramanian, James.Bottomley, linux-integrity
Cc: eric.snowberg, dhowells, mathew.j.martineau, matthewgarrett,
sashal, jamorris, linux-kernel, keyrings
On Wed, 2019-12-18 at 08:44 -0800, Lakshmi Ramasubramanian wrote:
> 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 asymmetric keys when the key is created or updated.
> But keys created or updated before a custom IMA policy is loaded
> are currently not measured. This includes keys added to, for instance,
> .builtin_trusted_keys which happens early in the boot process.
>
> This change adds support for queuing keys created or updated before
> a custom IMA policy is loaded. The queued keys are processed when
> a custom policy is loaded. Keys created or updated after a custom policy
> is loaded are measured immediately (not queued).
>
> If the kernel is built with both CONFIG_IMA and
> CONFIG_ASYMMETRIC_PUBLIC_KEY_SUBTYPE enabled then the IMA policy
> must be applied as a custom policy. Not providing a custom policy
> in the above configuration would result in asymmeteric keys being queued
> until a custom policy is loaded. This is by design.
I didn't notice the "This is by design" here, referring to the memory
never being freed. "This is by design" was suppose to refer to
requiring a custom policy for measuring keys.
For now, these two patches are queued in the next-integrity-testing
branch, but I would appreciate your addressing not freeing the memory
associated with the keys, if a custom policy is not loaded.
Please note that I truncated the 2/2 patch description, as it repeats
the existing verification example in commit ("2b60c0ecedf8 IMA: Read
keyrings= option from the IMA policy").
thanks,
Mimi
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 0/2] IMA: Deferred measurement of keys
2019-12-20 19:01 ` [PATCH v5 0/2] IMA: Deferred measurement of keys Mimi Zohar
@ 2019-12-20 19:25 ` Lakshmi Ramasubramanian
2019-12-20 19:36 ` Mimi Zohar
0 siblings, 1 reply; 10+ messages in thread
From: Lakshmi Ramasubramanian @ 2019-12-20 19:25 UTC (permalink / raw)
To: Mimi Zohar, James.Bottomley, linux-integrity
Cc: eric.snowberg, dhowells, mathew.j.martineau, matthewgarrett,
sashal, jamorris, linux-kernel, keyrings
On 12/20/2019 11:01 AM, Mimi Zohar wrote:
Hi Mimi,
>> If the kernel is built with both CONFIG_IMA and
>> CONFIG_ASYMMETRIC_PUBLIC_KEY_SUBTYPE enabled then the IMA policy
>> must be applied as a custom policy. Not providing a custom policy
>> in the above configuration would result in asymmeteric keys being queued
>> until a custom policy is loaded. This is by design.
>
> I didn't notice the "This is by design" here, referring to the memory
> never being freed. "This is by design" was suppose to refer to
> requiring a custom policy for measuring keys.
>
> For now, these two patches are queued in the next-integrity-testing
> branch, but I would appreciate your addressing not freeing the memory
> associated with the keys, if a custom policy is not loaded.
>
> Please note that I truncated the 2/2 patch description, as it repeats
> the existing verification example in commit ("2b60c0ecedf8 IMA: Read
> keyrings= option from the IMA policy").
>
> thanks,
>
> Mimi
>
Sure - I am fine with truncating the 2/2 patch description. Thanks for
doing that.
Regarding "Freeing the queued keys if custom policy is not loaded":
Shall I create a new patch set to address that and have that be reviewed
independent of this patch set?
Like you'd suggested earlier, we can wait for a certain time, after IMA
is initialized, and free the queue if a custom policy was not loaded.
Please let me know.
thanks,
-lakshmi
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 0/2] IMA: Deferred measurement of keys
2019-12-20 19:25 ` Lakshmi Ramasubramanian
@ 2019-12-20 19:36 ` Mimi Zohar
2019-12-20 20:50 ` Lakshmi Ramasubramanian
0 siblings, 1 reply; 10+ messages in thread
From: Mimi Zohar @ 2019-12-20 19:36 UTC (permalink / raw)
To: Lakshmi Ramasubramanian, James.Bottomley, linux-integrity
Cc: eric.snowberg, dhowells, mathew.j.martineau, matthewgarrett,
sashal, jamorris, linux-kernel, keyrings
On Fri, 2019-12-20 at 11:25 -0800, Lakshmi Ramasubramanian wrote:
> On 12/20/2019 11:01 AM, Mimi Zohar wrote:
>
> Hi Mimi,
>
> >> If the kernel is built with both CONFIG_IMA and
> >> CONFIG_ASYMMETRIC_PUBLIC_KEY_SUBTYPE enabled then the IMA policy
> >> must be applied as a custom policy. Not providing a custom policy
> >> in the above configuration would result in asymmeteric keys being queued
> >> until a custom policy is loaded. This is by design.
> >
> > I didn't notice the "This is by design" here, referring to the memory
> > never being freed. "This is by design" was suppose to refer to
> > requiring a custom policy for measuring keys.
> >
> > For now, these two patches are queued in the next-integrity-testing
> > branch, but I would appreciate your addressing not freeing the memory
> > associated with the keys, if a custom policy is not loaded.
> >
> > Please note that I truncated the 2/2 patch description, as it repeats
> > the existing verification example in commit ("2b60c0ecedf8 IMA: Read
> > keyrings= option from the IMA policy").
> >
> > thanks,
> >
> > Mimi
> >
>
> Sure - I am fine with truncating the 2/2 patch description. Thanks for
> doing that.
>
> Regarding "Freeing the queued keys if custom policy is not loaded":
>
> Shall I create a new patch set to address that and have that be reviewed
> independent of this patch set?
If it is just a single additional patch, feel free to post it without
a cover letter.
>
> Like you'd suggested earlier, we can wait for a certain time, after IMA
> is initialized, and free the queue if a custom policy was not loaded.
Different types of systems vary in boot time, but perhaps a certain
amount of time after IMA is initialized would be consistent. This
would need to work for IoT devices/sensors to servers.
Mimi
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 0/2] IMA: Deferred measurement of keys
2019-12-20 19:36 ` Mimi Zohar
@ 2019-12-20 20:50 ` Lakshmi Ramasubramanian
0 siblings, 0 replies; 10+ messages in thread
From: Lakshmi Ramasubramanian @ 2019-12-20 20:50 UTC (permalink / raw)
To: Mimi Zohar, James.Bottomley, linux-integrity
Cc: eric.snowberg, dhowells, mathew.j.martineau, matthewgarrett,
sashal, jamorris, linux-kernel, keyrings
On 12/20/19 11:36 AM, Mimi Zohar wrote:
>>
>> Shall I create a new patch set to address that and have that be reviewed
>> independent of this patch set?
>
> If it is just a single additional patch, feel free to post it without
> a cover letter.
Sure
>>
>> Like you'd suggested earlier, we can wait for a certain time, after IMA
>> is initialized, and free the queue if a custom policy was not loaded.
>
> Different types of systems vary in boot time, but perhaps a certain
> amount of time after IMA is initialized would be consistent. This
> would need to work for IoT devices/sensors to servers.
>
> Mimi
>
Yes - I agree.
thanks,
-lakshmi
^ permalink raw reply [flat|nested] 10+ messages in thread