[PATCH v5 2/2] ima: Handle -ESTALE returned by ima_filter_rule_match()

[Guozihua (Scott)]

On 2022/9/23 19:19, Mimi Zohar wrote:
> On Fri, 2022-09-23 at 12:01 +0800, Guozihua (Scott) wrote:
>> On 2022/9/22 19:09, Mimi Zohar wrote:
>>> Hi Scott,
>>>
>>> On Wed, 2022-09-21 at 20:58 +0800, GUO Zihua wrote:
>>>>                   }
>>>> -               if (!rc)
>>>> -                       return false;
>>>> +
>>>> +               if (rc == -ESTALE && !rule_reinitialized) {
>>>
>>> Ok, this limits allocating ima_lsm_copy_rule() to the first -ESTALE,
>>>
>>>> +                       lsm_rule = ima_lsm_copy_rule(rule);
>>>> +                       if (lsm_rule) {
>>>> +                               rule_reinitialized = true;
>>>> +                               goto retry;
>>>
>>> but "retry" is also limited to the first -ESTALE.
>>
>> Technically we would only need one retry. This loop is looping on all
>> the lsm members of one rule, and ima_lsm_copy_rule would update all the
>> lsm members of this rule. The "lsm member" here refers to LSM defined
>> properties like obj_user, obj_role etc. These members are of AND
>> relation, meaning all lsm members together would form one LSM rule.
>>
>> As of the scenario you mentioned, I think it should be really rare.
>> Spending to much time and code on this might not worth it.
>>>
>>>> +                       }
>>>> +               }
> 
> Either there can be multiple LSM fields and the memory is allocated
> once and freed once at the end, as you suggested, or the memory should
> be freed here and rule_reinitialized reset, minimizing the code change.

I might have overlooked something, but if I understands the code 
correctly, we would be copying the same rule over and over again till 
the loop ends in that case. ima_lsm_update_rule() would replace the rule 
node on the rule list without updating the rule in place. Although 
synchronize_rcu() should prevent a UAF, the rule in ima_match_rules() 
would not be updated. Meaning SELinux would always return -ESTALE before 
we copy and retry as we keep passing in the outdated rule entry.
> 
>>>> +               if (!rc) {
>>>> +                       result = false;
>>>> +                       goto out;
>>>> +               }
>>>>           }
>>>> -       return true;
>>>> +       result = true;
>>>> +
>>>> +out:
>>>> +       if (rule_reinitialized) {
>>>> +               for (i = 0; i < MAX_LSM_RULES; i++)
>>>> +                       ima_filter_rule_free(lsm_rule->lsm[i].rule);
>>>> +               kfree(lsm_rule);
>>>> +       }
>>>> +       return result;
>>>>    }
> 


-- 
Best
GUO Zihua