Hi Simon,
On Thu, 2021-07-29 at 07:47 +0000, THOBY Simon wrote:
>
> >> + * for setxattr writes
> >> + *
> >> + * Update the atomic variable holding the set of allowed hash algorithms
> >> + * that can be used to update the security.ima xattr of a file.
> >> + *
> >> + * Context: called when updating the IMA policy.
> >> + *
> >> + * SETXATTR_CHECK rules do not implement a full policy check because of
> >> + * the performance impact performing rules checking on setxattr() would
> >> + * have. The consequence is that only one SETXATTR_CHECK can be active at
> >> + * a time.
> >> + */
> >> +static void update_allowed_hash_algorithms(void)
> >> +{
> >> + struct ima_rule_entry *entry;
> >> +
> >> + /*
> >> + * We scan in reverse order because only the last entry with the
> >> + * 'func=SETXATTR_CHECK' apply: this allows runtime upgrades of the
> >> + * digest algorithm policy, unlike the other IMA rules that are
> >> + * usually append-only. Old rules will still be present in the
> >> + * ruleset, but inactive.
> >> + */
> >
> > Oh, my! I really hope this won't be used as precedent. Before
> > agreeing to this, the existing policy rules must require loading of
> > only signed IMA policies.
> >
>
>
> After some though, I think you're right, there is not much point to do anything
> different from the other IMA rules,
>
> Below is the modified version that I will submit in the next patch.
>
> However given the similarities between this function and ima_update_policy_flag,
> I think maybe I should merge them together: they are mostly called from the
> same places and they both serve the same role: updating some of the global ima
> variables after a policy update or at system initialization.
>
> Do you think it would be ok to add that functionality to ima_update_policy_flag?
> Maybe updating the name to reflect that the function updates multiples flags?
We've gone through a couple of iterations of this patch. At this
point, it's defining a single set of setxattr permitted hash
algorithms. Renaming and adding the change to
ima_update_policy_flags() definitely makes sense.
At the same time, I'd appreciate your fixing the RCU locking there.
> As a side note on the patchset, maybe I should refrain from posting for a few days to give people time
> to comment on it, instead of sending new versions in such a quick succession?
Definitely.
thanks,
Mimi
