[PATCH v13 3/5] security: Replace indirect LSM hook calls with static calls

[Paul Moore]

On Wed, Jul 3, 2024 at 12:55 PM KP Singh <kpsingh at kernel.org> wrote:
> On Wed, Jul 3, 2024 at 2:07 AM Paul Moore <paul at paul-moore.com> wrote:
> >
> > On Jun 29, 2024 KP Singh <kpsingh at kernel.org> wrote:
> > >
> > > LSM hooks are currently invoked from a linked list as indirect calls
> > > which are invoked using retpolines as a mitigation for speculative
> > > attacks (Branch History / Target injection) and add extra overhead which
> > > is especially bad in kernel hot paths:
>
> [...]
>
> > should fix the more obvious problems.  I'd like to know if you are
> > aware of any others?  If not, the text above should be adjusted and
> > we should reconsider patch 5/5.  If there are other problems I'd
> > like to better understand them as there may be an independent
> > solution for those particular problems.
>
> We did have problems with some other hooks but I was unable to dig up
> specific examples though, it's been a while. More broadly speaking, a
> default decision is still a decision. Whereas the intent from the BPF
> LSM is not to make a default decision unless a BPF program is loaded.
> I am quite worried about the holes this leaves open, subtle bugs
> (security or crashes) we have not caught yet and PATCH 5/5 engineers away
>  the problem of the "default decision".

The inode/xattr problem you originally mentioned wasn't really rooted
in a "bad" default return value, it was really an issue with how the
LSM hook was structured due to some legacy design assumptions made
well before the initial stacking patches were merged.  That should be
fixed now[1] and given that the inode/xattr set/remove hooks were
unique in this regard (individual LSMs were responsible for performing
the capabilities checks) I don't expect this to be a general problem.

There were also some issues caused by the fact that we were defining
the default return value in multiple places and these values had gone
out of sync in a number of hooks.  We've also fixed this problem by
only defining the default return value once for each hook, solving all
of those problems.

I'm not aware of any other existing problems relating to the LSM hook
default values, if there are any, we need to fix them independent of
this patchset.  The LSM framework should function properly if the
"default" values are used.

[1] I just realized that commit 61df7b828204 doesn't properly update
the removexattr implementations for SELinux and Smack, expect a patch
for that soon.  The current code is okay, it just does some
unnecessary work (see the setxattr changes to get an idea of the
changes needed).

> > > +#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))
> >
> > This is probably a stupid question, but why use static_key_enabled()
> > here instead of static_branch_unlikely() as in the call_XXX_macros?
>
> The static_key_enabled is a check for the key being enabled, whereas
> the static_branch_likely is for laying out the right assembly code
> (jump tables etc.) and based on the value of the static key, here we
> are not using the static calls or even keys, rather we are following
> back from direct calls to indirect calls and don't really need any
> jump tables in the slow path.

Gotcha, thanks for the explanation.

-- 
paul-moore.com