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 Reviewed-by: Zhu Guanglei 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