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@xxxxxxxxxx/ > >>>>>> > >>>>>> 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?
