[PATCH] ima: fix infinite loop within "ima_match_policy" function.

[PATCH] ima: fix infinite loop within "ima_match_policy" function.

[liqiong]

When "ima_match_policy" is looping while "ima_update_policy" changs
the variable "ima_rules", then "ima_match_policy" may can't exit loop,
and kernel keeps printf "rcu_sched detected stall on CPU ...".

It occurs at boot phase, systemd-services are being checked within
"ima_match_policy,at the same time, the variable "ima_rules"
is changed by a service.

Signed-off-by: liqiong <liqiong@nfschina.com>
---
 security/integrity/ima/ima_policy.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index fd5d46e511f1..7e71e643457c 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -217,6 +217,7 @@ static LIST_HEAD(ima_default_rules);
 static LIST_HEAD(ima_policy_rules);
 static LIST_HEAD(ima_temp_rules);
 static struct list_head *ima_rules = &ima_default_rules;
+static DECLARE_RWSEM(ima_rules_sem);
 
 static int ima_policy __initdata;
 
@@ -666,6 +667,7 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode,
 	if (template_desc && !*template_desc)
 		*template_desc = ima_template_desc_current();
 
+	down_read(&ima_rules_sem);
 	rcu_read_lock();
 	list_for_each_entry_rcu(entry, ima_rules, list) {
 
@@ -702,6 +704,7 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode,
 			break;
 	}
 	rcu_read_unlock();
+	up_read(&ima_rules_sem);
 
 	return action;
 }
@@ -919,7 +922,9 @@ void ima_update_policy(void)
 
 	if (ima_rules != policy) {
 		ima_policy_flag = 0;
+		down_write(&ima_rules_sem);
 		ima_rules = policy;
+		up_write(&ima_rules_sem);
 
 		/*
 		 * IMA architecture specific policy rules are specified
-- 
2.11.0

Re: [PATCH] ima: fix infinite loop within "ima_match_policy" function.

[THOBY Simon]

Hi Liqiong,

On 8/19/21 12:15 PM, liqiong wrote:
> When "ima_match_policy" is looping while "ima_update_policy" changs
> the variable "ima_rules", then "ima_match_policy" may can't exit loop,
> and kernel keeps printf "rcu_sched detected stall on CPU ...".
> 
> It occurs at boot phase, systemd-services are being checked within
> "ima_match_policy,at the same time, the variable "ima_rules"
> is changed by a service.

First off, thanks for finding and identifying this nasty bug.

> 
> Signed-off-by: liqiong <liqiong@nfschina.com>
> ---
>  security/integrity/ima/ima_policy.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index fd5d46e511f1..7e71e643457c 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -217,6 +217,7 @@ static LIST_HEAD(ima_default_rules);
>  static LIST_HEAD(ima_policy_rules);
>  static LIST_HEAD(ima_temp_rules);
>  static struct list_head *ima_rules = &ima_default_rules;
> +static DECLARE_RWSEM(ima_rules_sem);
>  
>  static int ima_policy __initdata;
>  
> @@ -666,6 +667,7 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode,
>  	if (template_desc && !*template_desc)
>  		*template_desc = ima_template_desc_current();
>  
> +	down_read(&ima_rules_sem);
>  	rcu_read_lock();
>  	list_for_each_entry_rcu(entry, ima_rules, list) {
>  
> @@ -702,6 +704,7 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode,
>  			break;
>  	}
>  	rcu_read_unlock();
> +	up_read(&ima_rules_sem);
>  
>  	return action;
>  }
> @@ -919,7 +922,9 @@ void ima_update_policy(void)
>  
>  	if (ima_rules != policy) {
>  		ima_policy_flag = 0;
> +		down_write(&ima_rules_sem);
>  		ima_rules = policy;
> +		up_write(&ima_rules_sem);
>  
>  		/*
>  		 * IMA architecture specific policy rules are specified
> 

Rather than introducing a new semaphore, I wonder if you couldn't have done something
like the following?

@@ -674,13 +674,15 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode,
                     const char *func_data, unsigned int *allowed_algos)
 {
        struct ima_rule_entry *entry;
+       struct list_head *ima_rules_tmp;
        int action = 0, actmask = flags | (flags << 1);

        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;
@@ -970,7 +972,7 @@ 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


Also, ima_match_policy is not the only place where we iterate over ima_rules, maybe
this change should be applied to every function that perform a call the like of
"list_for_each_entry_rcu(entry, ima_rules_tmp, list)" ?

All that being said, your change is quite small and I have no objection to it,
I was just wondering whether we could achieve the same effect without locks
with RCU.

What do you think?

Thanks,
Simon

Re: [PATCH] ima: fix infinite loop within "ima_match_policy" function.

[Mimi Zohar]

On Thu, 2021-08-19 at 12:58 +0000, THOBY Simon wrote:
> Hi Liqiong,
> 
> On 8/19/21 12:15 PM, liqiong wrote:
> > When "ima_match_policy" is looping while "ima_update_policy" changs
> > the variable "ima_rules", then "ima_match_policy" may can't exit loop,
> > and kernel keeps printf "rcu_sched detected stall on CPU ...".
> > 
> > It occurs at boot phase, systemd-services are being checked within
> > "ima_match_policy,at the same time, the variable "ima_rules"
> > is changed by a service.
> 
> First off, thanks for finding and identifying this nasty bug.

Once the initial builtin policy rules have been replaced by a custom
policy, rules may only be appended by splicing the new rules with the
existing rules.  There should never be a problem reading the rules at
that point.   Does this problem occur before the builtin policy rules
have been replaced with a custom policy?

Mimi

Re: [PATCH] ima: fix infinite loop within "ima_match_policy" function.

[Mimi Zohar]

On Thu, 2021-08-19 at 09:47 -0400, Mimi Zohar wrote:
> On Thu, 2021-08-19 at 12:58 +0000, THOBY Simon wrote:
> > Hi Liqiong,
> > 
> > On 8/19/21 12:15 PM, liqiong wrote:
> > > When "ima_match_policy" is looping while "ima_update_policy" changs
> > > the variable "ima_rules", then "ima_match_policy" may can't exit loop,
> > > and kernel keeps printf "rcu_sched detected stall on CPU ...".
> > > 
> > > It occurs at boot phase, systemd-services are being checked within
> > > "ima_match_policy,at the same time, the variable "ima_rules"
> > > is changed by a service.
> > 
> > First off, thanks for finding and identifying this nasty bug.
> 
> Once the initial builtin policy rules have been replaced by a custom
> policy, rules may only be appended by splicing the new rules with the
> existing rules.  There should never be a problem reading the rules at
> that point.   Does this problem occur before the builtin policy rules
> have been replaced with a custom policy?

Yes, the problem is limited to transitioning from the builtin policy to
the custom policy.   Adding a new lock around rcu code seems counter
productive, especially since switching the policy rules happens once,
normally during early boot before access to real root.  Please consider
Simon's suggestion or finding some other solution.

thanks,

Mimi

Re: [PATCH] ima: fix infinite loop within "ima_match_policy" function.

[李力琼]

Hi, Simon:

This solution is better then rwsem, a temp "ima_rules" variable should 
can fix. I also have a another idea, with a little trick, default list
can traverse to the new list, so we don't need care about the read side. 

here is the patch:

@@ -918,8 +918,21 @@ void ima_update_policy(void)
        list_splice_tail_init_rcu(&ima_temp_rules, policy, synchronize_rcu);

        if (ima_rules != policy) {
+               struct list_head *prev_rules = ima_rules;
+               struct list_head *first = ima_rules->next;
                ima_policy_flag = 0;
+
+               /*
+                * Make the previous list can traverse to new list,
+                * that is tricky, or there is a deadly loop whithin
+                * "list_for_each_entry_rcu(entry, ima_rules, list)"
+                *
+                * After update "ima_rules", restore the previous list.
+                */
+               prev_rules->next = policy->next;
                ima_rules = policy;
+               syncchronize_rcu();
+               prev_rules->next = first;


The side effect is the "ima_default_rules" will be changed a little while.
But it make sense, the process should be checked again by the new policy.

This patch has been tested, if will do, I can resubmit this patch.

How about this ?

----------
Regards,
liqiong

在 2021年08月19日 20:58, THOBY Simon 写道:
> Hi Liqiong,
>
> On 8/19/21 12:15 PM, liqiong wrote:
>> When "ima_match_policy" is looping while "ima_update_policy" changs
>> the variable "ima_rules", then "ima_match_policy" may can't exit loop,
>> and kernel keeps printf "rcu_sched detected stall on CPU ...".
>>
>> It occurs at boot phase, systemd-services are being checked within
>> "ima_match_policy,at the same time, the variable "ima_rules"
>> is changed by a service.
> First off, thanks for finding and identifying this nasty bug.
>
>> Signed-off-by: liqiong <liqiong@nfschina.com>
>> ---
>>  security/integrity/ima/ima_policy.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
>> index fd5d46e511f1..7e71e643457c 100644
>> --- a/security/integrity/ima/ima_policy.c
>> +++ b/security/integrity/ima/ima_policy.c
>> @@ -217,6 +217,7 @@ static LIST_HEAD(ima_default_rules);
>>  static LIST_HEAD(ima_policy_rules);
>>  static LIST_HEAD(ima_temp_rules);
>>  static struct list_head *ima_rules = &ima_default_rules;
>> +static DECLARE_RWSEM(ima_rules_sem);
>>  
>>  static int ima_policy __initdata;
>>  
>> @@ -666,6 +667,7 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode,
>>  	if (template_desc && !*template_desc)
>>  		*template_desc = ima_template_desc_current();
>>  
>> +	down_read(&ima_rules_sem);
>>  	rcu_read_lock();
>>  	list_for_each_entry_rcu(entry, ima_rules, list) {
>>  
>> @@ -702,6 +704,7 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode,
>>  			break;
>>  	}
>>  	rcu_read_unlock();
>> +	up_read(&ima_rules_sem);
>>  
>>  	return action;
>>  }
>> @@ -919,7 +922,9 @@ void ima_update_policy(void)
>>  
>>  	if (ima_rules != policy) {
>>  		ima_policy_flag = 0;
>> +		down_write(&ima_rules_sem);
>>  		ima_rules = policy;
>> +		up_write(&ima_rules_sem);
>>  
>>  		/*
>>  		 * IMA architecture specific policy rules are specified
>>
> Rather than introducing a new semaphore, I wonder if you couldn't have done something
> like the following?
>
> @@ -674,13 +674,15 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode,
>                      const char *func_data, unsigned int *allowed_algos)
>  {
>         struct ima_rule_entry *entry;
> +       struct list_head *ima_rules_tmp;
>         int action = 0, actmask = flags | (flags << 1);
>
>         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;
> @@ -970,7 +972,7 @@ 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
>
>
> Also, ima_match_policy is not the only place where we iterate over ima_rules, maybe
> this change should be applied to every function that perform a call the like of
> "list_for_each_entry_rcu(entry, ima_rules_tmp, list)" ?
>
> All that being said, your change is quite small and I have no objection to it,
> I was just wondering whether we could achieve the same effect without locks
> with RCU.
>
> What do you think?
>
> Thanks,
> Simon

-- 
李力琼<liqiong@nfschina.com>  13524287433
上海市浦东新区海科路99号中科院上海高等研究院3号楼3楼

Re: [PATCH] ima: fix infinite loop within "ima_match_policy" function.

[THOBY Simon]

Hi Liqiong,

On 8/20/21 12:15 PM, 李力琼 wrote:
> Hi, Simon:
> 
> This solution is better then rwsem, a temp "ima_rules" variable should 
> can fix. I also have a another idea, with a little trick, default list
> can traverse to the new list, so we don't need care about the read side. 
> 
> here is the patch:
> 
> @@ -918,8 +918,21 @@ void ima_update_policy(void)
>         list_splice_tail_init_rcu(&ima_temp_rules, policy, synchronize_rcu);
> 
>         if (ima_rules != policy) {
> +               struct list_head *prev_rules = ima_rules;
> +               struct list_head *first = ima_rules->next;
>                 ima_policy_flag = 0;
> +
> +               /*
> +                * Make the previous list can traverse to new list,
> +                * that is tricky, or there is a deadly loop whithin
> +                * "list_for_each_entry_rcu(entry, ima_rules, list)"
> +                *
> +                * After update "ima_rules", restore the previous list.
> +                */

I think this could be rephrased to be a tad clearer, I am not quite sure
how I must interpret the first sentence of the comment.


> +               prev_rules->next = policy->next;
>                 ima_rules = policy;
> +               syncchronize_rcu();

I'm a bit puzzled as you seem to imply in the mail this patch was tested,
but there is no 'syncchronize_rcu' (with two 'c') symbol in the kernel.
Was that a copy/paste error? Or maybe you forgot the 'not' in "This
patch has been tested"? These errors happen, and I am myself quite an
expert in doing them :)

