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