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

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

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?

thanks,

Mimi