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? thanks, Mimi
