All of lore.kernel.org
 help / color / mirror / Atom feed
From: Paul Moore <paul@paul-moore.com>
To: KP Singh <kpsingh@kernel.org>
Cc: bpf@vger.kernel.org, linux-security-module@vger.kernel.org,
	 Kees Cook <keescook@chromium.org>,
	Casey Schaufler <casey@schaufler-ca.com>,
	song@kernel.org,  daniel@iogearbox.net, ast@kernel.org,
	pabeni@redhat.com, andrii@kernel.org
Subject: Re: [PATCH v9 3/4] security: Replace indirect LSM hook calls with static calls
Date: Fri, 12 Apr 2024 11:39:02 -0400	[thread overview]
Message-ID: <CAHC9VhTN-H1m01BZ2Z_W_roTEeUGf-k=_YmOb-12ZNJ_996foA@mail.gmail.com> (raw)
In-Reply-To: <492036A7-4944-4225-B045-3C2F79DBEA31@kernel.org>

On Thu, Apr 11, 2024 at 3:12 AM KP Singh <kpsingh@kernel.org> wrote:
> > On 11 Apr 2024, at 02:38, Paul Moore <paul@paul-moore.com> wrote:
> > On Feb  7, 2024 KP Singh <kpsingh@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:

...

> > Beyond that, let's find a way to use static calls in the LSM hooks
> > which don't use the call_{int,void}_hook() macros.  If we're going to do
> > this to help close some attack vectors, let's make sure we do the
> > conversion everywhere.
>
> This is surely doable, We can unroll the loop individually in these separate hooks. It would need separate
>
> LSM_LOOP_UNROLL(__CALL_STATIC_xfrm_state_pol_flow_match, x, xp file)
>
> Would you be okay if we do it in a follow up series? These are special hooks and I don't want to introduce any subtle logical bugs when fixing potential speculative side channels (Which could be fixed with retpolines, proper flushing at privilege changes etc).

I'm okay if you want to do it in a separate patch, but I would like to
see it included in the same patchset.  The good news is that recent
commits have significantly reduced the number of cases where we aren't
using the macros.

> >> @@ -846,29 +906,41 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, size_t *uctx_len,
> >>  * call_int_hook:
> >>  * This is a hook that returns a value.
> >>  */
> >> +#define __CALL_STATIC_VOID(NUM, HOOK, ...)      \
> >> +do {      \
> >> + if (static_branch_unlikely(&SECURITY_HOOK_ACTIVE_KEY(HOOK, NUM))) {    \
> >
> > I'm not a fan of the likely()/unlikely() style markings/macros in cases
> > like this as it can vary tremendously.  Drop the likely()/unlikely()
> > checks and just do a static_call().
> >
>
> These are actually not the the classical likely, unlikely macros which are just hints to the compiler:
>
> #define likely(x) __builtin_expect(!!(x), 1)
> #define unlikely(x) __builtin_expect(!!(x), 0
>
>
> but a part of the static keys API which generates jump tables and the code generated depends on the (default state, likelyhood). It could have been named better, all we need is to have a jump table so that we can optimize this extra branch in hotpaths, in one direction.
>
>    https://www.kernel.org/doc/Documentation/static-keys.txt
>
>
> If you want I can put this behind a macro:
>
>
> #define LSM_HOOK_ACTIVE(HOOK, NUM) static_branch_unlikely(&SECURITY_HOOK_ACTIVE_KEY(HOOK, NUM)
>
> the static_branch_likely / static_branch_unlikey actually does not matter much here, because without this we have a conditional branch and an extra load.

Fair enough, leave it as-is.  Thanks for the explanation.

-- 
paul-moore.com

  reply	other threads:[~2024-04-12 15:38 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-02-07 12:49 [PATCH v9 0/4] Reduce overhead of LSMs with static calls KP Singh
2024-02-07 12:49 ` [PATCH v9 1/4] kernel: Add helper macros for loop unrolling KP Singh
2024-02-07 12:49 ` [PATCH v9 2/4] security: Count the LSMs enabled at compile time KP Singh
2024-02-07 12:49 ` [PATCH v9 3/4] security: Replace indirect LSM hook calls with static calls KP Singh
2024-04-11  0:38   ` Paul Moore
2024-04-11  7:12     ` KP Singh
2024-04-12 15:39       ` Paul Moore [this message]
2024-02-07 12:49 ` [PATCH v9 4/4] bpf: Only enable BPF LSM hooks when an LSM program is attached KP Singh
2024-04-11  0:38   ` Paul Moore
2024-05-05 16:25     ` KP Singh

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAHC9VhTN-H1m01BZ2Z_W_roTEeUGf-k=_YmOb-12ZNJ_996foA@mail.gmail.com' \
    --to=paul@paul-moore.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=casey@schaufler-ca.com \
    --cc=daniel@iogearbox.net \
    --cc=keescook@chromium.org \
    --cc=kpsingh@kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=song@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.