[PATCH v13 4/5] security: Update non standard hooks to use static calls

[Paul Moore]

On Tue, Jul 9, 2024 at 8:36 AM KP Singh <kpsingh at kernel.org> wrote:
> > > --- a/security/security.c
> > > +++ b/security/security.c

...

> > > -#define lsm_for_each_hook(scall, NAME)                                       \
> > > -     for (scall = static_calls_table.NAME;                           \
> > > -          scall - static_calls_table.NAME < MAX_LSM_COUNT; scall++)  \
> > > -             if (static_key_enabled(&scall->active->key))
> > > +/*
> > > + * Can be used in the context passed to lsm_for_each_hook to get the lsmid of the
> > > + * current hook
> > > + */
> > > +#define current_lsmid() _hook_lsmid
> >
> > See my comments below about security_getselfattr(), I think we can drop
> > the current_lsmid() macro.  If we really must keep it, we need to rename
> > it to something else as it clashes too much with the other current_XXX()
> > macros/functions which are useful outside of our wacky macros.
>
> call_hook_with_lsmid is a pattern used by quite a few hooks, happy to
> update the name.
>
> What do you think about __security_hook_lsm_id().

I guess we can't get rid of it due to the crazy macro stuff with loop
unrolling, BEFORE/AFTER blocks, etc.  Ooof.  If you were looking for
another example of why I don't really like these patches, this would
be a good candidate.

More below ...

> > I know I was the one who asked to implement the static_calls for *all*
> > of the LSM functions - thank you for doing that - but I think we can
> > all agree that some of the resulting code is pretty awful.  I'm probably
> > missing something important, but would an apporach similar to the pseudo
> > code below work?
>
> This does not work.
>
> The special macro you are defining does not have the static_call
> invocation and if you add that bit it's basically the __CALL_HOOK
> macro or __CALL_STATIC_INT, __CALL_STATIC_VOID macro inlined
> everywhere, I tried implementing it but it gets very dirty.

Thanks for testing it out.  Perhaps trying to move all of these hooks
to use the static_call approach was a mistake.  I realize you're doing
your best adapting the static_call API to support multiple LSMs, but
it just doesn't look like a good fit to me for the "unconventional"
hooks here in this patch.

> > I think we may need to admit defeat with security_getselfattr() and
> > leave it as-is, the above is just too ugly to live.  I'd suggest
> > adding a comment explaining that it wasn't converted due to complexity
> > and the resulting awfulness.
>
> I think your position on fixing everything is actually a valid one for
> security, which is why I did not contest it.
>
> The security_getselfattr is called very close to the syscall boundary
> and the closer to the boundary the call is, the greater control the
> attacker has over arguments and the easier it is to mount the attack.
> This is why LSM indirect calls are a lucrative target because they
> happen fairly early in the transition from user to kernel.
> security_getselfattr is literally just in a SYSCALL_DEFINE

I recognize that your comments are in reference to that last flaw
rooted in the hardware that used indirect calls at an attack vector,
but wasn't that resolved through other means?  I never saw the PoC or
had time to follow up on whatever mitigation was ultimately merged (if
any).  However, my understanding is that the move to static_calls is
not strictly necessary to patch over that particular hardware flaw, it
is just a we-really-want-this for either a performance or a
non-specific security reason; pick your favorite  of the two based on
your audience.

Regardless, since none of the previous suggestions/options proved to
be workable, I'm going to suggest we just kill this patch too and move
forward with the others.  I had hoped we could get the changes in this
patch cleaned up, but it doesn't look like that is going to be the
case, or at least not within a week or two, so let's drop it and we
can always reconsider this in the future if a cleaner implementation
is presented.

-- 
paul-moore.com