ima: kexec: Add RCU read lock protection for ima_measurements list traversal

[PATCH] ima: kexec: Add RCU read lock protection for ima_measurements list traversal

Posted by Breno Leitao 2 weeks, 6 days ago

Fix a potential RCU issue where ima_measurements list is traversed using
list_for_each_entry_rcu() without proper RCU read lock protection. This
caused warnings when CONFIG_PROVE_RCU was enabled:

  security/integrity/ima/ima_kexec.c:40 RCU-list traversed in non-reader section!!

Add rcu_read_lock() before iterating over ima_measurements list to ensure
proper RCU synchronization, consistent with other RCU list traversals in
the codebase.

Signed-off-by: Breno Leitao <leitao@debian.org>
Fixes: 7b8589cc29e7 ("ima: on soft reboot, save the measurement list")
---
 security/integrity/ima/ima_kexec.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index 52e00332defed39774c9e23e045f1377cfa30d0c..3b17ddb91d35ac806aedd2ee970ff365675dac0b 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -37,6 +37,7 @@ static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
 
 	memset(&khdr, 0, sizeof(khdr));
 	khdr.version = 1;
+	rcu_read_lock();
 	list_for_each_entry_rcu(qe, &ima_measurements, later) {
 		if (file.count < file.size) {
 			khdr.count++;
@@ -46,6 +47,7 @@ static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
 			break;
 		}
 	}
+	rcu_read_unlock();
 
 	if (ret < 0)
 		goto out;

---
base-commit: f488649e40f8900d23b86afeab7d4b78c063d5d1
change-id: 20241104-ima_rcu-ee83da87d050

Best regards,
-- 
Breno Leitao <leitao@debian.org>

Re: [PATCH] ima: kexec: Add RCU read lock protection for ima_measurements list traversal

Posted by Mimi Zohar 4 days, 19 hours ago

Hi Breno,

On Mon, 2024-11-04 at 02:47 -0800, Breno Leitao wrote:
> Fix a potential RCU issue where ima_measurements list is traversed using
> list_for_each_entry_rcu() without proper RCU read lock protection. This
> caused warnings when CONFIG_PROVE_RCU was enabled:
> 
>   security/integrity/ima/ima_kexec.c:40 RCU-list traversed in non-reader section!!
> 
> Add rcu_read_lock() before iterating over ima_measurements list to ensure
> proper RCU synchronization, consistent with other RCU list traversals in
> the codebase.

The synchronization is to prevent freeing of data while walking the RCU list. In
this case, new measurements are only appended to the IMA measurement list.  So
there shouldn't be an issue.

The IMA measurement list is being copied during kexec "load", while other
processes are still running.  Depending on the IMA policy, the kexec "load",
itself, and these other processes may result in additional measurements, which
should be copied across kexec.  Adding the rcu_read_{lock, unlock} would
unnecessarily prevent them from being copied.

There have been discussions about deferring copying the IMA measurement list
from kexec "load" to later at "exec" and about trimming the IMA measurement
list.  At least for now, neither of these changes have been upstreamed.

Perhaps add a comment as a reminder as to why it is currently safe.

Mimi

> 
> Signed-off-by: Breno Leitao <leitao@debian.org>
> Fixes: 7b8589cc29e7 ("ima: on soft reboot, save the measurement list")
> ---
>  security/integrity/ima/ima_kexec.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> index 52e00332defed39774c9e23e045f1377cfa30d0c..3b17ddb91d35ac806aedd2ee970ff365675dac0b 100644
> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -37,6 +37,7 @@ static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
>  
>  	memset(&khdr, 0, sizeof(khdr));
>  	khdr.version = 1;
> +	rcu_read_lock();
>  	list_for_each_entry_rcu(qe, &ima_measurements, later) {
>  		if (file.count < file.size) {
>  			khdr.count++;
> @@ -46,6 +47,7 @@ static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
>  			break;
>  		}
>  	}
> +	rcu_read_unlock();
>  
>  	if (ret < 0)
>  		goto out;
> 
> ---
> base-commit: f488649e40f8900d23b86afeab7d4b78c063d5d1
> change-id: 20241104-ima_rcu-ee83da87d050
> 
> Best regards,

Re: [PATCH] ima: kexec: Add RCU read lock protection for ima_measurements list traversal

Posted by Breno Leitao 3 days, 18 hours ago

Hello Mimi,

On Tue, Nov 19, 2024 at 01:10:10PM -0500, Mimi Zohar wrote:
> Hi Breno,
> 
> On Mon, 2024-11-04 at 02:47 -0800, Breno Leitao wrote:
> > Fix a potential RCU issue where ima_measurements list is traversed using
> > list_for_each_entry_rcu() without proper RCU read lock protection. This
> > caused warnings when CONFIG_PROVE_RCU was enabled:
> > 
> >   security/integrity/ima/ima_kexec.c:40 RCU-list traversed in non-reader section!!
> > 
> > Add rcu_read_lock() before iterating over ima_measurements list to ensure
> > proper RCU synchronization, consistent with other RCU list traversals in
> > the codebase.
> 
> The synchronization is to prevent freeing of data while walking the RCU list. In
> this case, new measurements are only appended to the IMA measurement list.  So
> there shouldn't be an issue.
> 
> The IMA measurement list is being copied during kexec "load", while other
> processes are still running.  Depending on the IMA policy, the kexec "load",
> itself, and these other processes may result in additional measurements, which
> should be copied across kexec.  Adding the rcu_read_{lock, unlock} would
> unnecessarily prevent them from being copied.