> +               prev_rules->next = first;
> 
> 
> The side effect is the "ima_default_rules" will be changed a little while.
> But it make sense, the process should be checked again by the new policy.
> 
> This patch has been tested, if will do, I can resubmit this patch.> 
> How about this ?


Correct me if I'm wrong, here is how I think I understand you patch.
We start with a situation like that (step 0):
ima_rules --> List entry 0 (head node) = ima_default_rules <-> List entry 1 <-> List entry 2 <-> ... <-> List entry 0

Then we decide to update the policy for the first time, so
'ima_rules [&ima_default_rules] != policy [&ima_policy_rules]'.
We enter the condition.
First we copy the current value of ima_rules (&ima_default_rules)
to a temporary variable 'prev_rules'. We also create a pointer dubbed
'first' to the entry 1 in the default list (step 1):
prev_rules -------------
                       \/
ima_rules --> List entry 0 (head node) = ima_default_rules <-> List entry 1 <-> List entry 2 <-> ... <-> List entry 0
                                                                   /\
first --------------------------------------------------------------


Then we update prev_rules->next to point to policy->next (step 2):
List entry 1 <-> List entry 2 <-> ... -> List entry 0
 /\
first
	(notice that list entry 0 no longer points backwards to 'list entry 1',
	but I don't think there is any reverse iteration in IMA, so it should be
	safe)

prev_rules -------------
                       \/
ima_rules --> List entry 0 (head node) = ima_default_rules   
                       |
                       |
                       -------------------------------------------
                                                                 \/
policy --> policy entry 0' (head node) = ima_policy_rules <-> policy entry 1' <-> policy entry 2' <-> .... <-> policy entry 0'


We then update ima_rules to point to ima_policy_rules (step 3):
List entry 1 <-> List entry 2 <-> ... -> List entry 0
 /\
first

prev_rules -------------
                       \/
ima_rules     List entry 0 (head node) = ima_default_rules   
     |                 |
     |                 |
     |                 ------------------------------------------
     ---------------                                            |
                   \/                                           \/
policy --> policy entry 0' (head node) = ima_policy_rules <-> policy entry 1' <-> policy entry 2' <-> .... <-> policy entry 0'
                                                                   /\
first --------------------------------------------------------------

Then we run synchronize_rcu() to wait for any RCU reader to exit their loops (step 4).

Finally we update prev_rules->next to point back to the ima policy and fix the loop (step 5):

List entry 1 <-> List entry 2 <-> ... -> List entry 0
 /\
first

prev_rules ---> List entry 0 (head node) = ima_default_rules <-> List entry 1 <-> List entry 2 <-> ... <-> List entry 0
                                                                     /\
                                                                 first (now useless)
ima_rules        
     |
     |
     |
     ---------------
                   \/
policy --> policy entry 0' (head node) = ima_policy_rules <-> policy entry 1' <-> policy entry 2' <-> .... <-> policy entry 0'

The goal is that readers should still be able to loop
(forward, as we saw that backward looping is temporarily broken)
while in steps 0-4.

I'm not completely sure what would happen to a client that started iterating
over ima_rules right after step 2.

Wouldn't they be able to start looping through the new policy
as 'List entry 0 (head node) = ima_default_rules' points to ima_policy_rules?
And if they, wouldn't they loop until the write to 'ima_rule' at step 3 (admittedly
very shortly thereafter) completed?
And would the compiler be allowed to optimize the read to 'ima_rules' in the
list_for_each_entry() loop, thereby never reloading the new value for
'ima_rules', and thus looping forever, just what we are trying to avoid?

Overall, I'm tempted to say this is perhaps a bit too complex (at least,
my head tells me it is, but that may very well be because I'm terrible
at concurrency issues).

Honestly, in this case I think awaiting input from more experienced
kernel devs than I is the best path forward :-)

> 
> ----------
> Regards,
> liqiong
> 

Thanks,
Simon

Re: [PATCH] ima: fix infinite loop within "ima_match_policy" function.

[Mimi Zohar]

On Fri, 2021-08-20 at 13:23 +0000, THOBY Simon wrote:
> Hi Liqiong,
> 
> On 8/20/21 12:15 PM, 李力琼 wrote:
> > Hi, Simon:
> > 
> > This solution is better then rwsem, a temp "ima_rules" variable should 
> > can fix. I also have a another idea, with a little trick, default list
> > can traverse to the new list, so we don't need care about the read side. 
> > 
> > here is the patch:
> > 
> > @@ -918,8 +918,21 @@ void ima_update_policy(void)
> >         list_splice_tail_init_rcu(&ima_temp_rules, policy, synchronize_rcu);
> > 
> >         if (ima_rules != policy) {
> > +               struct list_head *prev_rules = ima_rules;
> > +               struct list_head *first = ima_rules->next;
> >                 ima_policy_flag = 0;
> > +
> > +               /*
> > +                * Make the previous list can traverse to new list,
> > +                * that is tricky, or there is a deadly loop whithin
> > +                * "list_for_each_entry_rcu(entry, ima_rules, list)"
> > +                *
> > +                * After update "ima_rules", restore the previous list.
> > +                */
> 
> I think this could be rephrased to be a tad clearer, I am not quite sure
> how I must interpret the first sentence of the comment.
> 
> 
> > +               prev_rules->next = policy->next;
> >                 ima_rules = policy;
> > +               syncchronize_rcu();
> 
> I'm a bit puzzled as you seem to imply in the mail this patch was tested,
> but there is no 'syncchronize_rcu' (with two 'c') symbol in the kernel.
> Was that a copy/paste error? Or maybe you forgot the 'not' in "This
> patch has been tested"? These errors happen, and I am myself quite an
> expert in doing them :)
> 
> > +               prev_rules->next = first;
> > 
> > 
> > The side effect is the "ima_default_rules" will be changed a little while.
> > But it make sense, the process should be checked again by the new policy.
> > 
> > This patch has been tested, if will do, I can resubmit this patch.> 
> > How about this ?
> 
> 
> Correct me if I'm wrong, here is how I think I understand you patch.
> We start with a situation like that (step 0):
> ima_rules --> List entry 0 (head node) = ima_default_rules <-> List entry 1 <-> List entry 2 <-> ... <-> List entry 0
> 
> Then we decide to update the policy for the first time, so
> 'ima_rules [&ima_default_rules] != policy [&ima_policy_rules]'.
> We enter the condition.
> First we copy the current value of ima_rules (&ima_default_rules)
> to a temporary variable 'prev_rules'. We also create a pointer dubbed
> 'first' to the entry 1 in the default list (step 1):
> prev_rules -------------
>                        \/
> ima_rules --> List entry 0 (head node) = ima_default_rules <-> List entry 1 <-> List entry 2 <-> ... <-> List entry 0
>                                                                    /\
> first --------------------------------------------------------------
> 
> 
> Then we update prev_rules->next to point to policy->next (step 2):
> List entry 1 <-> List entry 2 <-> ... -> List entry 0
>  /\
> first
> 	(notice that list entry 0 no longer points backwards to 'list entry 1',
> 	but I don't think there is any reverse iteration in IMA, so it should be
> 	safe)
> 
> prev_rules -------------
>                        \/
> ima_rules --> List entry 0 (head node) = ima_default_rules   
>                        |
>                        |
>                        -------------------------------------------
>                                                                  \/
> policy --> policy entry 0' (head node) = ima_policy_rules <-> policy entry 1' <-> policy entry 2' <-> .... <-> policy entry 0'
> 
> 
> We then update ima_rules to point to ima_policy_rules (step 3):
> List entry 1 <-> List entry 2 <-> ... -> List entry 0
>  /\
> first
> 
> prev_rules -------------
>                        \/
> ima_rules     List entry 0 (head node) = ima_default_rules   
>      |                 |
>      |                 |
>      |                 ------------------------------------------
>      ---------------                                            |
>                    \/                                           \/
> policy --> policy entry 0' (head node) = ima_policy_rules <-> policy entry 1' <-> policy entry 2' <-> .... <-> policy entry 0'
>                                                                    /\
> first --------------------------------------------------------------
> 
> Then we run synchronize_rcu() to wait for any RCU reader to exit their loops (step 4).
> 
> Finally we update prev_rules->next to point back to the ima policy and fix the loop (step 5):
> 
> List entry 1 <-> List entry 2 <-> ... -> List entry 0
>  /\
> first
> 
> prev_rules ---> List entry 0 (head node) = ima_default_rules <-> List entry 1 <-> List entry 2 <-> ... <-> List entry 0
>                                                                      /\
>                                                                  first (now useless)
> ima_rules        
>      |
>      |
>      |
>      ---------------
>                    \/
> policy --> policy entry 0' (head node) = ima_policy_rules <-> policy entry 1' <-> policy entry 2' <-> .... <-> policy entry 0'
> 
> The goal is that readers should still be able to loop
> (forward, as we saw that backward looping is temporarily broken)
> while in steps 0-4.
> 
> I'm not completely sure what would happen to a client that started iterating
> over ima_rules right after step 2.
> 
> Wouldn't they be able to start looping through the new policy
> as 'List entry 0 (head node) = ima_default_rules' points to ima_policy_rules?
> And if they, wouldn't they loop until the write to 'ima_rule' at step 3 (admittedly
> very shortly thereafter) completed?
> And would the compiler be allowed to optimize the read to 'ima_rules' in the
> list_for_each_entry() loop, thereby never reloading the new value for
> 'ima_rules', and thus looping forever, just what we are trying to avoid?
> 
> Overall, I'm tempted to say this is perhaps a bit too complex (at least,
> my head tells me it is, but that may very well be because I'm terrible
> at concurrency issues).
> 
> Honestly, in this case I think awaiting input from more experienced
> kernel devs than I is the best path forward :-)

