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

[KP Singh]

On Mon, Feb 6, 2023 at 6:49 PM Song Liu <song at kernel.org> 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

+1 to everything Song said, I am not saying that one direction is
better than the other and for distros that have LSMs (like SELinux and
AppArmor enabled) it's okay to have this default to
static_branch_likely. On systems that will have just the BPF LSM
enabled, it's the opposite that is true, i.e. one would never add a
hook on a hotpath as the overheads are unacceptable, and when one does
add a hook, they are willing to add the extra overhead (this is
already much less compared to the indirect calls). I am okay with the
default being static_branch_likely if that's what the other LSM
maintainers prefer.


> 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.
>
> >
> > >  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.
>
> Thanks,
> Song