| Message ID | 20220909011516.55957-3-guozihua@huawei.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | ima: Handle -ESTALE returned by ima_filter_rule_match() | expand |
Hi Scott, > @@ -612,6 +614,8 @@ static bool ima_match_rules(struct ima_rule_entry *rule, > else > return false; > } > + > +retry: > switch (i) { > case LSM_OBJ_USER: > case LSM_OBJ_ROLE: > @@ -631,10 +635,28 @@ static bool ima_match_rules(struct ima_rule_entry *rule, > default: > break; > } > - if (!rc) > - return false; > + > + if (rc == -ESTALE) { > + rule = ima_lsm_copy_rule(rule); Re-using rule here > + if (rule) { and here doesn't look right. > + rule_reinitialized = true; > + goto retry; > + } > + } > + 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(rule->lsm[i].rule); > + kfree(rule); > + } Shouldn't freeing the memory be immediately after the retry? Otherwise, only the last instance of processing -ESTALE would be freed. > + return result; > } > > /*
On 2022/9/20 5:35, Mimi Zohar wrote: > Hi Scott, > >> @@ -612,6 +614,8 @@ static bool ima_match_rules(struct ima_rule_entry *rule, >> else >> return false; >> } >> + >> +retry: >> switch (i) { >> case LSM_OBJ_USER: >> case LSM_OBJ_ROLE: >> @@ -631,10 +635,28 @@ static bool ima_match_rules(struct ima_rule_entry *rule, >> default: >> break; >> } >> - if (!rc) >> - return false; >> + >> + if (rc == -ESTALE) { >> + rule = ima_lsm_copy_rule(rule); > > Re-using rule here I'll use another variable here. > >> + if (rule) { > > and here doesn't look right. What seems to be wrong here? ima_lsm_copy_rule() returns a shallow copy of the rule, and NULL if the copy fails. Only if the returned rule is not NULL should we proceed with the retry. I used rule_reinitialized to memorize whether the current rule is copied so that we should free it later on. > >> + rule_reinitialized = true; >> + goto retry; >> + } >> + } >> + 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(rule->lsm[i].rule); >> + kfree(rule); >> + } > > Shouldn't freeing the memory be immediately after the retry? > Otherwise, only the last instance of processing -ESTALE would be freed. ima_lsm_copy_rule() would update every member of rule->lsm, and the retry is within the for loop on members of rule->lsm. We'd better keep the copied rule till the loop ends. To avoid race condition if the LSM rule has been updated again during the loop, I can add a guard here. > >> + return result; >> } >> >> /* >
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 8040215c0252..cb672df9b888 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -545,6 +545,8 @@ static bool ima_match_rules(struct ima_rule_entry *rule, const char *func_data) { int i; + bool result = false; + bool rule_reinitialized = false; if ((rule->flags & IMA_FUNC) && (rule->func != func && func != POST_SETATTR)) @@ -612,6 +614,8 @@ static bool ima_match_rules(struct ima_rule_entry *rule, else return false; } + +retry: switch (i) { case LSM_OBJ_USER: case LSM_OBJ_ROLE: @@ -631,10 +635,28 @@ static bool ima_match_rules(struct ima_rule_entry *rule, default: break; } - if (!rc) - return false; + + if (rc == -ESTALE) { + rule = ima_lsm_copy_rule(rule); + if (rule) { + rule_reinitialized = true; + goto retry; + } + } + 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(rule->lsm[i].rule); + kfree(rule); + } + return result; } /*
IMA relies on the blocking LSM policy notifier callback to update the LSM based IMA policy rules. When SELinux update its policies, IMA would be notified and starts updating all its lsm rules one-by-one. During this time, -ESTALE would be returned by ima_filter_rule_match() if it is called with a LSM rule that has not yet been updated. In ima_match_rules(), -ESTALE is not handled, and the LSM rule is considered a match, causing extra files to be measured by IMA. Fix it by re-initializing a temporary rule if -ESTALE is returned by ima_filter_rule_match(). The origin rule in the rule list would be updated by the LSM policy notifier callback. Fixes: b16942455193 ("ima: use the lsm policy update notifier") Signed-off-by: GUO Zihua <guozihua@huawei.com> --- security/integrity/ima/ima_policy.c | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-)