I'm far from an expert on RCU locking, but __list_splice_init_rcu()
provides an example of how to make sure there aren't any readers
traversing the list, before two lists are spliced together.   In our
case, after there aren't any readers, instead of splicing two lists
together, it should be safe to point to the new list.

thanks,

Mimi

Re: [PATCH] ima: fix infinite loop within "ima_match_policy" function.

[liqiong]

Hi Simon,

On 2021/8/20 21:23, THOBY Simon wrote:
> Hi Liqiong,
>
> On 8/20/21 12:15 PM, 李力琼 wrote:
>> Hi, Simon:
>>
>> This solution is better then rwsem, a temp "ima_rules" variable should
>> can fix. I also have a another idea, with a little trick, default list
>> can traverse to the new list, so we don't need care about the read side.
>>
>> here is the patch:
>>
>> @@ -918,8 +918,21 @@ void ima_update_policy(void)
>>          list_splice_tail_init_rcu(&ima_temp_rules, policy, synchronize_rcu);
>>
>>          if (ima_rules != policy) {
>> +               struct list_head *prev_rules = ima_rules;
>> +               struct list_head *first = ima_rules->next;
>>                  ima_policy_flag = 0;
>> +
>> +               /*
>> +                * Make the previous list can traverse to new list,
>> +                * that is tricky, or there is a deadly loop whithin
>> +                * "list_for_each_entry_rcu(entry, ima_rules, list)"
>> +                *
>> +                * After update "ima_rules", restore the previous list.
>> +                */
> I think this could be rephrased to be a tad clearer, I am not quite sure
> how I must interpret the first sentence of the comment.
I got it,  how about this:
  /*
   * The previous list has to traverse to new list,
   * Or there may be a deadly loop within
   * "list_for_each_entry_rcu(entry, ima_rules, list)"
   *
   * That is tricky, after updated "ima_rules", restore the previous list.
   */
>
>
>> +               prev_rules->next = policy->next;
>>                  ima_rules = policy;
>> +               syncchronize_rcu();
> I'm a bit puzzled as you seem to imply in the mail this patch was tested,
> but there is no 'syncchronize_rcu' (with two 'c') symbol in the kernel.
> Was that a copy/paste error? Or maybe you forgot the 'not' in "This
> patch has been tested"? These errors happen, and I am myself quite an
> expert in doing them :)


Sorry for the mistake, I copy/paste the patch and delete/edit some lines,
have reviewed before sending, but not found. I have made a case to reproduce
the error, dumping "ima_rules" and every item address of list in the error
situaiton, I can watchthe "ima_rules" change, old list traversing to the 
new list.
And I have been doing a reboot test which found this bug. This patch 
seems to work fine.


>
>> +               prev_rules->next = first;
>>
>>
>> The side effect is the "ima_default_rules" will be changed a little while.
>> But it make sense, the process should be checked again by the new policy.
>>
>> This patch has been tested, if will do, I can resubmit this patch.>
>> How about this ?
>
> Correct me if I'm wrong, here is how I think I understand you patch.
> We start with a situation like that (step 0):
> ima_rules --> List entry 0 (head node) = ima_default_rules <-> List entry 1 <-> List entry 2 <-> ... <-> List entry 0
>
> Then we decide to update the policy for the first time, so
> 'ima_rules [&ima_default_rules] != policy [&ima_policy_rules]'.
> We enter the condition.
> First we copy the current value of ima_rules (&ima_default_rules)
> to a temporary variable 'prev_rules'. We also create a pointer dubbed
> 'first' to the entry 1 in the default list (step 1):
> prev_rules -------------
>                         \/
> ima_rules --> List entry 0 (head node) = ima_default_rules <-> List entry 1 <-> List entry 2 <-> ... <-> List entry 0
>                                                                     /\
> first --------------------------------------------------------------
>
>
> Then we update prev_rules->next to point to policy->next (step 2):
> List entry 1 <-> List entry 2 <-> ... -> List entry 0
>   /\
> first
> 	(notice that list entry 0 no longer points backwards to 'list entry 1',
> 	but I don't think there is any reverse iteration in IMA, so it should be
> 	safe)
>
> prev_rules -------------
>                         \/
> ima_rules --> List entry 0 (head node) = ima_default_rules
>                         |
>                         |
>                         -------------------------------------------
>                                                                   \/
> policy --> policy entry 0' (head node) = ima_policy_rules <-> policy entry 1' <-> policy entry 2' <-> .... <-> policy entry 0'
>
>
> We then update ima_rules to point to ima_policy_rules (step 3):
> List entry 1 <-> List entry 2 <-> ... -> List entry 0
>   /\
> first
>
> prev_rules -------------
>                         \/
> ima_rules     List entry 0 (head node) = ima_default_rules
>       |                 |
>       |                 |
>       |                 ------------------------------------------
>       ---------------                                            |
>                     \/                                           \/
> policy --> policy entry 0' (head node) = ima_policy_rules <-> policy entry 1' <-> policy entry 2' <-> .... <-> policy entry 0'
>                                                                     /\
> first --------------------------------------------------------------
>
> Then we run synchronize_rcu() to wait for any RCU reader to exit their loops (step 4).
>
> Finally we update prev_rules->next to point back to the ima policy and fix the loop (step 5):
>
> List entry 1 <-> List entry 2 <-> ... -> List entry 0
>   /\
> first
>
> prev_rules ---> List entry 0 (head node) = ima_default_rules <-> List entry 1 <-> List entry 2 <-> ... <-> List entry 0
>                                                                       /\
>                                                                   first (now useless)
> ima_rules
>       |
>       |
>       |
>       ---------------
>                     \/
> policy --> policy entry 0' (head node) = ima_policy_rules <-> policy entry 1' <-> policy entry 2' <-> .... <-> policy entry 0'
>
> The goal is that readers should still be able to loop
> (forward, as we saw that backward looping is temporarily broken)
> while in steps 0-4.


Yes, It's the workflow.


> I'm not completely sure what would happen to a client that started iterating
> over ima_rules right after step 2.
>
> Wouldn't they be able to start looping through the new policy
> as 'List entry 0 (head node) = ima_default_rules' points to ima_policy_rules?
> And if they, wouldn't they loop until the write to 'ima_rule' at step 3 (admittedly
> very shortly thereafter) completed?
> And would the compiler be allowed to optimize the read to 'ima_rules' in the
> list_for_each_entry() loop, thereby never reloading the new value for
> 'ima_rules', and thus looping forever, just what we are trying to avoid?


Yes,  "ima_rules" cache not update in time, It's a risk. I am not sure 
if "WRITE_ONCE"
can do this trick. How about:
     WRITE_ONCE(prev_rules->next, policy->next);
     WRITE_ONCE(ima_rules, policy);


If can't fix the cache issue, maybe the "ima_rules_tmp" solution is the 
best way.
I will test it.


> Overall, I'm tempted to say this is perhaps a bit too complex (at least,
> my head tells me it is, but that may very well be because I'm terrible
> at concurrency issues).
>
> Honestly, in this case I think awaiting input from more experienced
> kernel devs than I is the best path forward :-)
>
>> ----------
>> Regards,
>> liqiong
>>
> Thanks,
> Simon

Re: [PATCH] ima: fix infinite loop within "ima_match_policy" function.

[李力琼]

Hi Mimi :

