linux-kernel - Re: [PATCH v4 2/2] IMA: Call workqueue functions to measure queued keys

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <edda07dc-c615-58f1-5689-4692c3fb3315@linux.microsoft.com>
Date:   Tue, 17 Dec 2019 18:44:31 -0800
From:   Lakshmi Ramasubramanian <nramas@...ux.microsoft.com>
To:     James Bottomley <James.Bottomley@...senPartnership.com>,
        zohar@...ux.ibm.com, linux-integrity@...r.kernel.org
Cc:     eric.snowberg@...cle.com, dhowells@...hat.com,
        mathew.j.martineau@...ux.intel.com, matthewgarrett@...gle.com,
        sashal@...nel.org, jamorris@...ux.microsoft.com,
        linux-kernel@...r.kernel.org, keyrings@...r.kernel.org
Subject: Re: [PATCH v4 2/2] IMA: Call workqueue functions to measure queued
 keys

On 12/17/2019 6:01 PM, James Bottomley wrote:

> 
> This code is confusing me:
> 
> +       /*
> +        * 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);
> +
> +       if (!ima_process_keys) {
> +               ima_process_keys = true;
> +
> +               if (!list_empty(&ima_keys)) {
> +                       list_for_each_entry_safe(entry, tmp, &ima_keys, list)
> +                               list_move_tail(&entry->list, &temp_ima_keys);
> +                       process = true;
> +               }
> +       }
> +
> +       mutex_unlock(&ima_keys_mutex);
> +
> 
> The direct implication of the comment and the lock dance with the
> temporary list and the processed flag is that stuff can be added to the
> ima_keys list after you drop the mutex.  Your explanation in the prior
> couple of emails says that nothing can be added because the
> ima_process_keys flag setting prevents it.  If the latter is true, you
> can simply drop the lock after setting the flag and rely on ima_keys
> not changing to run it through process_buffer_measurement without
> needing any of the intermediate list or the processed flag.  If the
> latter isn't true then any key added to ima_keys after the mutex is
> dropped is never processed.
> 
> James

Once the flag is set no new key will be added to ima_keys list.

You are right - if the flag is set with the lock taken, then there is no 
need for the temp list. After dropping the lock, measurement can be done 
directly from ima_keys list.

Thanks for reviewing the code. I'll send an update tomorrow.

  -lakshmi