| Message ID | 20241104-ima_rcu-v1-1-5157460c5907@debian.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ima: kexec: Add RCU read lock protection for ima_measurements list traversal | expand |
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,
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. 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? 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);
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); > >
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;
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. 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(+) --- base-commit: f488649e40f8900d23b86afeab7d4b78c063d5d1 change-id: 20241104-ima_rcu-ee83da87d050 Best regards,