The situation is a little different,'list_splice_init_rcu'
don't change the list head. If "ima_rules" being changed,
readers may can't reload the new value in time for cpu cache
or compiler optimization. Defining "ima_rules" as a volatile 
variable can fix, but It is inefficient.

Maybe using a temporary ima_rules variable for every 
"list_for_each_entry_rcu(entry, ima_rules, list)" loop is 
a better solution to fix the "endless loop" bug. 

Regards,

liqiong

在 2021年08月20日 23:48, Mimi Zohar 写道:
> On Fri, 2021-08-20 at 13:23 +0000, THOBY Simon wrote:
>> Hi Liqiong,
>>
>> On 8/20/21 12:15 PM, 李力琼 wrote:
>>> Hi, Simon:
>>>
>>> This solution is better then rwsem, a temp "ima_rules" variable should 
>>> can fix. I also have a another idea, with a little trick, default list
>>> can traverse to the new list, so we don't need care about the read side. 
>>>
>>> here is the patch:
>>>
>>> @@ -918,8 +918,21 @@ void ima_update_policy(void)
>>>         list_splice_tail_init_rcu(&ima_temp_rules, policy, synchronize_rcu);
>>>
>>>         if (ima_rules != policy) {
>>> +               struct list_head *prev_rules = ima_rules;
>>> +               struct list_head *first = ima_rules->next;
>>>                 ima_policy_flag = 0;
>>> +
>>> +               /*
>>> +                * Make the previous list can traverse to new list,
>>> +                * that is tricky, or there is a deadly loop whithin
>>> +                * "list_for_each_entry_rcu(entry, ima_rules, list)"
>>> +                *
>>> +                * After update "ima_rules", restore the previous list.
>>> +                */
>> I think this could be rephrased to be a tad clearer, I am not quite sure
>> how I must interpret the first sentence of the comment.
>>
>>
>>> +               prev_rules->next = policy->next;
>>>                 ima_rules = policy;
>>> +               syncchronize_rcu();
>> I'm a bit puzzled as you seem to imply in the mail this patch was tested,
>> but there is no 'syncchronize_rcu' (with two 'c') symbol in the kernel.
>> Was that a copy/paste error? Or maybe you forgot the 'not' in "This
>> patch has been tested"? These errors happen, and I am myself quite an
>> expert in doing them :)
>>
>>> +               prev_rules->next = first;
>>>
>>>
>>> The side effect is the "ima_default_rules" will be changed a little while.
>>> But it make sense, the process should be checked again by the new policy.
>>>
>>> This patch has been tested, if will do, I can resubmit this patch.> 
>>> How about this ?
>> least
>>
>> Correct me if I'm wrong, here is how I think I understand you patch.
>> We start with a situation like that (step 0):
>> ima_rules --> List entry 0 (head node) = ima_default_rules <-> List entry 1 <-> List entry 2 <-> ... <-> List entry 0
>>
>> Then we decide to update the policy for the first time, so
>> 'ima_rules [&ima_default_rules] != policy [&ima_policy_rules]'.
>> We enter the condition.
>> First we copy the current value of ima_rules (&ima_default_rules)
>> to a temporary variable 'prev_rules'. We also create a pointer dubbed
>> 'first' to the entry 1 in the default list (step 1):
>> prev_rules -------------
>>                        \/
>> ima_rules --> List entry 0 (head node) = ima_default_rules <-> List entry 1 <-> List entry 2 <-> ... <-> List entry 0
>>                                                                    /\
>> first --------------------------------------------------------------
>>
>>
>> Then we update prev_rules->next to point to policy->next (step 2):
>> List entry 1 <-> List entry 2 <-> ... -> List entry 0
>>  /\
>> first
>> 	(notice that list entry 0 no longer points backwards to 'list entry 1',
>> 	but I don't think there is any reverse iteration in IMA, so it should be
>> 	safe)
>>
>> prev_rules -------------
>>                        \/
>> ima_rules --> List entry 0 (head node) = ima_default_rules   
>>                        |
>>                        |
>>                        -------------------------------------------
>>                                                                  \/
>> policy --> policy entry 0' (head node) = ima_policy_rules <-> policy entry 1' <-> policy entry 2' <-> .... <-> policy entry 0'
>>
>>
>> We then update ima_rules to point to ima_policy_rules (step 3):
>> List entry 1 <-> List entry 2 <-> ... -> List entry 0
>>  /\
>> first
>>
>> prev_rules -------------
>>                        \/
>> ima_rules     List entry 0 (head node) = ima_default_rules   
>>      |                 |
>>      |                 |
>>      |                 ------------------------------------------
>>      ---------------                                            |
>>                    \/                                           \/
>> policy --> policy entry 0' (head node) = ima_policy_rules <-> policy entry 1' <-> policy entry 2' <-> .... <-> policy entry 0'
>>                                                   synchronize_rcu                 /\
>> first --------------------------------------------------------------
>>
>> Then we run synchronize_rcu() to wait for any RCU reader to exit their loops (step 4).
>>
>> Finally we update prev_rules->next to point back to the ima policy and fix the loop (step 5):
>>
>> List entry 1 <-> List entry 2 <-> ... -> List entry 0
>>  /\
>> first
>>
>> prev_rules ---> List entry 0 (head node) = ima_default_rules <-> List entry 1 <-> List entry 2 <-> ... <-> List entry 0
>>                                                                      /\
>>                                                                  first (now useless)
>> ima_rules        
>>      |
>>      |
>>      |
>>      ---------------
>>                    \/
>> policy --> policy entry 0' (head node) = ima_policy_rules <-> policy entry 1' <-> policy entry 2' <-> .... <-> policy entry 0'
>>
>> The goal is that readers should still be able to loop
>> (forward, as we saw that backward looping is temporarily broken)
>> while in steps 0-4.
>>
>> I'm not completely sure what would happen to a client that started iterating
>> over ima_rules right after step 2.
>>
>> Wouldn't they be able to start looping through the new policy
>> as 'List entry 0 (head node) = ima_default_rules' points to ima_policy_rules?
>> And if they, wouldn't they loop until the write to 'ima_rule' at step 3 (admittedly
>> very shortly thereafter) completed?
>> And would the compiler be allowed to optimize the read to 'ima_rules' in the
>> list_for_each_entry() loop, thereby never reloading the new value for
>> 'ima_rules', and thus looping forever, just what we are trying to avoid?
>>
>> Overall, I'm tempted to say this is perhaps a bit too complex (at least,
>> my head tells me it is, but that may very well be because I'm terrible
>> at concurrency issues).
>>
>> Honestly, in this case I think awaiting input from more experienced
>> kernel devs than I is the best path forward :-)
> I'm far from an expert on RCU locking, but __list_splice_init_rcu()
> provides an example of how to make sure there aren't any readers
> traversing the list, before two lists are spliced together.   In our
> case, after there aren't any readers, instead of splicing two lists
> together, it should be safe to point to the new list.
>
> thanks,
>
> Mimi
>

Re: [PATCH] ima: fix infinite loop within "ima_match_policy" function.

[THOBY Simon]

Hi Liqiong,

On 8/20/21 7:53 PM, liqiong wrote:
> Hi Simon,
> 
> On 2021/8/20 21:23, THOBY Simon wrote:
>> Hi Liqiong,
>>
>> On 8/20/21 12:15 PM, 李力琼 wrote:
>>> Hi, Simon:
>>>
>>> This solution is better then rwsem, a temp "ima_rules" variable should
>>> can fix. I also have a another idea, with a little trick, default list
>>> can traverse to the new list, so we don't need care about the read side.
>>>
>>> here is the patch:
>>>
>>> @@ -918,8 +918,21 @@ void ima_update_policy(void)
>>>          list_splice_tail_init_rcu(&ima_temp_rules, policy, synchronize_rcu);
>>>
>>>          if (ima_rules != policy) {
>>> +               struct list_head *prev_rules = ima_rules;
>>> +               struct list_head *first = ima_rules->next;
>>>                  ima_policy_flag = 0;
>>> +
>>> +               /*
>>> +                * Make the previous list can traverse to new list,
>>> +                * that is tricky, or there is a deadly loop whithin
>>> +                * "list_for_each_entry_rcu(entry, ima_rules, list)"
>>> +                *
>>> +                * After update "ima_rules", restore the previous list.
>>> +                */
>> I think this could be rephrased to be a tad clearer, I am not quite sure
>> how I must interpret the first sentence of the comment.
> I got it,  how about this:
>  /*
>   * The previous list has to traverse to new list,
>   * Or there may be a deadly loop within

Maybe 'deadlock' would be clearer than 'deadly loop'?

>   * "list_for_each_entry_rcu(entry, ima_rules, list)"
>   *
>   * That is tricky, after updated "ima_rules", restore the previous list.

Maybe something like "This is tricky, so we restore the previous list (ima_default_rules)
once 'ima_rules' is updated" ?

>   */>>
>>
>>> +               prev_rules->next = policy->next;
>>>                  ima_rules = policy;
>>> +               syncchronize_rcu();
>> I'm a bit puzzled as you seem to imply in the mail this patch was tested,
>> but there is no 'syncchronize_rcu' (with two 'c') symbol in the kernel.
>> Was that a copy/paste error? Or maybe you forgot the 'not' in "This
>> patch has been tested"? These errors happen, and I am myself quite an
>> expert in doing them :)
> 
> 
> Sorry for the mistake, I copy/paste the patch and delete/edit some lines,
> have reviewed before sending, but not found. I have made a case to reproduce
> the error, dumping "ima_rules" and every item address of list in the error
> situaiton, I can watchthe "ima_rules" change, old list traversing to the new list.
> And I have been doing a reboot test which found this bug. This patch seems to work fine.
> 

No worries, i just wanted to make sure I understood you correctly.

