Linux-Integrity Archive on lore.kernel.org
 help / color / Atom feed
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
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

  reply index

Thread overview: 20+ 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 ` [PATCH v4 01/10] IMA: Defined an IMA hook to measure keys on key create or update Lakshmi Ramasubramanian
2019-11-06 22:43   ` Mimi Zohar
2019-11-07  0:21     ` Lakshmi Ramasubramanian
2019-11-07  3:40       ` Mimi Zohar
2019-11-07 18:42         ` Lakshmi Ramasubramanian
2019-11-07 20:53           ` Mimi Zohar
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 ` [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 ` [PATCH v4 05/10] IMA: Updated IMA policy functions to return keyrings option read from the policy 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 ` [PATCH v4 08/10] IMA: Defined functions to queue and dequeue keys for measurement Lakshmi Ramasubramanian
2019-11-06 22:44   ` Mimi Zohar
2019-11-06 23:52     ` Lakshmi Ramasubramanian [this message]
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 ` [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 publically 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: link

Linux-Integrity Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-integrity/0 linux-integrity/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-integrity linux-integrity/ https://lore.kernel.org/linux-integrity \
		linux-integrity@vger.kernel.org
	public-inbox-index linux-integrity

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-integrity


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git