linux-kernel - Re: [PATCH] ima: kexec: Add RCU read lock protection for ima

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <5a315c98bcb8b900c8f4bac06fdfccec1308fcce.camel@linux.ibm.com>
Date: Wed, 20 Nov 2024 15:38:45 -0500
From: Mimi Zohar <zohar@...ux.ibm.com>
To: Breno Leitao <leitao@...ian.org>
Cc: Roberto Sassu <roberto.sassu@...wei.com>,
        Dmitry Kasatkin
 <dmitry.kasatkin@...il.com>,
        Eric Snowberg <eric.snowberg@...cle.com>,
        Paul
 Moore <paul@...l-moore.com>, James Morris <jmorris@...ei.org>,
        "Serge E.
 Hallyn" <serge@...lyn.com>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Thiago Jung Bauermann
 <bauerman@...ux.vnet.ibm.com>,
        Mimi Zohar <zohar@...ux.vnet.ibm.com>, linux-integrity@...r.kernel.org,
        linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org,
        noodles@...th.li
Subject: Re: [PATCH] ima: kexec: Add RCU read lock protection for
 ima_measurements list traversal

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@...ian.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@...ian.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);
> 
>