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

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

[Weibin Liu]

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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] ima: comment and commit message in ima_queue_delete_partial() cite non-existent ima_extend_list()
--

commit 1fb4a03b7cbb60d3d99dc9d31af10d612ce10f6a
Author: Weibin Liu <liuwb@xiaopeng.com>

ima: hold ima_extend_list_mutex while locating the trim position

This commit moves the measurement list lookup in ima_queue_delete_partial()
under ima_extend_list_mutex so that locating the trim position and cutting the
list are serialized against concurrent writers on separate file descriptors.

> diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
> index 0f1b7e4113c4f..cf8c164eea139 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.
>  	 */

[Severity: Low]
This isn't a bug, but both the comment in ima_queue_delete_partial() and the
commit message refer to ima_extend_list(), which does not exist in the tree.

Entries are appended to ima_measurements in ima_add_digest_entry() (called
from ima_add_template_entry() and ima_restore_measurement_entry()) while
holding ima_extend_list_mutex. Should the comment and commit message refer to
ima_add_digest_entry() instead of ima_extend_list()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104053705.3967228-1-liuwb@xiaopeng.com?part=1

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

[Roberto Sassu]

On Wed, 2026-11-04 at 13:37 +0800, Weibin Liu wrote:
> 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).

Yes, I assumed the good behavior of the privileged task, but you are
right that it must be fixed.

> 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.

Not enough, it does not protect against the read concurrent with
replacing/cutting the list (originated from the same writer).

A reasonable way to solve the problem would be to introduce an rwsem,
and wrap the write, read and lseek operations. This restores the
assumptions the code was written with.

We still need the current exclusion to avoid that a process deletes a
staged list created by another process.

Thanks

Roberto

> 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);
>