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

Mimi Zohar
2022-12-13T15:30:35Z

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