> 
>>
>>> +               prev_rules->next = first;
>>>
>>>
>>> The side effect is the "ima_default_rules" will be changed a little while.
>>> But it make sense, the process should be checked again by the new policy.
>>>
>>> This patch has been tested, if will do, I can resubmit this patch.>
>>> How about this ?
>>
>> Correct me if I'm wrong, here is how I think I understand you patch.
>> We start with a situation like that (step 0):
>> ima_rules --> List entry 0 (head node) = ima_default_rules <-> List entry 1 <-> List entry 2 <-> ... <-> List entry 0
>>
>> Then we decide to update the policy for the first time, so
>> 'ima_rules [&ima_default_rules] != policy [&ima_policy_rules]'.
>> We enter the condition.
>> First we copy the current value of ima_rules (&ima_default_rules)
>> to a temporary variable 'prev_rules'. We also create a pointer dubbed
>> 'first' to the entry 1 in the default list (step 1):
>> prev_rules -------------
>>                         \/
>> ima_rules --> List entry 0 (head node) = ima_default_rules <-> List entry 1 <-> List entry 2 <-> ... <-> List entry 0
>>                                                                     /\
>> first --------------------------------------------------------------
>>
>>
>> Then we update prev_rules->next to point to policy->next (step 2):
>> List entry 1 <-> List entry 2 <-> ... -> List entry 0
>>   /\
>> first
>>     (notice that list entry 0 no longer points backwards to 'list entry 1',
>>     but I don't think there is any reverse iteration in IMA, so it should be
>>     safe)
>>
>> prev_rules -------------
>>                         \/
>> ima_rules --> List entry 0 (head node) = ima_default_rules
>>                         |
>>                         |
>>                         -------------------------------------------
>>                                                                   \/
>> policy --> policy entry 0' (head node) = ima_policy_rules <-> policy entry 1' <-> policy entry 2' <-> .... <-> policy entry 0'
>>
>>
>> We then update ima_rules to point to ima_policy_rules (step 3):
>> List entry 1 <-> List entry 2 <-> ... -> List entry 0
>>   /\
>> first
>>
>> prev_rules -------------
>>                         \/
>> ima_rules     List entry 0 (head node) = ima_default_rules
>>       |                 |
>>       |                 |
>>       |                 ------------------------------------------
>>       ---------------                                            |
>>                     \/                                           \/
>> policy --> policy entry 0' (head node) = ima_policy_rules <-> policy entry 1' <-> policy entry 2' <-> .... <-> policy entry 0'
>>                                                                     /\
>> first --------------------------------------------------------------
>>
>> Then we run synchronize_rcu() to wait for any RCU reader to exit their loops (step 4).
>>
>> Finally we update prev_rules->next to point back to the ima policy and fix the loop (step 5):
>>
>> List entry 1 <-> List entry 2 <-> ... -> List entry 0
>>   /\
>> first
>>
>> prev_rules ---> List entry 0 (head node) = ima_default_rules <-> List entry 1 <-> List entry 2 <-> ... <-> List entry 0
>>                                                                       /\
>>                                                                   first (now useless)
>> ima_rules
>>       |
>>       |
>>       |
>>       ---------------
>>                     \/
>> policy --> policy entry 0' (head node) = ima_policy_rules <-> policy entry 1' <-> policy entry 2' <-> .... <-> policy entry 0'
>>
>> The goal is that readers should still be able to loop
>> (forward, as we saw that backward looping is temporarily broken)
>> while in steps 0-4.
> 
> 
> Yes, It's the workflow.
> 
> 
>> I'm not completely sure what would happen to a client that started iterating
>> over ima_rules right after step 2.
>>
>> Wouldn't they be able to start looping through the new policy
>> as 'List entry 0 (head node) = ima_default_rules' points to ima_policy_rules?
>> And if they, wouldn't they loop until the write to 'ima_rule' at step 3 (admittedly
>> very shortly thereafter) completed?
>> And would the compiler be allowed to optimize the read to 'ima_rules' in the
>> list_for_each_entry() loop, thereby never reloading the new value for
>> 'ima_rules', and thus looping forever, just what we are trying to avoid?
> 
> 
> Yes,  "ima_rules" cache not update in time, It's a risk. I am not sure if "WRITE_ONCE"
> can do this trick. How about:
>     WRITE_ONCE(prev_rules->next, policy->next);
>     WRITE_ONCE(ima_rules, policy);

Quite frankly, I don't know. As I said earlier, this is really way above my level.
I'm fine waiting for more experienced opinions on this one.

On the aspect of maintainability, I do think this solution is perhaps too complex
when compared to other solutions like the semaphore you first proposed.
A solution of similar complexity with RCU would be ideal to prevent adding a
semaphore on a read-mostly scenario, but I'm still more confident in the semaphore
than in the solution above, because it is easy to have confidence in the semaphore,
while this patch is not at all obvious to me, and maybe the next person who will
have to edit that piece of code.

> 
> If can't fix the cache issue, maybe the "ima_rules_tmp" solution is the best way.
> I will test it.
> 
> 
>> Overall, I'm tempted to say this is perhaps a bit too complex (at least,
>> my head tells me it is, but that may very well be because I'm terrible
>> at concurrency issues).
>>
>> Honestly, in this case I think awaiting input from more experienced
>> kernel devs than I is the best path forward :-)
>>
>>> ----------
>>> Regards,
>>> liqiong

Thanks,
Simon

Re: [PATCH] ima: fix infinite loop within "ima_match_policy" function.

[李力琼]

Hi Simon :

Using a temporary ima_rules variable is not working for "ima_policy_next". void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos) { struct ima_rule_entry *entry = v; + struct list_head *ima_rules_tmp = rcu_dereference(ima_rules); rcu_read_lock(); entry = list_entry_rcu(entry->list.next, struct ima_rule_entry, list); rcu_read_unlock(); (*pos)++; - return (&entry->list == ima_rules) ? NULL : entry; + return (&entry->list == ima_rules_tmp) ? NULL : entry; }

It seems no way to fix "ima_rules" change within this function, it will alway
return a entry if "ima_rules" being changed.

Regrads,

liqiong

在 2021年08月23日 11:04, 李力琼 写道:
> Hi Mimi :
>
> The situation is a little different,'list_splice_init_rcu'
> don't change the list head. If "ima_rules" being changed,
> readers may can't reload the new value in time for cpu cache
> or compiler optimization. Defining "ima_rules" as a volatile 
> variable can fix, but It is inefficient.
>
> Maybe using a temporary ima_rules variable for every 
> "list_for_each_entry_rcu(entry, ima_rules, list)" loop is 
> a better solution to fix the "endless loop" bug. 
>
> Regards,
>
> liqiong
>
> 在 2021年08月20日 23:48, Mimi Zohar 写道:
>> On Fri, 2021-08-20 at 13:23 +0000, THOBY Simon wrote:
>>> Hi Liqiong,
>>>
>>> On 8/20/21 12:15 PM, 李力琼 wrote:
>>>> Hi, Simon:
>>>>
>>>> This solution is better then rwsem, a temp "ima_rules" variable should 
>>>> can fix. I also have a another idea, with a little trick, default list
>>>> can traverse to the new list, so we don't need care about the read side. 
>>>>
>>>> here is the patch:
>>>>
>>>> @@ -918,8 +918,21 @@ void ima_update_policy(void)
>>>>         list_splice_tail_init_rcu(&ima_temp_rules, policy, synchronize_rcu);
>>>>
>>>>         if (ima_rules != policy) {
>>>> +               struct list_head *prev_rules = ima_rules;
>>>> +               struct list_head *first = ima_rules->next;
>>>>                 ima_policy_flag = 0;
>>>> +
>>>> +               /*
>>>> +                * Make the previous list can traverse to new list,
>>>> +                * that is tricky, or there is a deadly loop whithin
>>>> +                * "list_for_each_entry_rcu(entry, ima_rules, list)"
>>>> +                *
>>>> +                * After update "ima_rules", restore the previous list.
>>>> +                */
>>> I think this could be rephrased to be a tad clearer, I am not quite sure
>>> how I must interpret the first sentence of the comment.
>>>
>>>
>>>> +               prev_rules->next = policy->next;
>>>>                 ima_rules = policy;
>>>> +               syncchronize_rcu();
>>> I'm a bit puzzled as you seem to imply in the mail this patch was tested,
>>> but there is no 'syncchronize_rcu' (with two 'c') symbol in the kernel.
>>> Was that a copy/paste error? Or maybe you forgot the 'not' in "This
>>> patch has been tested"? These errors happen, and I am myself quite an
>>> expert in doing them :)
>>>
>>>> +               prev_rules->next = first;
>>>>
>>>>
>>>> The side effect is the "ima_default_rules" will be changed a little while.
>>>> But it make sense, the process should be checked again by the new policy.
>>>>
>>>> This patch has been tested, if will do, I can resubmit this patch.> 
>>>> How about this ?
>>> least
>>>
>>> Correct me if I'm wrong, here is how I think I understand you patch.
>>> We start with a situation like that (step 0):
>>> ima_rules --> List entry 0 (head node) = ima_default_rules <-> List entry 1 <-> List entry 2 <-> ... <-> List entry 0
>>>
>>> Then we decide to update the policy for the first time, so
>>> 'ima_rules [&ima_default_rules] != policy [&ima_policy_rules]'.
>>> We enter the condition.
>>> First we copy the current value of ima_rules (&ima_default_rules)
>>> to a temporary variable 'prev_rules'. We also create a pointer dubbed
>>> 'first' to the entry 1 in the default list (step 1):
>>> prev_rules -------------
>>>                        \/
>>> ima_rules --> List entry 0 (head node) = ima_default_rules <-> List entry 1 <-> List entry 2 <-> ... <-> List entry 0
>>>                                                                    /\
>>> first --------------------------------------------------------------
>>>
>>>
>>> Then we update prev_rules->next to point to policy->next (step 2):
>>> List entry 1 <-> List entry 2 <-> ... -> List entry 0
>>>  /\
>>> first
>>> 	(notice that list entry 0 no longer points backwards to 'list entry 1',
>>> 	but I don't think there is any reverse iteration in IMA, so it should be
>>> 	safe)
>>>
>>> prev_rules -------------
>>>                        \/
>>> ima_rules --> List entry 0 (head node) = ima_default_rules   
>>>                        |
>>>                        |
>>>                        -------------------------------------------
>>>                                                                  \/
>>> policy --> policy entry 0' (head node) = ima_policy_rules <-> policy entry 1' <-> policy entry 2' <-> .... <-> policy entry 0'
>>>
>>>
>>> We then update ima_rules to point to ima_policy_rules (step 3):
>>> List entry 1 <-> List entry 2 <-> ... -> List entry 0
>>>  /\
>>> first
>>>
>>> prev_rules -------------
>>>                        \/
>>> ima_rules     List entry 0 (head node) = ima_default_rules   
>>>      |                 |
>>>      |                 |
>>>      |                 ------------------------------------------
>>>      ---------------                                            |
>>>                    \/                                           \/
>>> policy --> policy entry 0' (head node) = ima_policy_rules <-> policy entry 1' <-> policy entry 2' <-> .... <-> policy entry 0'
>>>                                                   synchronize_rcu                 /\
>>> first --------------------------------------------------------------
>>>
>>> Then we run synchronize_rcu() to wait for any RCU reader to exit their loops (step 4).
>>>
>>> Finally we update prev_rules->next to point back to the ima policy and fix the loop (step 5):
>>>
>>> List entry 1 <-> List entry 2 <-> ... -> List entry 0
>>>  /\
>>> first
>>>
>>> prev_rules ---> List entry 0 (head node) = ima_default_rules <-> List entry 1 <-> List entry 2 <-> ... <-> List entry 0
>>>                                                                      /\
>>>                                                                  first (now useless)
>>> ima_rules        
>>>      |
>>>      |
>>>      |
>>>      ---------------
>>>                    \/
>>> policy --> policy entry 0' (head node) = ima_policy_rules <-> policy entry 1' <-> policy entry 2' <-> .... <-> policy entry 0'
>>>
>>> The goal is that readers should still be able to loop
>>> (forward, as we saw that backward looping is temporarily broken)
>>> while in steps 0-4.
>>>
>>> I'm not completely sure what would happen to a client that started iterating
>>> over ima_rules right after step 2.
>>>
>>> Wouldn't they be able to start looping through the new policy
>>> as 'List entry 0 (head node) = ima_default_rules' points to ima_policy_rules?
>>> And if they, wouldn't they loop until the write to 'ima_rule' at step 3 (admittedly
>>> very shortly thereafter) completed?
>>> And would the compiler be allowed to optimize the read to 'ima_rules' in the
>>> list_for_each_entry() loop, thereby never reloading the new value for
>>> 'ima_rules', and thus looping forever, just what we are trying to avoid?
>>>
>>> Overall, I'm tempted to say this is perhaps a bit too complex (at least,
>>> my head tells me it is, but that may very well be because I'm terrible
>>> at concurrency issues).
>>>
>>> Honestly, in this case I think awaiting input from more experienced
>>> kernel devs than I is the best path forward :-)
>> I'm far from an expert on RCU locking, but __list_splice_init_rcu()
>> provides an example of how to make sure there aren't any readers
>> traversing the list, before two lists are spliced together.   In our
>> case, after there aren't any readers, instead of splicing two lists
>> together, it should be safe to point to the new list.
>>
>> thanks,
>>
>> Mimi
>>

