| Message ID | 20191213004250.21132-2-nramas@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | IMA: Deferred measurement of keys | expand |
> +/* > + * 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; > + LIST_HEAD(temp_ima_keys); > + > + if (ima_process_keys) > + return; > + > + /* > + * 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 is set to true, any new key will be > + * processed immediately and not be queued. > + */ > + INIT_LIST_HEAD(&temp_ima_keys); > + > + mutex_lock(&ima_keys_mutex); Don't you need a test here, before setting ima_process_keys? if (ima_process_keys) return; Mimi > + > + ima_process_keys = true; > + > + list_for_each_entry_safe(entry, tmp, &ima_keys, list) > + list_move_tail(&entry->list, &temp_ima_keys); > + > + mutex_unlock(&ima_keys_mutex); > + > + list_for_each_entry_safe(entry, tmp, &temp_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); > + } > +} > + >
On 12/12/19 5:55 PM, Mimi Zohar 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; >> + LIST_HEAD(temp_ima_keys); >> + >> + if (ima_process_keys) >> + return; >> + >> + /* >> + * 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 is set to true, any new key will be >> + * processed immediately and not be queued. >> + */ >> + INIT_LIST_HEAD(&temp_ima_keys); >> + >> + mutex_lock(&ima_keys_mutex); > > Don't you need a test here, before setting ima_process_keys? > > if (ima_process_keys) > return; > > Mimi That check is done before the comment - at the start of ima_process_queued_keys(). +void ima_process_queued_keys(void) +{ + struct ima_key_entry *entry, *tmp; + LIST_HEAD(temp_ima_keys); + + if (ima_process_keys) + return; thanks, -lakshmi
On Thu, 2019-12-12 at 17:59 -0800, Lakshmi Ramasubramanian wrote: > On 12/12/19 5:55 PM, Mimi Zohar 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; > >> + LIST_HEAD(temp_ima_keys); > >> + > >> + if (ima_process_keys) > >> + return; > >> + > >> + /* > >> + * 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 is set to true, any new key will be > >> + * processed immediately and not be queued. > >> + */ > >> + INIT_LIST_HEAD(&temp_ima_keys); > >> + > >> + mutex_lock(&ima_keys_mutex); > > > > Don't you need a test here, before setting ima_process_keys? > > > > if (ima_process_keys) > > return; > > > > Mimi > > That check is done before the comment - at the start of > ima_process_queued_keys(). The first test prevents taking the mutex unnecessarily. Mimi > +void ima_process_queued_keys(void) > +{ > + struct ima_key_entry *entry, *tmp; > + LIST_HEAD(temp_ima_keys); > + > + if (ima_process_keys) > + return; > > thanks, > -lakshmi
On 12/12/2019 6:32 PM, Mimi Zohar wrote: >>> >>> Don't you need a test here, before setting ima_process_keys? >>> >>> if (ima_process_keys) >>> return; >>> >>> Mimi >> >> That check is done before the comment - at the start of >> ima_process_queued_keys(). > > The first test prevents taking the mutex unnecessarily. > > Mimi I am trying to understand your concern here. Could you please clarify? => If ima_process_keys is false -> With the mutex held, should check ima_process_keys again before setting? Let's say 2 or more threads are racing in calling ima_process_queued_keys(): The 1st one will set ima_process_keys and process queued keys. The 2nd and subsequent ones - even if they have gone past the initial check, will find an empty list of keys (the list "ima_keys") when they take the mutex. So they'll not process any keys. thanks, -lakshmi
On Thu, 2019-12-12 at 18:59 -0800, Lakshmi Ramasubramanian wrote: > On 12/12/2019 6:32 PM, Mimi Zohar wrote: > > >>> > >>> Don't you need a test here, before setting ima_process_keys? > >>> > >>> if (ima_process_keys) > >>> return; > >> That check is done before the comment - at the start of > >> ima_process_queued_keys(). > > > > The first test prevents taking the mutex unnecessarily. > > > > I am trying to understand your concern here. Could you please clarify? > > => If ima_process_keys is false > -> With the mutex held, should check ima_process_keys again > before setting? > > Let's say 2 or more threads are racing in calling ima_process_queued_keys(): > > The 1st one will set ima_process_keys and process queued keys. > > The 2nd and subsequent ones - even if they have gone past the initial > check, will find an empty list of keys (the list "ima_keys") when they > take the mutex. So they'll not process any keys. I just need to convince myself that this is correct. Normally before reading and writing a flag, there is some sort of locking. With taking the mutex before setting the flag, there is now only a lock around the single writer. Without taking a lock before reading the flag, will the queue always be empty is the question. If it is, then the comment is correct, but the code assumes not and processes the list again. Testing the flag after taking the mutex just re-enforces the comment. Bottom line, does reading the flag need to be lock protected? Mimi
On 12/13/19 5:06 AM, Mimi Zohar wrote: > I just need to convince myself that this is correct. Normally before > reading and writing a flag, there is some sort of locking. With > taking the mutex before setting the flag, there is now only a lock > around the single writer. > > Without taking a lock before reading the flag, will the queue always > be empty is the question. If it is, then the comment is correct, but > the code assumes not and processes the list again. Testing the flag > after taking the mutex just re-enforces the comment. > > Bottom line, does reading the flag need to be lock protected? > > Mimi > I'll change this function to check the flag again after taking the lock and process only if the queue has entries. Will send an update today. Please let me know if you have any concern in other functions in this file. I'll address them, if any, in today's update. thanks, -lakshmi
On Fri, 2019-12-13 at 08:06 -0500, Mimi Zohar wrote: > On Thu, 2019-12-12 at 18:59 -0800, Lakshmi Ramasubramanian wrote: > > On 12/12/2019 6:32 PM, Mimi Zohar wrote: > > > > >>> > > >>> Don't you need a test here, before setting ima_process_keys? > > >>> > > >>> if (ima_process_keys) > > >>> return; > > > >> That check is done before the comment - at the start of > > >> ima_process_queued_keys(). > > > > > > The first test prevents taking the mutex unnecessarily. > > > > > > > I am trying to understand your concern here. Could you please clarify? > > > > => If ima_process_keys is false > > -> With the mutex held, should check ima_process_keys again > > before setting? > > > > Let's say 2 or more threads are racing in calling ima_process_queued_keys(): > > > > The 1st one will set ima_process_keys and process queued keys. > > > > The 2nd and subsequent ones - even if they have gone past the initial > > check, will find an empty list of keys (the list "ima_keys") when they > > take the mutex. So they'll not process any keys. > > I just need to convince myself that this is correct. Normally before > reading and writing a flag, there is some sort of locking. With > taking the mutex before setting the flag, there is now only a lock > around the single writer. > > Without taking a lock before reading the flag, will the queue always > be empty is the question. If it is, then the comment is correct, but > the code assumes not and processes the list again. Testing the flag > after taking the mutex just re-enforces the comment. > > Bottom line, does reading the flag need to be lock protected? Reading the flag IS lock protected, just spread across two functions. For performance, ima_post_key_create_or_update() checks ima_process_keys, before calling ima_queue_key(), which takes the mutex before checking ima_process_keys again. As long as both the reader and writer, take the mutex before checking the flag, the locking is fine. The additional check, before taking the mutex, is simply for performance. Mimi
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..a1781348ce69 100644 --- a/security/integrity/ima/ima_asymmetric_keys.c +++ b/security/integrity/ima/ima_asymmetric_keys.c @@ -14,6 +14,125 @@ #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; + LIST_HEAD(temp_ima_keys); + + if (ima_process_keys) + return; + + /* + * 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 is set to true, any new key will be + * processed immediately and not be queued. + */ + INIT_LIST_HEAD(&temp_ima_keys); + + mutex_lock(&ima_keys_mutex); + + ima_process_keys = true; + + list_for_each_entry_safe(entry, tmp, &ima_keys, list) + list_move_tail(&entry->list, &temp_ima_keys); + + mutex_unlock(&ima_keys_mutex); + + list_for_each_entry_safe(entry, tmp, &temp_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
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 | 119 +++++++++++++++++++ 2 files changed, 134 insertions(+)