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.
> >> + 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;
> >> }
--
thanks,
Mimi
