[PATCH v2] ima: fix races when deleting measurement list entries

[PATCH v2] ima: fix races when deleting measurement list entries

[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 only covers cross-process opens.  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() serializes neither these writers nor their readers.

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.

The extend mutex alone cannot protect readers: the binary and ASCII
file operations call seq_read()/seq_lseek(), which traverse the list
without taking ima_extend_list_mutex.  An independent O_RDWR descriptor
lets a reader race with deletion from the same task.  Adding a
concurrent reader to the writer reproducer triggers a general
protection fault in ima_measurements_show(), reached from seq_read().

Introduce ima_measurements_rwsem at the file-operation layer.  Writers
take it for write around _ima_measurements_write(); readers take it for
read around seq_read() and seq_lseek().  Cover the binary/ASCII and
current/staged file operations, since they all expose the same list
traversal and deletion commands.  The resulting lock order is rwsem
followed by seq_file's lock on the read side, or ima_extend_list_mutex
on the write side.  Freeing the private trimmed list remains outside
the extend mutex but inside the rwsem write-side critical section.
The existing open-time exclusion remains unchanged, so one task cannot
delete a staged list belonging to another.

Suggested-by: Roberto Sassu <roberto.sassu@huawei.com>
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>
---
Changes in v2:
- Add rwsem protection for read and lseek, as suggested by Roberto.
- Cover all current/staged binary/ASCII file operations.
- Keep the open-time exclusion unchanged.
- Clarify the lockless kexec dump protection in ima_measurements_show().

Link: https://lore.kernel.org/all/20261104053705.3967228-1-liuwb@xiaopeng.com/ [v1]

 security/integrity/ima/ima_fs.c    | 66 ++++++++++++++++++++++++------
 security/integrity/ima/ima_queue.c | 20 +++++----
 2 files changed, 65 insertions(+), 21 deletions(-)

diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 3b8174a91a34..9da881308d52 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -39,6 +39,8 @@ static DEFINE_MUTEX(ima_measure_mutex);
 static long ima_measure_users;
 static struct task_struct *measure_writer;
 static long measure_writer_extra_writes;
+/* Lock order: ima_measurements_rwsem -> ima_extend_list_mutex. */
+static DECLARE_RWSEM(ima_measurements_rwsem);
 
 bool ima_canonical_fmt;
 static int __init default_canonical_fmt_setup(char *str)
@@ -161,7 +163,11 @@ void ima_putc(struct seq_file *m, void *data, int datalen)
  */
 int ima_measurements_show(struct seq_file *m, void *v)
 {
-	/* the list never shrinks, so we don't need a lock here */
+	/*
+	 * seq_file callers hold ima_measurements_rwsem.  The kexec dump caller
+	 * relies on reboot notifier ordering: measurements are suspended and
+	 * staging/deletion are forbidden while it walks the lists locklessly.
+	 */
 	struct ima_queue_entry *qe = v;
 	struct ima_template_entry *e;
 	char *template_name;
@@ -400,32 +406,68 @@ static ssize_t _ima_measurements_write(struct file *file,
 	return datalen;
 }
 
+static ssize_t ima_measurements_read(struct file *file, char __user *buf,
+				     size_t datalen, loff_t *ppos)
+{
+	ssize_t ret;
+
+	down_read(&ima_measurements_rwsem);
+	ret = seq_read(file, buf, datalen, ppos);
+	up_read(&ima_measurements_rwsem);
+
+	return ret;
+}
+
+static loff_t ima_measurements_llseek(struct file *file, loff_t offset,
+				      int whence)
+{
+	loff_t ret;
+
+	down_read(&ima_measurements_rwsem);
+	ret = seq_lseek(file, offset, whence);
+	up_read(&ima_measurements_rwsem);
+
+	return ret;
+}
+
 static ssize_t ima_measurements_write(struct file *file, const char __user *buf,
 				      size_t datalen, loff_t *ppos)
 {
-	return _ima_measurements_write(file, buf, datalen, ppos, false);
+	ssize_t ret;
+
+	down_write(&ima_measurements_rwsem);
+	ret = _ima_measurements_write(file, buf, datalen, ppos, false);
+	up_write(&ima_measurements_rwsem);
+
+	return ret;
 }
 
 static ssize_t ima_measurements_staged_write(struct file *file,
 					     const char __user *buf,
 					     size_t datalen, loff_t *ppos)
 {
-	return _ima_measurements_write(file, buf, datalen, ppos, true);
+	ssize_t ret;
+
+	down_write(&ima_measurements_rwsem);
+	ret = _ima_measurements_write(file, buf, datalen, ppos, true);
+	up_write(&ima_measurements_rwsem);
+
+	return ret;
 }
 
 static const struct file_operations ima_measurements_ops = {
 	.open = ima_measurements_open,
-	.read = seq_read,
+	.read = ima_measurements_read,
 	.write = ima_measurements_write,
-	.llseek = seq_lseek,
+	.llseek = ima_measurements_llseek,
 	.release = ima_measurements_release,
 };
 
 static const struct file_operations ima_measurements_staged_ops = {
 	.open = ima_measurements_staged_open,
-	.read = seq_read,
+	.read = ima_measurements_read,
 	.write = ima_measurements_staged_write,
-	.llseek = seq_lseek,
+	.llseek = ima_measurements_llseek,
 	.release = ima_measurements_release,
 };
 
@@ -440,7 +482,7 @@ void ima_print_digest(struct seq_file *m, u8 *digest, u32 size)
 /* print in ascii */
 static int ima_ascii_measurements_show(struct seq_file *m, void *v)
 {
-	/* the list never shrinks, so we don't need a lock here */
+	/* The caller holds ima_measurements_rwsem. */
 	struct ima_queue_entry *qe = v;
 	struct ima_template_entry *e;
 	char *template_name;
@@ -497,9 +539,9 @@ static int ima_ascii_measurements_open(struct inode *inode, struct file *file)
 
 static const struct file_operations ima_ascii_measurements_ops = {
 	.open = ima_ascii_measurements_open,
-	.read = seq_read,
+	.read = ima_measurements_read,
 	.write = ima_measurements_write,
-	.llseek = seq_lseek,
+	.llseek = ima_measurements_llseek,
 	.release = ima_measurements_release,
 };
 
@@ -519,9 +561,9 @@ static int ima_ascii_measurements_staged_open(struct inode *inode,
 
 static const struct file_operations ima_ascii_measurements_staged_ops = {
 	.open = ima_ascii_measurements_staged_open,
-	.read = seq_read,
+	.read = ima_measurements_read,
 	.write = ima_measurements_staged_write,
-	.llseek = seq_lseek,
+	.llseek = ima_measurements_llseek,
 	.release = ima_measurements_release,
 };
 
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index 67782c004cdf..8c69684abd25 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 v2] ima: fix races when deleting measurement list entries

[Roberto Sassu]

On Mon, 2026-11-09 at 20:33 +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 only covers cross-process opens.  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() serializes neither these writers nor their readers.
> 
> 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.

Is it still true with the new semaphore?

Thanks

Roberto

> 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.
> 
> The extend mutex alone cannot protect readers: the binary and ASCII
> file operations call seq_read()/seq_lseek(), which traverse the list
> without taking ima_extend_list_mutex.  An independent O_RDWR descriptor
> lets a reader race with deletion from the same task.  Adding a
> concurrent reader to the writer reproducer triggers a general
> protection fault in ima_measurements_show(), reached from seq_read().
> 
> Introduce ima_measurements_rwsem at the file-operation layer.  Writers
> take it for write around _ima_measurements_write(); readers take it for
> read around seq_read() and seq_lseek().  Cover the binary/ASCII and
> current/staged file operations, since they all expose the same list
> traversal and deletion commands.  The resulting lock order is rwsem
> followed by seq_file's lock on the read side, or ima_extend_list_mutex
> on the write side.  Freeing the private trimmed list remains outside
> the extend mutex but inside the rwsem write-side critical section.
> The existing open-time exclusion remains unchanged, so one task cannot
> delete a staged list belonging to another.
> 
> Suggested-by: Roberto Sassu <roberto.sassu@huawei.com>
> 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>
> ---
> Changes in v2:
> - Add rwsem protection for read and lseek, as suggested by Roberto.
> - Cover all current/staged binary/ASCII file operations.
> - Keep the open-time exclusion unchanged.
> - Clarify the lockless kexec dump protection in ima_measurements_show().
> 
> Link: https://lore.kernel.org/all/20261104053705.3967228-1-liuwb@xiaopeng.com/ [v1]
> 
>  security/integrity/ima/ima_fs.c    | 66 ++++++++++++++++++++++++------
>  security/integrity/ima/ima_queue.c | 20 +++++----
>  2 files changed, 65 insertions(+), 21 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index 3b8174a91a34..9da881308d52 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -39,6 +39,8 @@ static DEFINE_MUTEX(ima_measure_mutex);
>  static long ima_measure_users;
>  static struct task_struct *measure_writer;
>  static long measure_writer_extra_writes;
> +/* Lock order: ima_measurements_rwsem -> ima_extend_list_mutex. */
> +static DECLARE_RWSEM(ima_measurements_rwsem);
>  
>  bool ima_canonical_fmt;
>  static int __init default_canonical_fmt_setup(char *str)
> @@ -161,7 +163,11 @@ void ima_putc(struct seq_file *m, void *data, int datalen)
>   */
>  int ima_measurements_show(struct seq_file *m, void *v)
>  {
> -	/* the list never shrinks, so we don't need a lock here */
> +	/*
> +	 * seq_file callers hold ima_measurements_rwsem.  The kexec dump caller
> +	 * relies on reboot notifier ordering: measurements are suspended and
> +	 * staging/deletion are forbidden while it walks the lists locklessly.
> +	 */
>  	struct ima_queue_entry *qe = v;
>  	struct ima_template_entry *e;
>  	char *template_name;
> @@ -400,32 +406,68 @@ static ssize_t _ima_measurements_write(struct file *file,
>  	return datalen;
>  }
>  
> +static ssize_t ima_measurements_read(struct file *file, char __user *buf,
> +				     size_t datalen, loff_t *ppos)
> +{
> +	ssize_t ret;
> +
> +	down_read(&ima_measurements_rwsem);
> +	ret = seq_read(file, buf, datalen, ppos);
> +	up_read(&ima_measurements_rwsem);
> +
> +	return ret;
> +}
> +
> +static loff_t ima_measurements_llseek(struct file *file, loff_t offset,
> +				      int whence)
> +{
> +	loff_t ret;
> +
> +	down_read(&ima_measurements_rwsem);
> +	ret = seq_lseek(file, offset, whence);
> +	up_read(&ima_measurements_rwsem);
> +
> +	return ret;
> +}
> +
>  static ssize_t ima_measurements_write(struct file *file, const char __user *buf,
>  				      size_t datalen, loff_t *ppos)
>  {
> -	return _ima_measurements_write(file, buf, datalen, ppos, false);
> +	ssize_t ret;
> +
> +	down_write(&ima_measurements_rwsem);
> +	ret = _ima_measurements_write(file, buf, datalen, ppos, false);
> +	up_write(&ima_measurements_rwsem);
> +
> +	return ret;
>  }
>  
>  static ssize_t ima_measurements_staged_write(struct file *file,
>  					     const char __user *buf,
>  					     size_t datalen, loff_t *ppos)
>  {
> -	return _ima_measurements_write(file, buf, datalen, ppos, true);
> +	ssize_t ret;
> +
> +	down_write(&ima_measurements_rwsem);
> +	ret = _ima_measurements_write(file, buf, datalen, ppos, true);
> +	up_write(&ima_measurements_rwsem);
> +
> +	return ret;
>  }
>  
>  static const struct file_operations ima_measurements_ops = {
>  	.open = ima_measurements_open,
> -	.read = seq_read,
> +	.read = ima_measurements_read,
>  	.write = ima_measurements_write,
> -	.llseek = seq_lseek,
> +	.llseek = ima_measurements_llseek,
>  	.release = ima_measurements_release,
>  };
>  
>  static const struct file_operations ima_measurements_staged_ops = {
>  	.open = ima_measurements_staged_open,
> -	.read = seq_read,
> +	.read = ima_measurements_read,
>  	.write = ima_measurements_staged_write,
> -	.llseek = seq_lseek,
> +	.llseek = ima_measurements_llseek,
>  	.release = ima_measurements_release,
>  };
>  
> @@ -440,7 +482,7 @@ void ima_print_digest(struct seq_file *m, u8 *digest, u32 size)
>  /* print in ascii */
>  static int ima_ascii_measurements_show(struct seq_file *m, void *v)
>  {
> -	/* the list never shrinks, so we don't need a lock here */
> +	/* The caller holds ima_measurements_rwsem. */
>  	struct ima_queue_entry *qe = v;
>  	struct ima_template_entry *e;
>  	char *template_name;
> @@ -497,9 +539,9 @@ static int ima_ascii_measurements_open(struct inode *inode, struct file *file)
>  
>  static const struct file_operations ima_ascii_measurements_ops = {
>  	.open = ima_ascii_measurements_open,
> -	.read = seq_read,
> +	.read = ima_measurements_read,
>  	.write = ima_measurements_write,
> -	.llseek = seq_lseek,
> +	.llseek = ima_measurements_llseek,
>  	.release = ima_measurements_release,
>  };
>  
> @@ -519,9 +561,9 @@ static int ima_ascii_measurements_staged_open(struct inode *inode,
>  
>  static const struct file_operations ima_ascii_measurements_staged_ops = {
>  	.open = ima_ascii_measurements_staged_open,
> -	.read = seq_read,
> +	.read = ima_measurements_read,
>  	.write = ima_measurements_staged_write,
> -	.llseek = seq_lseek,
> +	.llseek = ima_measurements_llseek,
>  	.release = ima_measurements_release,
>  };
>  
> diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
> index 67782c004cdf..8c69684abd25 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);
>  

Re: [PATCH v2] ima: fix races when deleting measurement list entries

[Weibin Liu]

On Mon, 2026-11-09 at 13:49 +0100, Roberto Sassu wrote:
> Is it still true with the new semaphore?

No. That paragraph describes the unpatched code. The patch serializes
writers with ima_measurements_rwsem and protects the lookup and cut
with ima_extend_list_mutex.