Re: [PATCH v0 1/2] IMA: Defined queue functions

Lakshmi Ramasubramanian
2019-11-27T21:18:01Z

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