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

Mimi Zohar
2022-12-14T12:23:05Z

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?

thanks,

Mimi