[PATCH RESEND bpf-next 3/4] security: Replace indirect LSM hook calls with static calls

[KP Singh]

On Mon, Feb 6, 2023 at 7:51 PM Kees Cook <keescook at chromium.org> wrote:
>
> On Mon, Feb 06, 2023 at 07:41:04PM +0100, KP Singh wrote:
> > On Mon, Feb 6, 2023 at 7:29 PM Casey Schaufler <casey at schaufler-ca.com> wrote:
> > >
> > > On 2/6/2023 9:48 AM, Song Liu wrote:
> > > > On Mon, Feb 6, 2023 at 8:29 AM Casey Schaufler <casey at schaufler-ca.com> wrote:
> > > >> On 2/6/2023 5:04 AM, KP Singh wrote:
> > > >>> On Fri, Jan 20, 2023 at 5:36 AM Kees Cook <keescook at chromium.org> wrote:
> > > >>>> On Fri, Jan 20, 2023 at 01:08:17AM +0100, KP Singh wrote:
> > > >>>>> The indirect calls are not really needed as one knows the addresses of
> > > >>> [...]
> > > >>>
> > > >>>>> +/*
> > > >>>>> + * Define static calls and static keys for each LSM hook.
> > > >>>>> + */
> > > >>>>> +
> > > >>>>> +#define DEFINE_LSM_STATIC_CALL(NUM, NAME, RET, ...)                  \
> > > >>>>> +     DEFINE_STATIC_CALL_NULL(LSM_STATIC_CALL(NAME, NUM),             \
> > > >>>>> +                             *((RET(*)(__VA_ARGS__))NULL));          \
> > > >>>>> +     DEFINE_STATIC_KEY_FALSE(SECURITY_HOOK_ENABLED_KEY(NAME, NUM));
> > > >>>> Hm, another place where we would benefit from having separated logic for
> > > >>>> "is it built?" and "is it enabled by default?" and we could use
> > > >>>> DEFINE_STATIC_KEY_MAYBE(). But, since we don't, I think we need to use
> > > >>>> DEFINE_STATIC_KEY_TRUE() here or else won't all the calls be
> > > >>>> out-of-line? (i.e. the default compiled state will be NOPs?) If we're
> > > >>>> trying to optimize for having LSMs, I think we should default to inline
> > > >>>> calls. (The machine code in the commit log seems to indicate that they
> > > >>>> are out of line -- it uses jumps.)
> > > >>>>
> > > >>> I should have added it in the commit description, actually we are
> > > >>> optimizing for "hot paths are less likely to have LSM hooks enabled"
> > > >>> (eg. socket_sendmsg).
> > > >> How did you come to that conclusion? Where is there a correlation between
> > > >> "hot path" and "less likely to be enabled"?
> > > > I could echo KP's reasoning here. AFAICT, the correlation is that LSMs on
> > > > hot path will give more performance overhead. In our use cases (Meta),
> > > > we are very careful with "small" performance hits. 0.25% is significant
> > > > overhead; 1% overhead will not fly without very good reasons (Do we
> > > > have to do this? Are there any other alternatives?). If it is possible to
> > > > achieve similar security on a different hook, we will not enable the hook on
> > > > the hot path. For example, we may not enable socket_sendmsg, but try
> > > > to disallow opening such sockets instead.
> > >
> > > I'm not asking about BPF. I'm asking about the impact on other LSMs.
> > > If you're talking strictly about BPF you need to say that. I'm all for
> > > performance improvement. But as I've said before, it should be for all
> > > the security modules, not just BPF.
> >
> > It's a trade off that will work differently for different LSMs and
> > distros (based on the LSM they chose) and this the config option. I
> > even suggested this be behind CONFIG_EXPERT (which is basically says
> > this:
> >
> >  "This option allows certain base kernel options and settings
> >  to be disabled or tweaked. This is for specialized
> >  environments which can tolerate a "non-standard" kernel.
> >  Only use this if you really know what you are doing."
>
> Using the DEFINE_STATIC_KEY_MAYBE() and static_branch_maybe() macros
> tied to a new CONFIG seems like it can give us a reasonable knob for
> in-line vs out-of-line calls.

Coming back to this after a while as I finally got time to work on
this. (work/personal downtime).

I am changing it to DEFINE_STATIC_KEY_TRUE in this patch and
DEFINE_STATIC_KEY_MAYBE in a subsequent one and guarded by a config
called CONFIG_SECURITY_HOOK_LIKELY. I am letting it default to yes,
but distros can change it depending on their choice of LSM and
performance characteristics.

>
> > > >>>  But I do see that there are LSMs that have these
> > > >>> enabled. Maybe we can put this behind a config option, possibly
> > > >>> depending on CONFIG_EXPERT?
> > > >> Help me, as the maintainer of one of those LSMs, understand why that would
> > > >> be a good idea.
> > > > IIUC, this is also from performance concerns. We would like to manage
> > > > the complexity at compile time for performance benefits.
> > >
> > > What complexity? What config option? I know that I'm slow, but it looks
> > > as if you're suggesting making the LSM infrastructure incredibly fragile
> > > and difficult to understand.
> >
> > I am sorry but the LSM is a core piece of the kernel that currently
> > has significant unnecessary overheads (look at the numbers that I
> > posted) and this not making it fragile, it's making it performant,
> > such optimisations are everywhere in the kernel and the LSM
> > infrastructure has somehow been neglected and is just catching up.
> > These are resources being wasted which could be saved.
>
> Let's just move forward to v2, which I think will look much cleaner. I
> think we can get to both maintainable code and run-time performance with
> this series.
>
> --
> Kees Cook