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

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

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,