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 <[email protected]>
> 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,
