From: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> To: Mimi Zohar <zohar@linux.ibm.com>, dhowells@redhat.com, matthewgarrett@google.com, sashal@kernel.org, jamorris@linux.microsoft.com, linux-integrity@vger.kernel.org, linux-security-module@vger.kernel.org, keyrings@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v4 08/10] IMA: Defined functions to queue and dequeue keys for measurement Date: Wed, 06 Nov 2019 23:52:31 +0000 [thread overview] Message-ID: <8b2fd578-7429-f5b8-4286-1face91e1ae6@linux.microsoft.com> (raw) In-Reply-To: <1573080281.5028.314.camel@linux.ibm.com> On 11/6/2019 2:44 PM, Mimi Zohar wrote: >> +int ima_queue_or_process_key_for_measurement(struct key *keyring, >> + struct key *key) >> +{ >> + int rc = 0; >> + struct ima_measure_key_entry *entry = NULL; >> + const struct public_key *pk; >> + >> + if (key->type != &key_type_asymmetric) >> + return 0; >> + >> + mutex_lock(&ima_measure_keys_mutex); > > Unless the key is being queued, there's no reason to take the lock. Reason the lock is taken even in the case the key is not queued is to avoid the following race condition: => ima_init() sets ima_initialized flag and calls the dequeue function => If IMA hook checks ima_initialized flag outside the lock and sees the flag is not set, it will call the queue function. => If the above two steps race, the key could get added to the queue after ima_init() has processed the queued keys. That's the reason I named the function called by the IMA hook to ima_queue_or_process_key_for_measurement(). But I can make the following change: => IMA hook checks the flag. => If it is set, process key immediately => If the flag is not set, call ima_queue_or_process_key_for_measurement() ima_queue_or_process_key_for_measurement() will do the following: => With the lock held check ima_initialized flag => If true release the lock and call process_buffer_measurement() => If false, queue the key and then release the lock Would that be acceptable? > Measuring the key should be done in ima_post_key_create_or_update() > unless, it is being deferred.  Please update the function name to > reflect this. Just wanted to confirm: Rename ima_post_key_create_or_update() to a more appropriate name? Another reason for doing all key related operations in ima_queue_or_process_key_for_measurement() is to isolate key related code in a separate C file and build it if and only if the CONFIG dependencies are met. With respect to loading custom policy, I will take a look at how to handle that case. Thanks for pointing that out. > Mimi thanks, -lakshmi
WARNING: multiple messages have this Message-ID (diff)
From: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> To: Mimi Zohar <zohar@linux.ibm.com>, dhowells@redhat.com, matthewgarrett@google.com, sashal@kernel.org, jamorris@linux.microsoft.com, linux-integrity@vger.kernel.org, linux-security-module@vger.kernel.org, keyrings@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v4 08/10] IMA: Defined functions to queue and dequeue keys for measurement Date: Wed, 6 Nov 2019 15:52:31 -0800 [thread overview] Message-ID: <8b2fd578-7429-f5b8-4286-1face91e1ae6@linux.microsoft.com> (raw) In-Reply-To: <1573080281.5028.314.camel@linux.ibm.com> On 11/6/2019 2:44 PM, Mimi Zohar wrote: >> +int ima_queue_or_process_key_for_measurement(struct key *keyring, >> + struct key *key) >> +{ >> + int rc = 0; >> + struct ima_measure_key_entry *entry = NULL; >> + const struct public_key *pk; >> + >> + if (key->type != &key_type_asymmetric) >> + return 0; >> + >> + mutex_lock(&ima_measure_keys_mutex); > > Unless the key is being queued, there's no reason to take the lock. Reason the lock is taken even in the case the key is not queued is to avoid the following race condition: => ima_init() sets ima_initialized flag and calls the dequeue function => If IMA hook checks ima_initialized flag outside the lock and sees the flag is not set, it will call the queue function. => If the above two steps race, the key could get added to the queue after ima_init() has processed the queued keys. That's the reason I named the function called by the IMA hook to ima_queue_or_process_key_for_measurement(). But I can make the following change: => IMA hook checks the flag. => If it is set, process key immediately => If the flag is not set, call ima_queue_or_process_key_for_measurement() ima_queue_or_process_key_for_measurement() will do the following: => With the lock held check ima_initialized flag => If true release the lock and call process_buffer_measurement() => If false, queue the key and then release the lock Would that be acceptable? > Measuring the key should be done in ima_post_key_create_or_update() > unless, it is being deferred. Please update the function name to > reflect this. Just wanted to confirm: Rename ima_post_key_create_or_update() to a more appropriate name? Another reason for doing all key related operations in ima_queue_or_process_key_for_measurement() is to isolate key related code in a separate C file and build it if and only if the CONFIG dependencies are met. With respect to loading custom policy, I will take a look at how to handle that case. Thanks for pointing that out. > Mimi thanks, -lakshmi
next prev parent reply other threads:[~2019-11-06 23:52 UTC|newest] Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-11-06 19:01 [PATCH v4 0/10] KEYS: Measure keys when they are created or updated Lakshmi Ramasubramanian 2019-11-06 19:01 ` Lakshmi Ramasubramanian 2019-11-06 19:01 ` [PATCH v4 01/10] IMA: Defined an IMA hook to measure keys on key create or update Lakshmi Ramasubramanian 2019-11-06 19:01 ` Lakshmi Ramasubramanian 2019-11-06 22:43 ` Mimi Zohar 2019-11-06 22:43 ` Mimi Zohar 2019-11-07 0:21 ` Lakshmi Ramasubramanian 2019-11-07 0:21 ` Lakshmi Ramasubramanian 2019-11-07 3:40 ` Mimi Zohar 2019-11-07 3:40 ` Mimi Zohar 2019-11-07 18:42 ` Lakshmi Ramasubramanian 2019-11-07 18:42 ` Lakshmi Ramasubramanian 2019-11-07 20:53 ` Mimi Zohar 2019-11-07 20:53 ` Mimi Zohar 2019-11-07 21:12 ` Lakshmi Ramasubramanian 2019-11-07 21:12 ` Lakshmi Ramasubramanian 2019-11-06 19:01 ` [PATCH v4 02/10] IMA: Added KEYRING_CHECK func in IMA policy to measure keys Lakshmi Ramasubramanian 2019-11-06 19:01 ` Lakshmi Ramasubramanian 2019-11-06 19:01 ` [PATCH v4 03/10] IMA: Added keyrings= option in IMA policy to only measure keys added to the specifi Lakshmi Ramasubramanian 2019-11-06 19:01 ` [PATCH v4 03/10] IMA: Added keyrings= option in IMA policy to only measure keys added to the specified keyrings Lakshmi Ramasubramanian 2019-11-06 19:01 ` [PATCH v4 04/10] IMA: Read keyrings= option from the IMA policy into ima_rule_entry Lakshmi Ramasubramanian 2019-11-06 19:01 ` Lakshmi Ramasubramanian 2019-11-06 19:01 ` [PATCH v4 05/10] IMA: Updated IMA policy functions to return keyrings option read from the policy Lakshmi Ramasubramanian 2019-11-06 19:01 ` Lakshmi Ramasubramanian 2019-11-06 19:01 ` [PATCH v4 06/10] IMA: Measure key if the IMA policy allows measurement for the keyring to which the Lakshmi Ramasubramanian 2019-11-06 19:01 ` [PATCH v4 06/10] IMA: Measure key if the IMA policy allows measurement for the keyring to which the key is linked to Lakshmi Ramasubramanian 2019-11-06 19:01 ` [PATCH v4 07/10] IMA: Added a boolean flag to track IMA initialization status Lakshmi Ramasubramanian 2019-11-06 19:01 ` Lakshmi Ramasubramanian 2019-11-06 19:01 ` [PATCH v4 08/10] IMA: Defined functions to queue and dequeue keys for measurement Lakshmi Ramasubramanian 2019-11-06 19:01 ` Lakshmi Ramasubramanian 2019-11-06 22:44 ` Mimi Zohar 2019-11-06 22:44 ` Mimi Zohar 2019-11-06 23:52 ` Lakshmi Ramasubramanian [this message] 2019-11-06 23:52 ` Lakshmi Ramasubramanian 2019-11-07 2:20 ` Lakshmi Ramasubramanian 2019-11-07 2:20 ` Lakshmi Ramasubramanian 2019-11-06 19:01 ` [PATCH v4 09/10] IMA: Call queue and dequeue functions to measure keys Lakshmi Ramasubramanian 2019-11-06 19:01 ` Lakshmi Ramasubramanian 2019-11-06 19:01 ` [PATCH v4 10/10] KEYS: Call the IMA hook to measure key when a new key is created or an existing key Lakshmi Ramasubramanian 2019-11-06 19:01 ` [PATCH v4 10/10] KEYS: Call the IMA hook to measure key when a new key is created or an existing key is updated Lakshmi Ramasubramanian
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=8b2fd578-7429-f5b8-4286-1face91e1ae6@linux.microsoft.com \ --to=nramas@linux.microsoft.com \ --cc=dhowells@redhat.com \ --cc=jamorris@linux.microsoft.com \ --cc=keyrings@vger.kernel.org \ --cc=linux-integrity@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-security-module@vger.kernel.org \ --cc=matthewgarrett@google.com \ --cc=sashal@kernel.org \ --cc=zohar@linux.ibm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.