[PATCH] ima: hold ima_extend_list_mutex while locating the trim position

[Weibin Liu <liuwb@xiaopeng.com>]

ima_queue_delete_partial() looks up the queue entry at which the
measurement list has to be cut without holding ima_extend_list_mutex,
and only takes the mutex afterwards for __list_cut_position().  The
comment above the cut justifies this with "ima_fs.c enforces
single-writer exclusion".

That exclusion does not cover the write() path.  ima_measure_lock()
registers measure_writer at open() time and even lets the same task
open the file again by incrementing measure_writer_extra_writes, while
_ima_measurements_write() never checks the caller's identity.  A
privileged task can therefore open several descriptors for the
measurement list and use them concurrently from its own threads.  Each
open() creates a separate struct file, so the f_pos_lock taken by
fdget_pos() does not serialize these writers (sharing a single
descriptor would be serialized there).

Two racing writers can then resolve the same, or an overlapping, trim
position, and the second one dereferences a queue entry that the first
one has already detached and handed to ima_queue_delete().  Depending
on the interleaving this corrupts the measurement list, reads freed
memory in __list_cut_position(), or frees the same entry twice.

Reproducer: generate some tens of thousands of measurement records,
open() the measurement list for writing several times from one task,
then write "20000\n" from that many threads using one descriptor each.
KASAN reports a stack-out-of-bounds access in ima_queue_delete(),
called from ima_queue_delete_partial().

Move the lookup into the same critical section as the cut, so that the
entry stays valid until it has been detached.  Holding
ima_extend_list_mutex also excludes ima_extend_list(), so the plain
list_for_each_entry() is sufficient and the RCU variant is no longer
needed.  Keep the existing error precedence (a short list returns
-ENOENT before the suspended check) and keep ima_queue_delete()
outside the mutex, since the trimmed list is already private.

Fixes: fcb0318a2969 ("ima: Support staging and deleting N measurements records")
Cc: stable@vger.kernel.org # v7.2+
Signed-off-by: Weibin Liu <liuwb@xiaopeng.com>
Reviewed-by: Zhu Guanglei <zhugl3@xiaopeng.com>
Assisted-by: LLM
---
 security/integrity/ima/ima_queue.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index 0f1b7e4113c4..cf8c164eea13 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -427,12 +427,16 @@ int ima_queue_delete_partial(unsigned long req_value)
 	struct ima_queue_entry *qe;
 	int ret = 0;
 
+	mutex_lock(&ima_extend_list_mutex);
+
 	/*
-	 * list_for_each_entry_rcu() without rcu_read_lock() is fine because
-	 * only list append can happen concurrently. No list replace due to the
-	 * staging/delete writers mutual exclusion.
+	 * The lookup and the cut have to be atomic with respect to other
+	 * writers: ima_fs.c serializes open(), but write() can run
+	 * concurrently on independently opened descriptors. Holding
+	 * ima_extend_list_mutex also excludes ima_extend_list(), so the
+	 * plain list traversal is sufficient.
 	 */
-	list_for_each_entry_rcu(qe, &ima_measurements, later, true) {
+	list_for_each_entry(qe, &ima_measurements, later) {
 		size_to_remove += get_binary_runtime_size(qe->entry);
 		num_to_remove++;
 
@@ -441,18 +445,16 @@ int ima_queue_delete_partial(unsigned long req_value)
 	}
 
 	/* Not enough records to delete. */
-	if (req_value_copy > 0)
+	if (req_value_copy > 0) {
+		mutex_unlock(&ima_extend_list_mutex);
 		return -ENOENT;
+	}
 
-	mutex_lock(&ima_extend_list_mutex);
 	if (ima_measurements_suspended) {
 		mutex_unlock(&ima_extend_list_mutex);
 		return -ESTALE;
 	}
 
-	/*
-	 * qe remains valid because ima_fs.c enforces single-writer exclusion.
-	 */
 	__list_cut_position(&ima_measurements_trim, &ima_measurements,
 			    &qe->later);
 
-- 
2.50.1