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

From: KP Singh <kpsingh@kernel.org>
To: Casey Schaufler <casey@schaufler-ca.com>
Cc: Song Liu <song@kernel.org>, Kees Cook <keescook@chromium.org>,
	linux-security-module@vger.kernel.org, bpf@vger.kernel.org,
	ast@kernel.org, daniel@iogearbox.net, jackmanb@google.com,
	renauld@google.com, paul@paul-moore.com, revest@chromium.org
Subject: Re: [PATCH RESEND bpf-next 3/4] security: Replace indirect LSM hook calls with static calls
Date: Mon, 6 Feb 2023 19:41:04 +0100	[thread overview]
Message-ID: <CACYkzJ7=xPB-mzAq-GBszm2Q3NapVNbcrqBdSS0FZsTeTbL9QA@mail.gmail.com> (raw)
In-Reply-To: <8b5f62f3-a2c4-9ba3-d1e4-af557047f44b@schaufler-ca.com>

On Mon, Feb 6, 2023 at 7:29 PM Casey Schaufler <casey@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@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@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."

>
> >
> >>>  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.

>
> >
> > Thanks,
> > Song