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 for Android: free password hash cracker in your pocket

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

Message-ID: <b89a084a98e7427911ac4344225eca99a04a52fb.camel@linux.ibm.com>
Date: Tue, 19 Nov 2024 13:10:10 -0500
From: Mimi Zohar <zohar@...ux.ibm.com>
To: Breno Leitao <leitao@...ian.org>,
        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>
Cc: 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

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@...ian.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,