Re: [PATCH] ima: fix infinite loop within "ima_match_policy" function.

[liqiong]

Hi Simon :

Using a temporary ima_rules variable is not working for "ima_policy_next". 

 void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos)
 {
 	struct ima_rule_entry *entry = v;
-
+	struct list_head *ima_rules_tmp = rcu_dereference(ima_rules);
 	rcu_read_lock();
 	entry = list_entry_rcu(entry->list.next, struct ima_rule_entry, list);
 	rcu_read_unlock();
 	(*pos)++;
 
-	return (&entry->list == ima_rules) ? NULL : entry;
+	return (&entry->list == ima_rules_tmp) ? NULL : entry;
 }

It seems no way to fix "ima_rules" change within this function, it will alway
return a entry if "ima_rules" being changed.

Regrads,

liqiong



在 2021年08月23日 11:04, 李力琼 写道:
> Hi Mimi :
>
> The situation is a little different,'list_splice_init_rcu'
> don't change the list head. If "ima_rules" being changed,
> readers may can't reload the new value in time for cpu cache
> or compiler optimization. Defining "ima_rules" as a volatile 
> variable can fix, but It is inefficient.
>
> Maybe using a temporary ima_rules variable for every 
> "list_for_each_entry_rcu(entry, ima_rules, list)" loop is 
> a better solution to fix the "endless loop" bug. 
>
> Regards,
>
> liqiong
>
> 在 2021年08月20日 23:48, Mimi Zohar 写道:
>> On Fri, 2021-08-20 at 13:23 +0000, THOBY Simon wrote:
>>> Hi Liqiong,
>>>
>>> On 8/20/21 12:15 PM, 李力琼 wrote:
>>>> Hi, Simon:
>>>>
>>>> This solution is better then rwsem, a temp "ima_rules" variable should 
>>>> can fix. I also have a another idea, with a little trick, default list
>>>> can traverse to the new list, so we don't need care about the read side. 
>>>>
>>>> here is the patch:
>>>>
>>>> @@ -918,8 +918,21 @@ void ima_update_policy(void)
>>>>         list_splice_tail_init_rcu(&ima_temp_rules, policy, synchronize_rcu);
>>>>
>>>>         if (ima_rules != policy) {
>>>> +               struct list_head *prev_rules = ima_rules;
>>>> +               struct list_head *first = ima_rules->next;
>>>>                 ima_policy_flag = 0;
>>>> +
>>>> +               /*
>>>> +                * Make the previous list can traverse to new list,
>>>> +                * that is tricky, or there is a deadly loop whithin
>>>> +                * "list_for_each_entry_rcu(entry, ima_rules, list)"
>>>> +                *
>>>> +                * After update "ima_rules", restore the previous list.
>>>> +                */
>>> I think this could be rephrased to be a tad clearer, I am not quite sure
>>> how I must interpret the first sentence of the comment.
>>>
>>>
>>>> +               prev_rules->next = policy->next;
>>>>                 ima_rules = policy;
>>>> +               syncchronize_rcu();
>>> I'm a bit puzzled as you seem to imply in the mail this patch was tested,
>>> but there is no 'syncchronize_rcu' (with two 'c') symbol in the kernel.
>>> Was that a copy/paste error? Or maybe you forgot the 'not' in "This
>>> patch has been tested"? These errors happen, and I am myself quite an
>>> expert in doing them :)
>>>
>>>> +               prev_rules->next = first;
>>>>
>>>>
>>>> The side effect is the "ima_default_rules" will be changed a little while.
>>>> But it make sense, the process should be checked again by the new policy.
>>>>
>>>> This patch has been tested, if will do, I can resubmit this patch.> 
>>>> How about this ?
>>> least
>>>
>>> Correct me if I'm wrong, here is how I think I understand you patch.
>>> We start with a situation like that (step 0):
>>> ima_rules --> List entry 0 (head node) = ima_default_rules <-> List entry 1 <-> List entry 2 <-> ... <-> List entry 0
>>>
>>> Then we decide to update the policy for the first time, so
>>> 'ima_rules [&ima_default_rules] != policy [&ima_policy_rules]'.
>>> We enter the condition.
>>> First we copy the current value of ima_rules (&ima_default_rules)
>>> to a temporary variable 'prev_rules'. We also create a pointer dubbed
>>> 'first' to the entry 1 in the default list (step 1):
>>> prev_rules -------------
>>>                        \/
>>> ima_rules --> List entry 0 (head node) = ima_default_rules <-> List entry 1 <-> List entry 2 <-> ... <-> List entry 0
>>>                                                                    /\
>>> first --------------------------------------------------------------
>>>
>>>
>>> Then we update prev_rules->next to point to policy->next (step 2):
>>> List entry 1 <-> List entry 2 <-> ... -> List entry 0
>>>  /\
>>> first
>>> 	(notice that list entry 0 no longer points backwards to 'list entry 1',
>>> 	but I don't think there is any reverse iteration in IMA, so it should be
>>> 	safe)
>>>
>>> prev_rules -------------
>>>                        \/
>>> ima_rules --> List entry 0 (head node) = ima_default_rules   
>>>                        |
>>>                        |
>>>                        -------------------------------------------
>>>                                                                  \/
>>> policy --> policy entry 0' (head node) = ima_policy_rules <-> policy entry 1' <-> policy entry 2' <-> .... <-> policy entry 0'
>>>
>>>
>>> We then update ima_rules to point to ima_policy_rules (step 3):
>>> List entry 1 <-> List entry 2 <-> ... -> List entry 0
>>>  /\
>>> first
>>>
>>> prev_rules -------------
>>>                        \/
>>> ima_rules     List entry 0 (head node) = ima_default_rules   
>>>      |                 |
>>>      |                 |
>>>      |                 ------------------------------------------
>>>      ---------------                                            |
>>>                    \/                                           \/
>>> policy --> policy entry 0' (head node) = ima_policy_rules <-> policy entry 1' <-> policy entry 2' <-> .... <-> policy entry 0'
>>>                                                   synchronize_rcu                 /\
>>> first --------------------------------------------------------------
>>>
>>> Then we run synchronize_rcu() to wait for any RCU reader to exit their loops (step 4).
>>>
>>> Finally we update prev_rules->next to point back to the ima policy and fix the loop (step 5):
>>>
>>> List entry 1 <-> List entry 2 <-> ... -> List entry 0
>>>  /\
>>> first
>>>
>>> prev_rules ---> List entry 0 (head node) = ima_default_rules <-> List entry 1 <-> List entry 2 <-> ... <-> List entry 0
>>>                                                                      /\
>>>                                                                  first (now useless)
>>> ima_rules        
>>>      |
>>>      |
>>>      |
>>>      ---------------
>>>                    \/
>>> policy --> policy entry 0' (head node) = ima_policy_rules <-> policy entry 1' <-> policy entry 2' <-> .... <-> policy entry 0'
>>>
>>> The goal is that readers should still be able to loop
>>> (forward, as we saw that backward looping is temporarily broken)
>>> while in steps 0-4.
>>>
>>> I'm not completely sure what would happen to a client that started iterating
>>> over ima_rules right after step 2.
>>>
>>> Wouldn't they be able to start looping through the new policy
>>> as 'List entry 0 (head node) = ima_default_rules' points to ima_policy_rules?
>>> And if they, wouldn't they loop until the write to 'ima_rule' at step 3 (admittedly
>>> very shortly thereafter) completed?
>>> And would the compiler be allowed to optimize the read to 'ima_rules' in the
>>> list_for_each_entry() loop, thereby never reloading the new value for
>>> 'ima_rules', and thus looping forever, just what we are trying to avoid?
>>>
>>> Overall, I'm tempted to say this is perhaps a bit too complex (at least,
>>> my head tells me it is, but that may very well be because I'm terrible
>>> at concurrency issues).
>>>
>>> Honestly, in this case I think awaiting input from more experienced
>>> kernel devs than I is the best path forward :-)
>> I'm far from an expert on RCU locking, but __list_splice_init_rcu()
>> provides an example of how to make sure there aren't any readers
>> traversing the list, before two lists are spliced together.   In our
>> case, after there aren't any readers, instead of splicing two lists
>> together, it should be safe to point to the new list.
>>
>> thanks,
>>
>> Mimi
>>