Thank you for the detailed explanation. Since rcu_read_lock() operations are
lightweight, I believe keeping them wouldn't impact performance significantly.

However, if you prefer the lockless approach, I would suggest adding an
argument to list_for_each_entry_rcu() to keep the warning out. What are
your thoughts on this?

Author: Breno Leitao <leitao@debian.org>
Date:   Mon Nov 4 02:26:45 2024 -0800

    ima: kexec: silence RCU list traversal warning

    The ima_measurements list is append-only and doesn't require rcu_read_lock()
    protection. However, lockdep issues a warning when traversing RCU lists
    without the read lock:

      security/integrity/ima/ima_kexec.c:40 RCU-list traversed in non-reader section!!

    Fix this by using the lockless variant of list_for_each_entry_rcu() with
    the last argument set to true. This tells the RCU subsystem that
    traversing this append-only list without the read lock is intentional
    and safe.

    This change silences the lockdep warning while maintaining the correct
    semantics for the append-only list traversal.

    Signed-off-by: Breno Leitao <leitao@debian.org>

diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index 52e00332defed..9d45f4d26f731 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -37,7 +37,8 @@ static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,

 	memset(&khdr, 0, sizeof(khdr));
 	khdr.version = 1;
-	list_for_each_entry_rcu(qe, &ima_measurements, later) {
+	/* This is an append-only list, no need to hold the RCU read lock */
+	list_for_each_entry_rcu(qe, &ima_measurements, later, true) {
 		if (file.count < file.size) {
 			khdr.count++;
 			ima_measurements_show(&file, qe);

Re: [PATCH] ima: kexec: Add RCU read lock protection for ima_measurements list traversal

Posted by Mimi Zohar 3 days, 17 hours ago

On Wed, 2024-11-20 at 19:44 +0000, Breno Leitao wrote:
> Hello Mimi,
> 
> On Tue, Nov 19, 2024 at 01:10:10PM -0500, Mimi Zohar wrote:
> > Hi Breno,
> > 
> > On Mon, 2024-11-04 at 02:47 -0800, Breno Leitao wrote:
> > > Fix a potential RCU issue where ima_measurements list is traversed using
> > > list_for_each_entry_rcu() without proper RCU read lock protection. This
> > > caused warnings when CONFIG_PROVE_RCU was enabled:
> > > 
> > >   security/integrity/ima/ima_kexec.c:40 RCU-list traversed in non-reader section!!
> > > 
> > > Add rcu_read_lock() before iterating over ima_measurements list to ensure
> > > proper RCU synchronization, consistent with other RCU list traversals in
> > > the codebase.
> > 
> > The synchronization is to prevent freeing of data while walking the RCU list. In
> > this case, new measurements are only appended to the IMA measurement list.  So
> > there shouldn't be an issue.
> > 
> > The IMA measurement list is being copied during kexec "load", while other
> > processes are still running.  Depending on the IMA policy, the kexec "load",
> > itself, and these other processes may result in additional measurements, which
> > should be copied across kexec.  Adding the rcu_read_{lock, unlock} would
> > unnecessarily prevent them from being copied.
> 
> Thank you for the detailed explanation. Since rcu_read_lock() operations are
> lightweight, I believe keeping them wouldn't impact performance significantly.

It's not a question of performance, but of missing measurements in the IMA
measurement list.

> 
> However, if you prefer the lockless approach, I would suggest adding an
> argument to list_for_each_entry_rcu() to keep the warning out. What are
> your thoughts on this?

Yes, this is better.

thanks,

Mimi
> 
> Author: Breno Leitao <leitao@debian.org>
> Date:   Mon Nov 4 02:26:45 2024 -0800
> 
>     ima: kexec: silence RCU list traversal warning
> 
>     The ima_measurements list is append-only and doesn't require rcu_read_lock()
>     protection. However, lockdep issues a warning when traversing RCU lists
>     without the read lock:
> 
>       security/integrity/ima/ima_kexec.c:40 RCU-list traversed in non-reader section!!
> 
>     Fix this by using the lockless variant of list_for_each_entry_rcu() with
>     the last argument set to true. This tells the RCU subsystem that
>     traversing this append-only list without the read lock is intentional
>     and safe.
> 
>     This change silences the lockdep warning while maintaining the correct
>     semantics for the append-only list traversal.
> 
>     Signed-off-by: Breno Leitao <leitao@debian.org>
> 
> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> index 52e00332defed..9d45f4d26f731 100644
> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -37,7 +37,8 @@ static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
> 
>  	memset(&khdr, 0, sizeof(khdr));
>  	khdr.version = 1;
> -	list_for_each_entry_rcu(qe, &ima_measurements, later) {
> +	/* This is an append-only list, no need to hold the RCU read lock */
> +	list_for_each_entry_rcu(qe, &ima_measurements, later, true) {
>  		if (file.count < file.size) {
>  			khdr.count++;
>  			ima_measurements_show(&file, qe);
> 
>