Re: [PATCH v5 5/5] IMA: introduce a new policy option func=SETXATTR_CHECK

[Mimi Zohar <zohar@linux.ibm.com>]

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