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

[Mimi Zohar <zohar@linux.ibm.com>]

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