| Message ID | 20210824085747.23604-1-liqiong@nfschina.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ima: fix deadlock within "ima_match_policy" function. | expand |
Hi liqiong, On 8/24/21 10:57 AM, liqiong wrote: > When "ima_match_policy" is looping while "ima_update_policy" changs Small typo: "changes"/"updates" > the variable "ima_rules", then "ima_match_policy" may can't exit > loop, Finally cause RCU CPU Stall Warnings: "rcu_sched detected > stall on CPU ...". This could perhaps be rephrased to something like: """ ima_match_policy() can loop on the policy ruleset while ima_update_policy() updates the variable "ima_rules". This can lead to a situation where ima_match_policy() can't exit the 'list_for_each_entry_rcu' loop, causing RCU stalls ("rcu_sched detected stall on CPU ..."). """ > > The problem is limited to transitioning from the builtin policy to > the custom policy. Eg. At boot time, systemd-services are being > checked within "ima_match_policy", at the same time, the variable > "ima_rules" is changed by another service. For the second sentence, consider something in the likes of: "This problem can happen in practice: updating the IMA policy in the boot process while systemd-services are being checked have been observed to trigger this issue.". Your commit message is also supposed to explain what you are doing, using the imperative form ((see 'Documentation/process/submitting-patches.rst'): """ Describe your changes in imperative mood, e.g. "make xyzzy do frotz" instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy to do frotz", as if you are giving orders to the codebase to change its behaviour. """ Maybe add a paragraph with something like "Fix locking by introducing ...."? > > Signed-off-by: liqiong <liqiong@nfschina.com> > --- > security/integrity/ima/ima_policy.c | 17 ++++++++++++----- > 1 file changed, 12 insertions(+), 5 deletions(-) > > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c > index fd5d46e511f1..e92b197bfd3c 100644 > --- a/security/integrity/ima/ima_policy.c > +++ b/security/integrity/ima/ima_policy.c > @@ -662,12 +662,14 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode, > { > struct ima_rule_entry *entry; > int action = 0, actmask = flags | (flags << 1); > + struct list_head *ima_rules_tmp; > > if (template_desc && !*template_desc) > *template_desc = ima_template_desc_current(); > > rcu_read_lock(); > - list_for_each_entry_rcu(entry, ima_rules, list) { > + ima_rules_tmp = rcu_dereference(ima_rules); > + list_for_each_entry_rcu(entry, ima_rules_tmp, list) { > > if (!(entry->action & actmask)) > continue; > @@ -919,8 +921,8 @@ void ima_update_policy(void) > > if (ima_rules != policy) { > ima_policy_flag = 0; > - ima_rules = policy; > > + rcu_assign_pointer(ima_rules, policy); > /* > * IMA architecture specific policy rules are specified > * as strings and converted to an array of ima_entry_rules > @@ -1649,9 +1651,11 @@ void *ima_policy_start(struct seq_file *m, loff_t *pos) > { > loff_t l = *pos; > struct ima_rule_entry *entry; > + struct list_head *ima_rules_tmp; > > rcu_read_lock(); > - list_for_each_entry_rcu(entry, ima_rules, list) { > + ima_rules_tmp = rcu_dereference(ima_rules); > + list_for_each_entry_rcu(entry, ima_rules_tmp, list) { > if (!l--) { > rcu_read_unlock(); > return entry; > @@ -1670,7 +1674,8 @@ void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos) > rcu_read_unlock(); > (*pos)++; > > - return (&entry->list == ima_rules) ? NULL : entry; > + return (&entry->list == &ima_default_rules || > + &entry->list == &ima_policy_rules) ? NULL : entry; > } > > void ima_policy_stop(struct seq_file *m, void *v) > @@ -1872,6 +1877,7 @@ bool ima_appraise_signature(enum kernel_read_file_id id) > struct ima_rule_entry *entry; > bool found = false; > enum ima_hooks func; > + struct list_head *ima_rules_tmp; > > if (id >= READING_MAX_ID) > return false; > @@ -1879,7 +1885,8 @@ bool ima_appraise_signature(enum kernel_read_file_id id) > func = read_idmap[id] ?: FILE_CHECK; > > rcu_read_lock(); > - list_for_each_entry_rcu(entry, ima_rules, list) { > + ima_rules_tmp = rcu_dereference(ima_rules); > + list_for_each_entry_rcu(entry, ima_rules_tmp, list) { > if (entry->action != APPRAISE) > continue; > > I haven't tested the patch myself, but the code diff looks fine to me. Thanks, Simon
Hi Simon :
ima: fix deadlock within RCU list of ima_rules.
ima_match_policy() is looping on the policy ruleset while
ima_update_policy() updates the variable "ima_rules". This can
lead to a situation where ima_match_policy() can't exit the
'list_for_each_entry_rcu' loop, causing RCU stalls
("rcu_sched detected stall on CPU ...").
This problem can happen in practice: updating the IMA policy
in the boot process while systemd-services are being checked.
In addition to ima_match_policy(), other function with
"list_for_each_entry_rcu" should happen too. Fix locking by
introducing a duplicate of "ima_rules" for each
"list_for_each_entry_rcu".
How about this commit message ?
I have tested this patch in lab, we can reproduced this error case,
have done reboot test many times. This patch should work.
在 2021年08月24日 17:50, THOBY Simon 写道:
> Hi liqiong,
>
> On 8/24/21 10:57 AM, liqiong wrote:
>> When "ima_match_policy" is looping while "ima_update_policy" changs
> Small typo: "changes"/"updates"
>
>> the variable "ima_rules", then "ima_match_policy" may can't exit
>> loop, Finally cause RCU CPU Stall Warnings: "rcu_sched detected
>> stall on CPU ...".
> This could perhaps be rephrased to something like:
> """
> ima_match_policy() can loop on the policy ruleset while
> ima_update_policy() updates the variable "ima_rules".
> This can lead to a situation where ima_match_policy()
> can't exit the 'list_for_each_entry_rcu' loop, causing
> RCU stalls ("rcu_sched detected stall on CPU ...").
> """
>
>
>> The problem is limited to transitioning from the builtin policy to
>> the custom policy. Eg. At boot time, systemd-services are being
>> checked within "ima_match_policy", at the same time, the variable
>> "ima_rules" is changed by another service.
> For the second sentence, consider something in the likes of:
> "This problem can happen in practice: updating the IMA policy
> in the boot process while systemd-services are being checked
> have been observed to trigger this issue.".
>
>
> Your commit message is also supposed to explain what you are doing,
> using the imperative form ((see 'Documentation/process/submitting-patches.rst'):
> """
> Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
> instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
> to do frotz", as if you are giving orders to the codebase to change
> its behaviour.
> """
>
> Maybe add a paragraph with something like "Fix locking by introducing ...."?
>
>
>> Signed-off-by: liqiong <liqiong@nfschina.com>
>> ---
>> security/integrity/ima/ima_policy.c | 17 ++++++++++++-----
>> 1 file changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
>> index fd5d46e511f1..e92b197bfd3c 100644
>> --- a/security/integrity/ima/ima_policy.c
>> +++ b/security/integrity/ima/ima_policy.c
>> @@ -662,12 +662,14 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode,
>> {
>> struct ima_rule_entry *entry;
>> int action = 0, actmask = flags | (flags << 1);
>> + struct list_head *ima_rules_tmp;
>>
>> if (template_desc && !*template_desc)
>> *template_desc = ima_template_desc_current();
>>
>> rcu_read_lock();
>> - list_for_each_entry_rcu(entry, ima_rules, list) {
>> + ima_rules_tmp = rcu_dereference(ima_rules);
>> + list_for_each_entry_rcu(entry, ima_rules_tmp, list) {
>>
>> if (!(entry->action & actmask))
>> continue;
>> @@ -919,8 +921,8 @@ void ima_update_policy(void)
>>
>> if (ima_rules != policy) {
>> ima_policy_flag = 0;
>> - ima_rules = policy;
>>
>> + rcu_assign_pointer(ima_rules, policy);
>> /*
>> * IMA architecture specific policy rules are specified
>> * as strings and converted to an array of ima_entry_rules
>> @@ -1649,9 +1651,11 @@ void *ima_policy_start(struct seq_file *m, loff_t *pos)
>> {
>> loff_t l = *pos;
>> struct ima_rule_entry *entry;
>> + struct list_head *ima_rules_tmp;
>>
>> rcu_read_lock();
>> - list_for_each_entry_rcu(entry, ima_rules, list) {
>> + ima_rules_tmp = rcu_dereference(ima_rules);
>> + list_for_each_entry_rcu(entry, ima_rules_tmp, list) {
>> if (!l--) {
>> rcu_read_unlock();
>> return entry;
>> @@ -1670,7 +1674,8 @@ void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos)
>> rcu_read_unlock();
>> (*pos)++;
>>
>> - return (&entry->list == ima_rules) ? NULL : entry;
>> + return (&entry->list == &ima_default_rules ||
>> + &entry->list == &ima_policy_rules) ? NULL : entry;
>> }
>>
>> void ima_policy_stop(struct seq_file *m, void *v)
>> @@ -1872,6 +1877,7 @@ bool ima_appraise_signature(enum kernel_read_file_id id)
>> struct ima_rule_entry *entry;
>> bool found = false;
>> enum ima_hooks func;
>> + struct list_head *ima_rules_tmp;
>>
>> if (id >= READING_MAX_ID)
>> return false;
>> @@ -1879,7 +1885,8 @@ bool ima_appraise_signature(enum kernel_read_file_id id)
>> func = read_idmap[id] ?: FILE_CHECK;
>>
>> rcu_read_lock();
>> - list_for_each_entry_rcu(entry, ima_rules, list) {
>> + ima_rules_tmp = rcu_dereference(ima_rules);
>> + list_for_each_entry_rcu(entry, ima_rules_tmp, list) {
>> if (entry->action != APPRAISE)
>> continue;
>>
>>
> I haven't tested the patch myself, but the code diff looks fine to me.
>
> Thanks,
> Simon
On Tue, 2021-08-24 at 20:09 +0800, liqiong wrote: > Hi Simon : > > ima: fix deadlock within RCU list of ima_rules. > Before the following paragraph, an introductory sentence is needed. Try adding a sentence to the affect that "ima_rules" initially points to the "ima_default_rules", but after loading a custom policy points to the "ima_policy_rules". Then describe the bug at a high level, something like - transitioning to the "ima_policy_rules" isn't being done safely. Followed by the details. > ima_match_policy() is looping on the policy ruleset while > ima_update_policy() updates the variable "ima_rules". This can > lead to a situation where ima_match_policy() can't exit the > 'list_for_each_entry_rcu' loop, causing RCU stalls > ("rcu_sched detected stall on CPU ..."). > > This problem can happen in practice: updating the IMA policy > in the boot process while systemd-services are being checked. > > In addition to ima_match_policy(), other function with > "list_for_each_entry_rcu" should happen too. Fix locking by > introducing a duplicate of "ima_rules" for each > "list_for_each_entry_rcu". > > > How about this commit message ? > > I have tested this patch in lab, we can reproduced this error case, > have done reboot test many times. This patch should work. The above comment doesn't belong in the commit message, but is a message to the reviewers/maintainers and goes after the patch descriptions three dashes line. thanks, Mimi
Hi Mimi, Thanks for the advice，maybe i should trim the message, here is a new copy: subject: ima: fix deadlock when iterating over the init "ima_rules" list. The init "ima_rules" list can't traverse back to head, if "ima_rules" is being updated to "ima_policy_rules". It causes soft lockup and RCU stalls. So we can introduce a duplicate of "ima_rules" for each "ima_rules" list loop. Signed-off-by: liqiong <liqiong@nfschina.com> --- This problem can happen in practice: updating the IMA policy in the boot process while systemd-services are being checked. security/integrity/ima/ima_policy.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index fd5d46e511f1..e92b197bfd3c 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c Regards, liqiong 在 2021年08月24日 20:38, Mimi Zohar 写道: > On Tue, 2021-08-24 at 20:09 +0800, liqiong wrote: >> Hi Simon : >> >> ima: fix deadlock within RCU list of ima_rules. >> > Before the following paragraph, an introductory sentence is needed. > Try adding a sentence to the affect that "ima_rules" initially points > to the "ima_default_rules", but after loading a custom policy points to > the "ima_policy_rules". Then describe the bug at a high level, > something like - transitioning to the "ima_policy_rules" isn't being > done safely. > > Followed by the details. > >> ima_match_policy() is looping on the policy ruleset while >> ima_update_policy() updates the variable "ima_rules". This can >> lead to a situation where ima_match_policy() can't exit the >> 'list_for_each_entry_rcu' loop, causing RCU stalls >> ("rcu_sched detected stall on CPU ..."). >> >> This problem can happen in practice: updating the IMA policy >> in the boot process while systemd-services are being checked. >> >> In addition to ima_match_policy(), other function with >> "list_for_each_entry_rcu" should happen too. Fix locking by >> introducing a duplicate of "ima_rules" for each >> "list_for_each_entry_rcu". >> >> >> How about this commit message ? >> >> I have tested this patch in lab, we can reproduced this error case, >> have done reboot test many times. This patch should work. > The above comment doesn't belong in the commit message, but is a > message to the reviewers/maintainers and goes after the patch > descriptions three dashes line. > > thanks, > > Mimi > >
Hi Mimi, This copy may be better. subject: ima: fix deadlock when iterating over the init "ima_rules" list. When traversing back to head, the init "ima_rules" list can't exit iterating if "ima_rules" has been updated to "ima_policy_rules". It causes soft lockup and RCU stalls. So we can introduce a duplicate of "ima_rules" for each "ima_rules" list loop. Signed-off-by: liqiong <liqiong@nfschina.com> --- This problem can happen in practice: updating the IMA policy in the boot process while systemd-services are being checked. security/integrity/ima/ima_policy.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index fd5d46e511f1..e92b197bfd3c 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c Regards, liqiong 在 2021年08月25日 15:05, liqiong 写道: > Hi Mimi, > > Thanks for the advice，maybe i should trim the message, > here is a new copy: > > > subject: ima: fix deadlock when iterating over the init "ima_rules" list. > > The init "ima_rules" list can't traverse back to head, if "ima_rules" > is being updated to "ima_policy_rules". It causes soft lockup and RCU stalls. > So we can introduce a duplicate of "ima_rules" for each "ima_rules" list loop. > > Signed-off-by: liqiong <liqiong@nfschina.com> > --- > This problem can happen in practice: updating the IMA policy > in the boot process while systemd-services are being checked. > > security/integrity/ima/ima_policy.c | 17 ++++++++++++----- > 1 file changed, 12 insertions(+), 5 deletions(-) > > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c > index fd5d46e511f1..e92b197bfd3c 100644 > --- a/security/integrity/ima/ima_policy.c > +++ b/security/integrity/ima/ima_policy.c > > > Regards, > > liqiong > > 在 2021年08月24日 20:38, Mimi Zohar 写道: >> On Tue, 2021-08-24 at 20:09 +0800, liqiong wrote: >>> Hi Simon : >>> >>> ima: fix deadlock within RCU list of ima_rules. >>> >> Before the following paragraph, an introductory sentence is needed. >> Try adding a sentence to the affect that "ima_rules" initially points >> to the "ima_default_rules", but after loading a custom policy points to >> the "ima_policy_rules". Then describe the bug at a high level, >> something like - transitioning to the "ima_policy_rules" isn't being >> done safely. >> >> Followed by the details. >> >>> ima_match_policy() is looping on the policy ruleset while >>> ima_update_policy() updates the variable "ima_rules". This can >>> lead to a situation where ima_match_policy() can't exit the >>> 'list_for_each_entry_rcu' loop, causing RCU stalls >>> ("rcu_sched detected stall on CPU ..."). >>> >>> This problem can happen in practice: updating the IMA policy >>> in the boot process while systemd-services are being checked. >>> >>> In addition to ima_match_policy(), other function with >>> "list_for_each_entry_rcu" should happen too. Fix locking by >>> introducing a duplicate of "ima_rules" for each >>> "list_for_each_entry_rcu". >>> >>> >>> How about this commit message ? >>> >>> I have tested this patch in lab, we can reproduced this error case, >>> have done reboot test many times. This patch should work. >> The above comment doesn't belong in the commit message, but is a >> message to the reviewers/maintainers and goes after the patch >> descriptions three dashes line. >> >> thanks, >> >> Mimi >> >> >
Hi Liqiong, On 8/25/21 1:45 PM, liqiong wrote: > Hi Mimi, > > This copy may be better. > > > subject: ima: fix deadlock when iterating over the init "ima_rules" list. > > As Mimi said, consider adding an introducing paragraph, like: 'The current IMA ruleset is identified by the variable "ima_rules", and the pointer starts pointing at the list "ima_default_rules". When loading a custom policy for the first time, the variable is updated to point to the list "ima_policy_rules" instead. That update isn't RCU-safe, and deadlocks are possible.' > > When traversing back to head, the init "ima_rules" list can't exit > iterating if "ima_rules" has been updated to "ima_policy_rules". > It causes soft lockup and RCU stalls. So we can introduce a duplicate > of "ima_rules" for each "ima_rules" list loop. Per the process (see 'Documentation/process/submitting-patches.rst'), please prefer an imperative style (written in another paragraph): 'Introduce a temporary value for "ima_rules" when iterating over the ruleset.' Thanks, Simon
Hi Simon, Thanks for your help, your advice is clear, can i just use it, how about this: The current IMA ruleset is identified by the variable "ima_rules", and the pointer starts pointing at the list "ima_default_rules". When loading a custom policy for the first time, the variable is updated to point to the list "ima_policy_rules" instead. That update isn't RCU-safe, and deadlocks are possible. Introduce a temporary value for "ima_rules" when iterating over the ruleset to avoid the deadlocks. Signed-off-by: liqiong <liqiong@nfschina.com> --- security/integrity/ima/ima_policy.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index fd5d46e511f1..e92b197bfd3c 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c Thanks liqiong 在 2021年08月25日 20:03, THOBY Simon 写道: > Hi Liqiong, > > On 8/25/21 1:45 PM, liqiong wrote: >> Hi Mimi, >> >> This copy may be better. >> >> >> subject: ima: fix deadlock when iterating over the init "ima_rules" list. >> >> > As Mimi said, consider adding an introducing paragraph, like: > 'The current IMA ruleset is identified by the variable "ima_rules", > and the pointer starts pointing at the list "ima_default_rules". When > loading a custom policy for the first time, the variable is > updated to point to the list "ima_policy_rules" instead. That update > isn't RCU-safe, and deadlocks are possible.' > >> When traversing back to head, the init "ima_rules" list can't exit >> iterating if "ima_rules" has been updated to "ima_policy_rules". >> It causes soft lockup and RCU stalls. So we can introduce a duplicate >> of "ima_rules" for each "ima_rules" list loop. > Per the process (see 'Documentation/process/submitting-patches.rst'), > please prefer an imperative style (written in another paragraph): > 'Introduce a temporary value for "ima_rules" when iterating over the ruleset.' > > > Thanks, > Simon
Hi liqiong, On 8/26/21 10:15 AM, liqiong wrote: > Hi Simon, > > Thanks for your help, your advice is clear, can i just use it, > how about this: > > > The current IMA ruleset is identified by the variable "ima_rules", > and the pointer starts pointing at the list "ima_default_rules". After reading it again, maybe "The current IMA ruleset is identified by the variable "ima_rules", that defaults to "&ima_default_rules".'? > When loading a custom policy for the first time, the variable is > updated to point to the list "ima_policy_rules" instead. That update > isn't RCU-safe, and deadlocks are possible. I think what Mimi was trying to say is that you could add the high-level overview above, but keep the details. Sorry if I wasn't clearer in my earlier messages. Consider re-adding your previous paragraph """ As a consequence, when traversing the ruleset, some functions like ima_match_policy() may loop indefinitely over "ima_default_rules" as list_for_each_entry_rcu() doesn't terminate (after the update, "ima_rules" no longer points to the list head, so the loop condition stays always true), causing RCU stalls. """ (note: I tweaked it above, feel free to fix it) > > Introduce a temporary value for "ima_rules" when iterating over > the ruleset to avoid the deadlocks. ... while keeping this a separate paragraph. > > > Signed-off-by: liqiong <liqiong@nfschina.com> > --- > security/integrity/ima/ima_policy.c | 17 ++++++++++++----- > 1 file changed, 12 insertions(+), 5 deletions(-) > > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c > index fd5d46e511f1..e92b197bfd3c 100644 > --- a/security/integrity/ima/ima_policy.c > +++ b/security/integrity/ima/ima_policy.c > > > > Thanks > > liqiong
Hi Simon, Thanks for you help, i may got it, here is the new commit message: The current IMA ruleset is identified by the variable "ima_rules" that default to "&ima_default_rules". When loading a custom policy for the first time, the variable is updated to "&ima_policy_rules" instead. That update isn't RCU-safe, and deadlocks are possible. Because some functions like ima_match_policy() may loop indefinitely over traversing the "ima_default_rules" as list_for_each_entry_rcu(). When iterating over the default ruleset back to head, value of "&entry->list" is "&ima_default_rules", and "ima_rules" may have been updated to "&ima_policy_rules", the loop condition (&entry->list != ima_rules) stay alway true, traversing doesn't terminate, cause soft lockup and RCU stalls. Introduce a temporary value for "ima_rules" when iterating over the ruleset to avoid the deadlocks. Signed-off-by: liqiong <liqiong@nfschina.com> --- security/integrity/ima/ima_policy.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index fd5d46e511f1..e92b197bfd3c 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c Thanks liqiong 在 2021年08月26日 17:01, THOBY Simon 写道: > Hi liqiong, > > On 8/26/21 10:15 AM, liqiong wrote: >> Hi Simon, >> >> Thanks for your help, your advice is clear, can i just use it, >> how about this: >> >> >> The current IMA ruleset is identified by the variable "ima_rules", >> and the pointer starts pointing at the list "ima_default_rules". > After reading it again, maybe > "The current IMA ruleset is identified by the variable "ima_rules", > that defaults to "&ima_default_rules".'? > >> When loading a custom policy for the first time, the variable is >> updated to point to the list "ima_policy_rules" instead. That update >> isn't RCU-safe, and deadlocks are possible. > I think what Mimi was trying to say is that you could add the high-level > overview above, but keep the details. Sorry if I wasn't clearer in my > earlier messages. > > Consider re-adding your previous paragraph > """ > As a consequence, when traversing the ruleset, some functions like ima_match_policy() > may loop indefinitely over "ima_default_rules" as list_for_each_entry_rcu() doesn't > terminate (after the update, "ima_rules" no longer points to the list head, so the > loop condition stays always true), causing RCU stalls. > """ > (note: I tweaked it above, feel free to fix it) >> Introduce a temporary value for "ima_rules" when iterating over >> the ruleset to avoid the deadlocks. > ... while keeping this a separate paragraph. > >> >> Signed-off-by: liqiong <liqiong@nfschina.com> >> --- >> security/integrity/ima/ima_policy.c | 17 ++++++++++++----- >> 1 file changed, 12 insertions(+), 5 deletions(-) >> >> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c >> index fd5d46e511f1..e92b197bfd3c 100644 >> --- a/security/integrity/ima/ima_policy.c >> +++ b/security/integrity/ima/ima_policy.c >> >> >> >> Thanks >> >> liqiong
Hi liqiong, a few nits but nothing significant. This is getting in good shape! On 8/27/21 8:41 AM, liqiong wrote: > Hi Simon, > > Thanks for you help, i may got it, here is the new commit message: > > > The current IMA ruleset is identified by the variable "ima_rules" > that default to "&ima_default_rules". When loading a custom policy > for the first time, the variable is updated to "&ima_policy_rules" > instead. That update isn't RCU-safe, and deadlocks are possible. > Because some functions like ima_match_policy() may loop indefinitely s/Because/Indeed,/ (in english, sentences with a subordinating conjunction like 'because' are usually written in two parts, and a dependent clause standing by itself is rarely used except for stylistic effect) > over traversing the "ima_default_rules" as list_for_each_entry_rcu(). s/over traversing the "ima_default_rules" as list_for_each_entry_rcu()/when traversing "ima_default_rules" with list_for_each_entry_rcu()./ > > When iterating over the default ruleset back to head, value of > "&entry->list" is "&ima_default_rules", and "ima_rules" may have been s/value of "&entry->list" is "&ima_default_rules"/if the list head is "ima_default_rules"/ s/may have been/have been/ > updated to "&ima_policy_rules", the loop condition (&entry->list != ima_rules) > stay alway true, traversing doesn't terminate, cause soft lockup and Don't forget the 's' in "stays" (or "remains") Ditto for "always" s/traversing doesn't/traversing won't/ Also: s/cause/causing a/ > RCU stalls. > > Introduce a temporary value for "ima_rules" when iterating over > the ruleset to avoid the deadlocks. > > > Signed-off-by: liqiong <liqiong@nfschina.com> > --- > security/integrity/ima/ima_policy.c | 17 ++++++++++++----- > 1 file changed, 12 insertions(+), 5 deletions(-) > > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c > index fd5d46e511f1..e92b197bfd3c 100644 > --- a/security/integrity/ima/ima_policy.c > +++ b/security/integrity/ima/ima_policy.c > > > Thanks > > liqiong > Thanks, Simon
Hi Simon,
Thanks for your patient, i learn a lot. If the commit message
does work, i would resubmit the patch. Here is the whole patch:
The current IMA ruleset is identified by the variable "ima_rules"
that default to "&ima_default_rules". When loading a custom policy
for the first time, the variable is updated to "&ima_policy_rules"
instead. That update isn't RCU-safe, and deadlocks are possible.
Indeed, some functions like ima_match_policy() may loop indefinitely
when traversing "ima_default_rules" with list_for_each_entry_rcu().
When iterating over the default ruleset back to head, if the list
head is "ima_default_rules", and "ima_rules" have been updated to
"&ima_policy_rules", the loop condition (&entry->list != ima_rules)
stays always true, traversing won't terminate, causing a soft lockup
and RCU stalls.
Introduce a temporary value for "ima_rules" when iterating over
the ruleset to avoid the deadlocks.
Signed-off-by: liqiong <liqiong@nfschina.com>
---
security/integrity/ima/ima_policy.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index fd5d46e511f1..e92b197bfd3c 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -662,12 +662,14 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode,
{
struct ima_rule_entry *entry;
int action = 0, actmask = flags | (flags << 1);
+ struct list_head *ima_rules_tmp;
if (template_desc && !*template_desc)
*template_desc = ima_template_desc_current();
rcu_read_lock();
- list_for_each_entry_rcu(entry, ima_rules, list) {
+ ima_rules_tmp = rcu_dereference(ima_rules);
+ list_for_each_entry_rcu(entry, ima_rules_tmp, list) {
if (!(entry->action & actmask))
continue;
@@ -919,8 +921,8 @@ void ima_update_policy(void)
if (ima_rules != policy) {
ima_policy_flag = 0;
- ima_rules = policy;
+ rcu_assign_pointer(ima_rules, policy);
/*
* IMA architecture specific policy rules are specified
* as strings and converted to an array of ima_entry_rules
@@ -1649,9 +1651,11 @@ void *ima_policy_start(struct seq_file *m, loff_t *pos)
{
loff_t l = *pos;
struct ima_rule_entry *entry;
+ struct list_head *ima_rules_tmp;
rcu_read_lock();
- list_for_each_entry_rcu(entry, ima_rules, list) {
+ ima_rules_tmp = rcu_dereference(ima_rules);
+ list_for_each_entry_rcu(entry, ima_rules_tmp, list) {
if (!l--) {
rcu_read_unlock();
return entry;
@@ -1670,7 +1674,8 @@ void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos)
rcu_read_unlock();
(*pos)++;
- return (&entry->list == ima_rules) ? NULL : entry;
+ return (&entry->list == &ima_default_rules ||
+ &entry->list == &ima_policy_rules) ? NULL : entry;
}
void ima_policy_stop(struct seq_file *m, void *v)
@@ -1872,6 +1877,7 @@ bool ima_appraise_signature(enum kernel_read_file_id id)
struct ima_rule_entry *entry;
bool found = false;
enum ima_hooks func;
+ struct list_head *ima_rules_tmp;
if (id >= READING_MAX_ID)
return false;
@@ -1879,7 +1885,8 @@ bool ima_appraise_signature(enum kernel_read_file_id id)
func = read_idmap[id] ?: FILE_CHECK;
rcu_read_lock();
- list_for_each_entry_rcu(entry, ima_rules, list) {
+ ima_rules_tmp = rcu_dereference(ima_rules);
+ list_for_each_entry_rcu(entry, ima_rules_tmp, list) {
if (entry->action != APPRAISE)
continue;
On 8/27/21 11:10 AM, liqiong wrote: > Hi Simon, > > Thanks for your patient, i learn a lot. If the commit message > does work, i would resubmit the patch. Here is the whole patch: > > > The current IMA ruleset is identified by the variable "ima_rules" > that default to "&ima_default_rules". When loading a custom policy > for the first time, the variable is updated to "&ima_policy_rules" > instead. That update isn't RCU-safe, and deadlocks are possible. > Indeed, some functions like ima_match_policy() may loop indefinitely > when traversing "ima_default_rules" with list_for_each_entry_rcu(). > > When iterating over the default ruleset back to head, if the list > head is "ima_default_rules", and "ima_rules" have been updated to > "&ima_policy_rules", the loop condition (&entry->list != ima_rules) > stays always true, traversing won't terminate, causing a soft lockup > and RCU stalls. > > Introduce a temporary value for "ima_rules" when iterating over > the ruleset to avoid the deadlocks. > > Signed-off-by: liqiong <liqiong@nfschina.com> > --- > security/integrity/ima/ima_policy.c | 17 ++++++++++++----- > 1 file changed, 12 insertions(+), 5 deletions(-) > > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c > index fd5d46e511f1..e92b197bfd3c 100644 > --- a/security/integrity/ima/ima_policy.c > +++ b/security/integrity/ima/ima_policy.c > @@ -662,12 +662,14 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode, > { > struct ima_rule_entry *entry; > int action = 0, actmask = flags | (flags << 1); > + struct list_head *ima_rules_tmp; > > if (template_desc && !*template_desc) > *template_desc = ima_template_desc_current(); > > rcu_read_lock(); > - list_for_each_entry_rcu(entry, ima_rules, list) { > + ima_rules_tmp = rcu_dereference(ima_rules); > + list_for_each_entry_rcu(entry, ima_rules_tmp, list) { > > if (!(entry->action & actmask)) > continue; > @@ -919,8 +921,8 @@ void ima_update_policy(void) > > if (ima_rules != policy) { > ima_policy_flag = 0; > - ima_rules = policy; > > + rcu_assign_pointer(ima_rules, policy); > /* > * IMA architecture specific policy rules are specified > * as strings and converted to an array of ima_entry_rules > @@ -1649,9 +1651,11 @@ void *ima_policy_start(struct seq_file *m, loff_t *pos) > { > loff_t l = *pos; > struct ima_rule_entry *entry; > + struct list_head *ima_rules_tmp; > > rcu_read_lock(); > - list_for_each_entry_rcu(entry, ima_rules, list) { > + ima_rules_tmp = rcu_dereference(ima_rules); > + list_for_each_entry_rcu(entry, ima_rules_tmp, list) { > if (!l--) { > rcu_read_unlock(); > return entry; > @@ -1670,7 +1674,8 @@ void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos) > rcu_read_unlock(); > (*pos)++; > > - return (&entry->list == ima_rules) ? NULL : entry; > + return (&entry->list == &ima_default_rules || > + &entry->list == &ima_policy_rules) ? NULL : entry; > } > > void ima_policy_stop(struct seq_file *m, void *v) > @@ -1872,6 +1877,7 @@ bool ima_appraise_signature(enum kernel_read_file_id id) > struct ima_rule_entry *entry; > bool found = false; > enum ima_hooks func; > + struct list_head *ima_rules_tmp; > > if (id >= READING_MAX_ID) > return false; > @@ -1879,7 +1885,8 @@ bool ima_appraise_signature(enum kernel_read_file_id id) > func = read_idmap[id] ?: FILE_CHECK; > > rcu_read_lock(); > - list_for_each_entry_rcu(entry, ima_rules, list) { > + ima_rules_tmp = rcu_dereference(ima_rules); > + list_for_each_entry_rcu(entry, ima_rules_tmp, list) { > if (entry->action != APPRAISE) > continue; > > Reviewed-By: THOBY Simon <Simon.THOBY@viveris.fr>
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index fd5d46e511f1..e92b197bfd3c 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -662,12 +662,14 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode, { struct ima_rule_entry *entry; int action = 0, actmask = flags | (flags << 1); + struct list_head *ima_rules_tmp; if (template_desc && !*template_desc) *template_desc = ima_template_desc_current(); rcu_read_lock(); - list_for_each_entry_rcu(entry, ima_rules, list) { + ima_rules_tmp = rcu_dereference(ima_rules); + list_for_each_entry_rcu(entry, ima_rules_tmp, list) { if (!(entry->action & actmask)) continue; @@ -919,8 +921,8 @@ void ima_update_policy(void) if (ima_rules != policy) { ima_policy_flag = 0; - ima_rules = policy; + rcu_assign_pointer(ima_rules, policy); /* * IMA architecture specific policy rules are specified * as strings and converted to an array of ima_entry_rules @@ -1649,9 +1651,11 @@ void *ima_policy_start(struct seq_file *m, loff_t *pos) { loff_t l = *pos; struct ima_rule_entry *entry; + struct list_head *ima_rules_tmp; rcu_read_lock(); - list_for_each_entry_rcu(entry, ima_rules, list) { + ima_rules_tmp = rcu_dereference(ima_rules); + list_for_each_entry_rcu(entry, ima_rules_tmp, list) { if (!l--) { rcu_read_unlock(); return entry; @@ -1670,7 +1674,8 @@ void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos) rcu_read_unlock(); (*pos)++; - return (&entry->list == ima_rules) ? NULL : entry; + return (&entry->list == &ima_default_rules || + &entry->list == &ima_policy_rules) ? NULL : entry; } void ima_policy_stop(struct seq_file *m, void *v) @@ -1872,6 +1877,7 @@ bool ima_appraise_signature(enum kernel_read_file_id id) struct ima_rule_entry *entry; bool found = false; enum ima_hooks func; + struct list_head *ima_rules_tmp; if (id >= READING_MAX_ID) return false; @@ -1879,7 +1885,8 @@ bool ima_appraise_signature(enum kernel_read_file_id id) func = read_idmap[id] ?: FILE_CHECK; rcu_read_lock(); - list_for_each_entry_rcu(entry, ima_rules, list) { + ima_rules_tmp = rcu_dereference(ima_rules); + list_for_each_entry_rcu(entry, ima_rules_tmp, list) { if (entry->action != APPRAISE) continue;
When "ima_match_policy" is looping while "ima_update_policy" changs the variable "ima_rules", then "ima_match_policy" may can't exit loop, Finally cause RCU CPU Stall Warnings: "rcu_sched detected stall on CPU ...". The problem is limited to transitioning from the builtin policy to the custom policy. Eg. At boot time, systemd-services are being checked within "ima_match_policy", at the same time, the variable "ima_rules" is changed by another service. Signed-off-by: liqiong <liqiong@nfschina.com> --- security/integrity/ima/ima_policy.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)