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]

Date:   Wed, 18 Dec 2019 11:01:39 +0900
From:   James Bottomley <James.Bottomley@...senPartnership.com>
To:     Lakshmi Ramasubramanian <nramas@...ux.microsoft.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 Tue, 2019-12-17 at 14:22 -0800, Lakshmi Ramasubramanian wrote:
> Hi James,
> 
> > > > 
> > > > This is the problem: in the race case you may still be adding
> > > > keys to
> > > > the queue after the other thread has processed it. Those keys
> > > > won't get
> > > > processed because the flag is now false in the post check so
> > > > the
> > > > current thread won't process them either.
> > > > 
> > > > James
> > > > 
> 
> Please let me know if you still think there is a race condition.
> 
> If yes, please explain how a key would be added to the queue after 
> ima_process_queued_keys() has processed queued keys.
> ima_process_keys flag will be true when queued keys have been
> processed.

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);
+
+       if (!process)
+               return;
+
+       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);
+       }
+}
+

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