Re: [PATCH] ima: fix infinite loop within "ima_match_policy" function.

[THOBY Simon]

Hi Liqiong,

On 8/23/21 10:06 AM, liqiong wrote:
> Hi Simon :
> 
> Using a temporary ima_rules variable is not working for "ima_policy_next". 
> 
>  void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos)
>  {
>  	struct ima_rule_entry *entry = v;
> -
> +	struct list_head *ima_rules_tmp = rcu_dereference(ima_rules);
>  	rcu_read_lock();
>  	entry = list_entry_rcu(entry->list.next, struct ima_rule_entry, list);
>  	rcu_read_unlock();
>  	(*pos)++;
>  
> -	return (&entry->list == ima_rules) ? NULL : entry;
> +	return (&entry->list == ima_rules_tmp) ? NULL : entry;
>  }
> 
> It seems no way to fix "ima_rules" change within this function, it will alway
> return a entry if "ima_rules" being changed.

- I think rcu_dereference() should be called inside the RCU read lock
- Maybe we could cheat with:
	return (&entry->list == &ima_policy_rules || &entry->list == &ima_default_rules) ? NULL : entry;
  as that's the only two rulesets IMA ever use?
  Admittedly, this is not as clean as previously, but it should work too.

The way I see it, the semaphore solution would not work here either,
as ima_policy_next() is called repeatedly as a seq_file
(it is set up in ima_fs.c) and we can't control the locking there:
we cannot lock across the seq_read() call (that cure could end up be
worse than the disease, deadlock-wise), so I fear we cannot protect
against a list update while a user is iterating with a lock.

So in both cases a cheat like "&entry->list == &ima_policy_rules || &entry->list == &ima_default_rules"
maybe need to be considered.

What do you think?


> 
> Regrads,
> 
> liqiong

Thanks,
Simon

Re: [PATCH] ima: fix infinite loop within "ima_match_policy" function.

[Mimi Zohar]

Hi Liqiong,

On Mon, 2021-08-23 at 11:04 +0800, 李力琼 wrote:
> Hi Mimi :
> 
> The situation is a little different,'list_splice_init_rcu'
> don't change the list head. If "ima_rules" being changed,
> readers may can't reload the new value in time for cpu cache
> or compiler optimization. Defining "ima_rules" as a volatile 
> variable can fix, but It is inefficient.

After looking at list_splice_tail_init_rcu() some more, it is
actually making sure there aren't any readers traversing
"ima_temp_rules", not "ima_policy_rules".   There aren't any readers
traversing "ima_temp_rules". 

> 
> Maybe using a temporary ima_rules variable for every 
> "list_for_each_entry_rcu(entry, ima_rules, list)" loop is 
> a better solution to fix the "endless loop" bug. 

Agreed

thanks,

Mimi

Re: [PATCH] ima: fix infinite loop within "ima_match_policy" function.

[Mimi Zohar]

On Mon, 2021-08-23 at 08:14 +0000, THOBY Simon wrote:
> Hi Liqiong,
> 
> On 8/23/21 10:06 AM, liqiong wrote:
> > Hi Simon :
> > 
> > Using a temporary ima_rules variable is not working for "ima_policy_next". 
> > 
> >  void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos)
> >  {
> >  	struct ima_rule_entry *entry = v;
> > -
> > +	struct list_head *ima_rules_tmp = rcu_dereference(ima_rules);
> >  	rcu_read_lock();
> >  	entry = list_entry_rcu(entry->list.next, struct ima_rule_entry, list);
> >  	rcu_read_unlock();
> >  	(*pos)++;
> >  
> > -	return (&entry->list == ima_rules) ? NULL : entry;
> > +	return (&entry->list == ima_rules_tmp) ? NULL : entry;
> >  }
> > 
> > It seems no way to fix "ima_rules" change within this function, it will alway
> > return a entry if "ima_rules" being changed.
> 
> - I think rcu_dereference() should be called inside the RCU read lock
> - Maybe we could cheat with:
> 	return (&entry->list == &ima_policy_rules || &entry->list == &ima_default_rules) ? NULL : entry;
>   as that's the only two rulesets IMA ever use?
>   Admittedly, this is not as clean as previously, but it should work too.
> 
> The way I see it, the semaphore solution would not work here either,
> as ima_policy_next() is called repeatedly as a seq_file
> (it is set up in ima_fs.c) and we can't control the locking there:
> we cannot lock across the seq_read() call (that cure could end up be
> worse than the disease, deadlock-wise), so I fear we cannot protect
> against a list update while a user is iterating with a lock.
> 
> So in both cases a cheat like "&entry->list == &ima_policy_rules || &entry->list == &ima_default_rules"
> maybe need to be considered.
> 
> What do you think?

Is this an overall suggestion or limited to just ima_policy_next()?

thanks,

Mimi

Re: [PATCH] ima: fix infinite loop within "ima_match_policy" function.

[THOBY Simon]

Hi Mimi,

On 8/23/21 1:57 PM, Mimi Zohar wrote:
> On Mon, 2021-08-23 at 08:14 +0000, THOBY Simon wrote:
>> Hi Liqiong,
>>
>> On 8/23/21 10:06 AM, liqiong wrote:
>>> Hi Simon :
>>>
>>> Using a temporary ima_rules variable is not working for "ima_policy_next". 
>>>
>>>  void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos)
>>>  {
>>>  	struct ima_rule_entry *entry = v;
>>> -
>>> +	struct list_head *ima_rules_tmp = rcu_dereference(ima_rules);
>>>  	rcu_read_lock();
>>>  	entry = list_entry_rcu(entry->list.next, struct ima_rule_entry, list);
>>>  	rcu_read_unlock();
>>>  	(*pos)++;
>>>  
>>> -	return (&entry->list == ima_rules) ? NULL : entry;
>>> +	return (&entry->list == ima_rules_tmp) ? NULL : entry;
>>>  }
>>>
>>> It seems no way to fix "ima_rules" change within this function, it will alway
>>> return a entry if "ima_rules" being changed.
>>
>> - I think rcu_dereference() should be called inside the RCU read lock
>> - Maybe we could cheat with:
>> 	return (&entry->list == &ima_policy_rules || &entry->list == &ima_default_rules) ? NULL : entry;
>>   as that's the only two rulesets IMA ever use?
>>   Admittedly, this is not as clean as previously, but it should work too.
>>
>> The way I see it, the semaphore solution would not work here either,
>> as ima_policy_next() is called repeatedly as a seq_file
>> (it is set up in ima_fs.c) and we can't control the locking there:
>> we cannot lock across the seq_read() call (that cure could end up be
>> worse than the disease, deadlock-wise), so I fear we cannot protect
>> against a list update while a user is iterating with a lock.
>>
>> So in both cases a cheat like "&entry->list == &ima_policy_rules || &entry->list == &ima_default_rules"
>> maybe need to be considered.
>>
>> What do you think?
> 
> Is this an overall suggestion or limited to just ima_policy_next()?

I was thinking only of ima_policy_next(), I don't think (from what I could see in a short glance)
that other functions need such a treatment. The ima_rules_tmp dance is probably safe for the
other uses of ima_rules.

> 
> thanks,
> 
> Mimi
> 
> 

Thanks,
Simon

Re: [PATCH] ima: fix infinite loop within "ima_match_policy" function.

[Mimi Zohar]

Hi Simon,

