Re: [PATCH v39 01/42] integrity: disassociate ima_filter_rule from security_audit_rule

[Casey Schaufler <casey@schaufler-ca.com>]

On 6/24/2024 4:05 PM, Paul Moore wrote:
> On Mon, Jun 24, 2024 at 6:19 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 6/24/2024 3:03 PM, Paul Moore wrote:
>>> On Mon, Jun 24, 2024 at 9:57 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>> On Mon, 2024-06-24 at 10:45 +0200, Roberto Sassu wrote:
>>>>> My only comment would be that I would not call the new functions with
>>>>> the ima_ prefix, being those in security.c, which is LSM agnostic, but
>>>>> I would rather use a name that more resembles the differences, if any.
>>>> Commit 4af4662fa4a9 ("integrity: IMA policy") originally referred to these hooks
>>>> as security_filter_rule_XXXX, but commit b8867eedcf76 ("ima: Rename internal
>>>> filter rule functions") renamed the function to ima_filter_rule_XXX) to avoid
>>>> security namespace polution.
>>>>
>>>> If these were regular security hooks, the hooks would be named:
>>>> filter_rule_init, filter_rule_free, filter_rule_match with the matching
>>>> "security" prefix functions. Audit and IMA would then register the hooks.
>>>>
>>>> I agree these functions should probably be renamed again, probably to
>>>> security_ima_filter_rule_XXXX.
>>> It's funny, my mind saw that the patch was removing those preprocessor
>>> macros and was so happy it must have shut off, because we already have
>>> security_XXX functions for these :)
>>>
>>> See security_audit_rule_init(), security_audit_rule_free(), and
>>> security_audit_rule_match().
>>>
>>> Casey, do you want to respin this patch to use the existing LSM
>>> functions?
>> If you want to use shared functions they shouldn't be security_audit_blah().
>> I like Mimi's suggestion. Rename security_audit_filter_rule_init() to
>> security_filter_rule_init() and use that in both places.
> They are currently shared, the preprocessor macros just hide that fact
> (which is not a good thing, IMO).  Renaming the existing LSM functions
> to drop the "audit" in the name doesn't really solve the problem as
> you still end up with "Audit_equal", etc. constants (which are awful
> for multiple reasons, some having nothing to do with the LSM) in the
> callers.
>
> .. and let me just get ahead of this, please do not do a macro-based
> rename of "Audit_equal" to something else to "fix" that problem;
> that's just as bad as what we have now.

Agreed.

> Properly fixing this may be worthwhile, but I think it's an
> unnecessary distraction at this point from improving that state of
> multiple LSMs.  If you aren't comfortable submitting a patch that just
> does a "/ima_filter_rule_match/security_audit_rule_match/" style
> rename, or if Mimi and Roberto aren't supportive of that, you might as
> well just drop this from the patchset.

There was a time (long ago by now) when the stacking patches really needed
the functions to be different. They don't now. I'd be perfectly happy with
dropping this patch from the set.

>
> --
> paul-moore.com
>