| Message ID | 20191127025212.3077-2-nramas@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | IMA: Deferred measurement of keys | expand |
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); > }
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
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
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
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
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
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); > + } > +} > +
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); }
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(-)