On Mon, 2021-08-23 at 12:02 +0000, THOBY Simon wrote:
> Hi Mimi,
> 
> On 8/23/21 1:57 PM, Mimi Zohar wrote:
> > On Mon, 2021-08-23 at 08:14 +0000, THOBY Simon wrote:
> >> Hi Liqiong,
> >>
> >> On 8/23/21 10:06 AM, liqiong wrote:
> >>> Hi Simon :
> >>>
> >>> Using a temporary ima_rules variable is not working for "ima_policy_next". 
> >>>
> >>>  void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos)
> >>>  {
> >>>  	struct ima_rule_entry *entry = v;
> >>> -
> >>> +	struct list_head *ima_rules_tmp = rcu_dereference(ima_rules);
> >>>  	rcu_read_lock();
> >>>  	entry = list_entry_rcu(entry->list.next, struct ima_rule_entry, list);
> >>>  	rcu_read_unlock();
> >>>  	(*pos)++;
> >>>  
> >>> -	return (&entry->list == ima_rules) ? NULL : entry;
> >>> +	return (&entry->list == ima_rules_tmp) ? NULL : entry;
> >>>  }
> >>>
> >>> It seems no way to fix "ima_rules" change within this function, it will alway
> >>> return a entry if "ima_rules" being changed.
> >>
> >> - I think rcu_dereference() should be called inside the RCU read lock
> >> - Maybe we could cheat with:
> >> 	return (&entry->list == &ima_policy_rules || &entry->list == &ima_default_rules) ? NULL : entry;
> >>   as that's the only two rulesets IMA ever use?
> >>   Admittedly, this is not as clean as previously, but it should work too.
> >>
> >> The way I see it, the semaphore solution would not work here either,
> >> as ima_policy_next() is called repeatedly as a seq_file
> >> (it is set up in ima_fs.c) and we can't control the locking there:
> >> we cannot lock across the seq_read() call (that cure could end up be
> >> worse than the disease, deadlock-wise), so I fear we cannot protect
> >> against a list update while a user is iterating with a lock.
> >>
> >> So in both cases a cheat like "&entry->list == &ima_policy_rules || &entry->list == &ima_default_rules"
> >> maybe need to be considered.
> >>
> >> What do you think?
> > 
> > Is this an overall suggestion or limited to just ima_policy_next()?
> 
> I was thinking only of ima_policy_next(), I don't think (from what I could see in a short glance)
> that other functions need such a treatment. The ima_rules_tmp dance is probably safe for the
> other uses of ima_rules.

Thanks, just making sure it is limited to here.

Mimi

Re: [PATCH] ima: fix infinite loop within "ima_match_policy" function.

[liqiong]

Hi Simon :

在 2021年08月23日 16:14, THOBY Simon 写道:
> Hi Liqiong,
>
> On 8/23/21 10:06 AM, liqiong wrote:
>> Hi Simon :
>>
>> Using a temporary ima_rules variable is not working for "ima_policy_next". 
>>
>>  void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos)
>>  {
>>  	struct ima_rule_entry *entry = v;
>> -
>> +	struct list_head *ima_rules_tmp = rcu_dereference(ima_rules);
>>  	rcu_read_lock();
>>  	entry = list_entry_rcu(entry->list.next, struct ima_rule_entry, list);
>>  	rcu_read_unlock();
>>  	(*pos)++;
>>  
>> -	return (&entry->list == ima_rules) ? NULL : entry;
>> +	return (&entry->list == ima_rules_tmp) ? NULL : entry;
>>  }
>>
>> It seems no way to fix "ima_rules" change within this function, it will alway
>> return a entry if "ima_rules" being changed.
> - I think rcu_dereference() should be called inside the RCU read lock
> - Maybe we could cheat with:
> 	return (&entry->list == &ima_policy_rules || &entry->list == &ima_default_rules) ? NULL : entry;
>   as that's the only two rulesets IMA ever use?
>   Admittedly, this is not as clean as previously, but it should work too.
>
> The way I see it, the semaphore solution would not work here either,
> as ima_policy_next() is called repeatedly as a seq_file
> (it is set up in ima_fs.c) and we can't control the locking there:
> we cannot lock across the seq_read() call (that cure could end up be
> worse than the disease, deadlock-wise), so I fear we cannot protect
> against a list update while a user is iterating with a lock.
>
> So in both cases a cheat like "&entry->list == &ima_policy_rules || &entry->list == &ima_default_rules"
> maybe need to be considered.
>
> What do you think?

Yes,  semaphore solution not work here,  splicing two list is a little complex.
This solution is  simple and clear,  should  work.  I will work on that, test and 
confirm  the patch. 

"rcu_dereference() should be called inside the RCU read lock", I will correct this.

Thanks for your help.


Regrads,

liqiong

>
>
>> Regrads,
>>
>> liqiong
> Thanks,
> Simon

[PATCH] ima: fix deadlock within "ima_match_policy" function.

[liqiong]

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(-)

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;
 
-- 
2.11.0

Re: [PATCH] ima: fix deadlock within "ima_match_policy" function.

[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

Re: [PATCH] ima: fix deadlock within "ima_match_policy" function.

[liqiong]

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

Re: [PATCH] ima: fix deadlock within "ima_match_policy" function.

[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

Re: [PATCH] ima: fix deadlock within RCU list of ima_rules

[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
>
>

Re: [PATCH] ima: fix deadlock within RCU list of ima_rules

[liqiong]

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
>>
>>
>

Re: [PATCH] ima: fix deadlock within RCU list of ima_rules

[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

Re: [PATCH] ima: fix deadlock within RCU list of ima_rules

[liqiong]

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

Re: [PATCH] ima: fix deadlock within RCU list of ima_rules

[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

Re: [PATCH] ima: fix deadlock within RCU list of ima_rules

[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

Re: [PATCH] ima: fix deadlock within RCU list of ima_rules

[THOBY Simon]

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

Re: [PATCH] ima: fix deadlock within RCU list of ima_rules

[liqiong]

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;
 

-- 
2.11.0


在 2021年08月27日 15:30, THOBY Simon 写道:

> 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

Re: [PATCH] ima: fix deadlock within RCU list of ima_rules

[THOBY Simon]

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>

[PATCH] ima: fix deadlock when traversing "ima_default_rules".

[liqiong]

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>
Reviewed-by: THOBY Simon <Simon.THOBY@viveris.fr>
---
 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;
 
-- 
2.11.0

Re: [PATCH] ima: fix deadlock when traversing "ima_default_rules".

[Mimi Zohar]

On Fri, 2021-08-27 at 18:35 +0800, liqiong wrote:
> 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>
> Reviewed-by: THOBY Simon <Simon.THOBY@viveris.fr>

Thank you, Liqiong, Simon.   This patch set is now queued in the next-
integrity-testing 
branch.

Mimi

[PATCH] ima: fix deadlock when traversing "ima_default_rules".

[liqiong]

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>
Reviewed-by: THOBY Simon <Simon.THOBY@viveris.fr>
Fixes: 38d859f991f3 ("IMA: policy can now be updated multiple times")
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
Reported-by: kernel test robot <lkp@intel.com>
Fix sparse: incompatible types in comparison expression.
---
 security/integrity/ima/ima_policy.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 87b9b71cb820..480de75eaf8c 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -228,7 +228,7 @@ static struct ima_rule_entry *arch_policy_entry __ro_after_init;
 static LIST_HEAD(ima_default_rules);
 static LIST_HEAD(ima_policy_rules);
 static LIST_HEAD(ima_temp_rules);
-static struct list_head *ima_rules = &ima_default_rules;
+static struct list_head __rcu *ima_rules = (struct list_head __rcu *)(&ima_default_rules);
 
 static int ima_policy __initdata;
 
@@ -675,12 +675,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;
@@ -970,8 +972,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
@@ -1768,9 +1770,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;
@@ -1789,7 +1793,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)
@@ -2014,6 +2019,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;
@@ -2021,7 +2027,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;
 
-- 
2.11.0

Re: [PATCH] ima: fix deadlock when traversing "ima_default_rules".

[Mimi Zohar]

Hi Liqiong,

On Sat, 2021-09-18 at 11:11 +0800, liqiong wrote:
> 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>
> Reviewed-by: THOBY Simon <Simon.THOBY@viveris.fr>
> Fixes: 38d859f991f3 ("IMA: policy can now be updated multiple times")
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> Reported-by: kernel test robot <lkp@intel.com>
> Fix sparse: incompatible types in comparison expression.

The "Fix sparse" line shouldn't be on a separate line.  Either post the
one line fix as a separate patch using the normal "Fixes:" tag or fix
the "Reported-by" line, as previously suggested.

thanks,

Mimi

[PATCH] ima: fix deadlock when traversing "ima_default_rules".

[liqiong]

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>
Reviewed-by: THOBY Simon <Simon.THOBY@viveris.fr>
Fixes: 38d859f991f3 ("IMA: policy can now be updated multiple times")
Reported-by: kernel test robot <lkp@intel.com> (Fix sparse: incompatible types in comparison expression.)
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 security/integrity/ima/ima_policy.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 87b9b71cb820..12e8adcd80a2 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -228,7 +228,7 @@ static struct ima_rule_entry *arch_policy_entry __ro_after_init;
 static LIST_HEAD(ima_default_rules);
 static LIST_HEAD(ima_policy_rules);
 static LIST_HEAD(ima_temp_rules);
-static struct list_head *ima_rules = &ima_default_rules;
+static struct list_head __rcu *ima_rules = (struct list_head __rcu *)(&ima_default_rules);
 
 static int ima_policy __initdata;
 
@@ -675,12 +675,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;
@@ -741,9 +743,11 @@ void ima_update_policy_flags(void)
 {
 	struct ima_rule_entry *entry;
 	int new_policy_flag = 0;
+	struct list_head *ima_rules_tmp;
 
 	rcu_read_lock();
-	list_for_each_entry(entry, ima_rules, list) {
+	ima_rules_tmp = rcu_dereference(ima_rules);
+	list_for_each_entry_rcu(entry, ima_rules_tmp, list) {
 		/*
 		 * SETXATTR_CHECK rules do not implement a full policy check
 		 * because rule checking would probably have an important
@@ -968,10 +972,10 @@ void ima_update_policy(void)
 
 	list_splice_tail_init_rcu(&ima_temp_rules, policy, synchronize_rcu);
 
-	if (ima_rules != policy) {
+	if (ima_rules != (struct list_head __rcu *)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
@@ -1061,7 +1065,7 @@ static int ima_lsm_rule_init(struct ima_rule_entry *entry,
 		pr_warn("rule for LSM \'%s\' is undefined\n",
 			entry->lsm[lsm_rule].args_p);
 
-		if (ima_rules == &ima_default_rules) {
+		if (ima_rules == (struct list_head __rcu *)(&ima_default_rules)) {
 			kfree(entry->lsm[lsm_rule].args_p);
 			entry->lsm[lsm_rule].args_p = NULL;
 			result = -EINVAL;
@@ -1768,9 +1772,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;
@@ -1789,7 +1795,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)
@@ -2014,6 +2021,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;
@@ -2021,7 +2029,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;
 
-- 
2.25.1