On 2022/9/23 19:19, Mimi Zohar wrote:
On Fri, 2022-09-23 at 12:01 +0800, Guozihua (Scott) wrote:
On 2022/9/22 19:09, Mimi Zohar wrote:
Hi Scott,
On Wed, 2022-09-21 at 20:58 +0800, GUO Zihua wrote:
}
- if (!rc)
- return false;
+
+ if (rc == -ESTALE && !rule_reinitialized) {
Ok, this limits allocating ima_lsm_copy_rule() to the first -ESTALE,
+ lsm_rule = ima_lsm_copy_rule(rule);
+ if (lsm_rule) {
+ rule_reinitialized = true;
+ goto retry;
but "retry" is also limited to the first -ESTALE.
Technically we would only need one retry. This loop is looping on all
the lsm members of one rule, and ima_lsm_copy_rule would update all the
lsm members of this rule. The "lsm member" here refers to LSM defined
properties like obj_user, obj_role etc. These members are of AND
relation, meaning all lsm members together would form one LSM rule.
As of the scenario you mentioned, I think it should be really rare.
Spending to much time and code on this might not worth it.
+ }
+ }
Either there can be multiple LSM fields and the memory is allocated
once and freed once at the end, as you suggested, or the memory should
be freed here and rule_reinitialized reset, minimizing the code change.
I might have overlooked something, but if I understands the code
correctly, we would be copying the same rule over and over again till
the loop ends in that case. ima_lsm_update_rule() would replace the rule
node on the rule list without updating the rule in place. Although
synchronize_rcu() should prevent a UAF, the rule in ima_match_rules()
would not be updated. Meaning SELinux would always return -ESTALE before
we copy and retry as we keep passing in the outdated rule entry.
+ if (!rc) {
+ result = false;
+ goto out;
+ }
}
- return true;
+ result = true;
+
+out:
+ if (rule_reinitialized) {
+ for (i = 0; i < MAX_LSM_RULES; i++)
+ ima_filter_rule_free(lsm_rule->lsm[i].rule);
+ kfree(lsm_rule);
+ }
+ return result;
}
--
Best
GUO Zihua
