Re: [RFC] IMA LSM based rule race condition issue on 4.19 LTS

[Mimi Zohar <zohar@linux.ibm.com>]

On Thu, 2022-12-15 at 21:15 +0800, Guozihua (Scott) wrote:
> On 2022/12/15 18:49, Mimi Zohar wrote:
> > On Thu, 2022-12-15 at 16:51 +0800, Guozihua (Scott) wrote:
> >> On 2022/12/14 20:19, Mimi Zohar wrote:
> >>> On Wed, 2022-12-14 at 09:33 +0800, Guozihua (Scott) wrote:
> >>>> On 2022/12/13 23:30, Mimi Zohar wrote:
> >>>>> On Fri, 2022-12-09 at 15:00 +0800, Guozihua (Scott) wrote:
> >>>>>> Hi community.
> >>>>>>
> >>>>>> Previously our team reported a race condition in IMA relates to LSM 
> >>>>>> based rules which would case IMA to match files that should be filtered 
> >>>>>> out under normal condition. The issue was originally analyzed and fixed 
> >>>>>> on mainstream. The patch and the discussion could be found here: 
> >>>>>> https://lore.kernel.org/all/20220921125804.59490-1-guozihua@huawei.com/
> >>>>>>
> >>>>>> After that, we did a regression test on 4.19 LTS and the same issue 
> >>>>>> arises. Further analysis reveled that the issue is from a completely 
> >>>>>> different cause.
> >>>>>>
> >>>>>> The cause is that selinux_audit_rule_init() would set the rule (which is 
> >>>>>> a second level pointer) to NULL immediately after called. The relevant 
> >>>>>> codes are as shown:
> >>>>>>
> >>>>>> security/selinux/ss/services.c:
> >>>>>>> int selinux_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule)
> >>>>>>> {
> >>>>>>>         struct selinux_state *state = &selinux_state;
> >>>>>>>         struct policydb *policydb = &state->ss->policydb;
> >>>>>>>         struct selinux_audit_rule *tmprule;
> >>>>>>>         struct role_datum *roledatum;
> >>>>>>>         struct type_datum *typedatum;
> >>>>>>>         struct user_datum *userdatum;
> >>>>>>>         struct selinux_audit_rule **rule = (struct selinux_audit_rule **)vrule;
> >>>>>>>         int rc = 0;
> >>>>>>>
> >>>>>>>         *rule = NULL;
> >>>>>> *rule is set to NULL here, which means the rule on IMA side is also NULL.
> >>>>>>>
> >>>>>>>         if (!state->initialized)
> >>>>>>>                 return -EOPNOTSUPP;
> >>>>>> ...
> >>>>>>> out:
> >>>>>>>         read_unlock(&state->ss->policy_rwlock);
> >>>>>>>
> >>>>>>>         if (rc) {
> >>>>>>>                 selinux_audit_rule_free(tmprule);
> >>>>>>>                 tmprule = NULL;
> >>>>>>>         }
> >>>>>>>
> >>>>>>>         *rule = tmprule;
> >>>>>> rule is updated at the end of the function.
> >>>>>>>
> >>>>>>>         return rc;
> >>>>>>> }
> >>>>>>
> >>>>>> security/integrity/ima/ima_policy.c:
> >>>>>>> static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
> >>>>>>>                             const struct cred *cred, u32 secid,
> >>>>>>>                             enum ima_hooks func, int mask)
> >>>>>>> {...
> >>>>>>> for (i = 0; i < MAX_LSM_RULES; i++) {
> >>>>>>>                 int rc = 0;
> >>>>>>>                 u32 osid;
> >>>>>>>                 int retried = 0;
> >>>>>>>
> >>>>>>>                 if (!rule->lsm[i].rule)
> >>>>>>>                         continue;
> >>>>>> Setting rule to NULL would lead to LSM based rule matching being skipped.
> >>>>>>> retry:
> >>>>>>>                 switch (i) {
> >>>>>>
> >>>>>> To solve this issue, there are multiple approaches we might take and I 
> >>>>>> would like some input from the community.
> >>>>>>
> >>>>>> The first proposed solution would be to change 
> >>>>>> selinux_audit_rule_init(). Remove the set to NULL bit and update the 
> >>>>>> rule pointer with cmpxchg.
> >>>>>>
> >>>>>>> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> >>>>>>> index a9f2bc8443bd..aa74b04ccaf7 100644
> >>>>>>> --- a/security/selinux/ss/services.c
> >>>>>>> +++ b/security/selinux/ss/services.c
> >>>>>>> @@ -3297,10 +3297,9 @@ int selinux_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule)
> >>>>>>>         struct type_datum *typedatum;
> >>>>>>>         struct user_datum *userdatum;
> >>>>>>>         struct selinux_audit_rule **rule = (struct selinux_audit_rule **)vrule;
> >>>>>>> +       struct selinux_audit_rule *orig = rule;
> >>>>>>>         int rc = 0;
> >>>>>>>  
> >>>>>>> -       *rule = NULL;
> >>>>>>> -
> >>>>>>>         if (!state->initialized)
> >>>>>>>                 return -EOPNOTSUPP;
> >>>>>>>  
> >>>>>>> @@ -3382,7 +3381,8 @@ int selinux_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule)
> >>>>>>>                 tmprule = NULL;
> >>>>>>>         }
> >>>>>>>  
> >>>>>>> -       *rule = tmprule;
> >>>>>>> +       if (cmpxchg(rule, orig, tmprule) != orig)
> >>>>>>> +               selinux_audit_rule_free(tmprule);
> >>>>>>>  
> >>>>>>>         return rc;
> >>>>>>>  }
> >>>>>>
> >>>>>> This solution would be an easy fix, but might influence other modules 
> >>>>>> calling selinux_audit_rule_init() directly or indirectly (on 4.19 LTS, 
> >>>>>> only auditfilter and IMA it seems). And it might be worth returning an 
> >>>>>> error code such as -EAGAIN.
> >>>>>>
> >>>>>> Or, we can access rules via RCU, similar to what we do on 5.10. This 
> >>>>>> could means more code change and testing.
> >>>>>
> >>>>> In the 4.19 kernel, IMA is doing a lazy LSM based policy rule update as
> >>>>> needed.  IMA waits for selinux_audit_rule_init() to complete and
> >>>>> shouldn't see NULL, unless there is an SELinux failure.  Before
> >>>>> "fixing" the problem, what exactly is the problem?
> >>>>
> >>>> IMA runs on multiple cores. On 4.19 kernel, IMA do a lazy update on ALL
> >>>> LSM based rules in one go without using RCU, which would still allow
> >>>> other cores to access the rule being updated. And that's the issue.
> >>>>
> >>>> An example scenario would be:
> >>>> 	CPU1			|	CPU2
> >>>> opened a file and starts	|
> >>>> updating LSM based rules.	|
> >>>> 				| opened a file and starts
> >>>> 				| matching rules.
> >>>> 				|
> >>>> set a LSM based rule to NULL.	| access the same LSM based rule and
> >>>>  				| see that it's NULL.
> >>>>
> >>>> In this situation, CPU 2 would recognize this rule as not LSM based and
> >>>> ignore the LSM part of the rule while matching.
> >>>
> >>> Would picking up just ima_lsm_update_rule(), without changing to the
> >>> lsm policy update notifier, from upstream and calling it from
> >>> ima_lsm_update_rules() resolve the RCU locking issue?  Or are there
> >>> other issues?
> >>
> >> Hi Mimi,
> >>
> >> That should resolve the issue just fine. However, that'd mean having a
> >> customized ima_lsm_update_rules as well as a customized
> >> ima_lsm_update_rule functions on 4.19.y, potentially decrease the
> >> maintainability. The customization of the two functions mentioned above
> >> would be to ripoff the RCU part so that we can leave the other rule-list
> >> access untouched.
> > 
> > Hi Scott,
> > 
> > Neither do we want to backport new functionality.  Perhaps it is only a
> > subset of commit b16942455193 ("ima: use the lsm policy update
> > notifier").
> Yes we are able to backport part of this commit to get a mainline-like
> behavior which would solve the issue at hand as well. However from a
> maintenance standpoint it might be better to form a different commit and
> not re-use the commit message from mainline commit.

I assume that is fine, but cherry-pick the original commit with the -x
option, so there is a correlation to the upstream commit.  The patch
description would mention that the patch is a partial backport.

thanks,

Mimi

> > 
> > Although your suggested solution of using "cmpxchg" isn't needed in
> > recent kernels,  it wouldn't hurt either, right?  Assuming that Paul
> > would be willing to accept it, that could be another option.
> The cmpxchg part is merely to avoid multiple updates on the same rule
> item. However I am not so sure why SELinux would set the rule to NULL.
> My guess is that SELinux is trying to stop others from using an
> invalidated rule?
> 
> Would Paul be able to provide some insight? as well as some Comment on
> the proposed solution?