All of lore.kernel.org
 help / color / mirror / Atom feed
From: KP Singh <kpsingh@kernel.org>
To: Paul Moore <paul@paul-moore.com>
Cc: bpf@vger.kernel.org, linux-security-module@vger.kernel.org,
	 keescook@chromium.org, casey@schaufler-ca.com, song@kernel.org,
	 daniel@iogearbox.net, ast@kernel.org, pabeni@redhat.com,
	andrii@kernel.org,  Jiri Olsa <jolsa@kernel.org>
Subject: Re: [PATCH v9 4/4] bpf: Only enable BPF LSM hooks when an LSM program is attached
Date: Sun, 5 May 2024 18:25:05 +0200	[thread overview]
Message-ID: <CACYkzJ7Pn+hA0yT0UeZEuwSr+ryytw5--Q0nUb+G+fWY5QMhRA@mail.gmail.com> (raw)
In-Reply-To: <f7e8a16b0815d9d901e019934d684c5f@paul-moore.com>

On Thu, Apr 11, 2024 at 2:38 AM Paul Moore <paul@paul-moore.com> wrote:
>
> On Feb  7, 2024 KP Singh <kpsingh@kernel.org> wrote:
> >
> > BPF LSM hooks have side-effects (even when a default value is returned),
> > as some hooks end up behaving differently due to the very presence of
> > the hook.
> >
> > The static keys guarding the BPF LSM hooks are disabled by default and
> > enabled only when a BPF program is attached implementing the hook
> > logic. This avoids the issue of the side-effects and also the minor
> > overhead associated with the empty callback.
> >
> > security_file_ioctl:
> >    0xffffffff818f0e30 <+0>:   endbr64
> >    0xffffffff818f0e34 <+4>:   nopl   0x0(%rax,%rax,1)
> >    0xffffffff818f0e39 <+9>:   push   %rbp
> >    0xffffffff818f0e3a <+10>:  push   %r14
> >    0xffffffff818f0e3c <+12>:  push   %rbx
> >    0xffffffff818f0e3d <+13>:  mov    %rdx,%rbx
> >    0xffffffff818f0e40 <+16>:  mov    %esi,%ebp
> >    0xffffffff818f0e42 <+18>:  mov    %rdi,%r14
> >    0xffffffff818f0e45 <+21>:  jmp    0xffffffff818f0e57 <security_file_ioctl+39>
> >                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >
> >    Static key enabled for SELinux
> >
> >    0xffffffff818f0e47 <+23>:  xchg   %ax,%ax
> >                               ^^^^^^^^^^^^^^
> >
> >    Static key disabled for BPF. This gets patched when a BPF LSM program
> >    is attached
> >
> >    0xffffffff818f0e49 <+25>:  xor    %eax,%eax
> >    0xffffffff818f0e4b <+27>:  xchg   %ax,%ax
> >    0xffffffff818f0e4d <+29>:  pop    %rbx
> >    0xffffffff818f0e4e <+30>:  pop    %r14
> >    0xffffffff818f0e50 <+32>:  pop    %rbp
> >    0xffffffff818f0e51 <+33>:  cs jmp 0xffffffff82c00000 <__x86_return_thunk>
> >    0xffffffff818f0e57 <+39>:  endbr64
> >    0xffffffff818f0e5b <+43>:  mov    %r14,%rdi
> >    0xffffffff818f0e5e <+46>:  mov    %ebp,%esi
> >    0xffffffff818f0e60 <+48>:  mov    %rbx,%rdx
> >    0xffffffff818f0e63 <+51>:  call   0xffffffff819033c0 <selinux_file_ioctl>
> >    0xffffffff818f0e68 <+56>:  test   %eax,%eax
> >    0xffffffff818f0e6a <+58>:  jne    0xffffffff818f0e4d <security_file_ioctl+29>
> >    0xffffffff818f0e6c <+60>:  jmp    0xffffffff818f0e47 <security_file_ioctl+23>
> >    0xffffffff818f0e6e <+62>:  endbr64
> >    0xffffffff818f0e72 <+66>:  mov    %r14,%rdi
> >    0xffffffff818f0e75 <+69>:  mov    %ebp,%esi
> >    0xffffffff818f0e77 <+71>:  mov    %rbx,%rdx
> >    0xffffffff818f0e7a <+74>:  call   0xffffffff8141e3b0 <bpf_lsm_file_ioctl>
> >    0xffffffff818f0e7f <+79>:  test   %eax,%eax
> >    0xffffffff818f0e81 <+81>:  jne    0xffffffff818f0e4d <security_file_ioctl+29>
> >    0xffffffff818f0e83 <+83>:  jmp    0xffffffff818f0e49 <security_file_ioctl+25>
> >    0xffffffff818f0e85 <+85>:  endbr64
> >    0xffffffff818f0e89 <+89>:  mov    %r14,%rdi
> >    0xffffffff818f0e8c <+92>:  mov    %ebp,%esi
> >    0xffffffff818f0e8e <+94>:  mov    %rbx,%rdx
> >    0xffffffff818f0e91 <+97>:  pop    %rbx
> >    0xffffffff818f0e92 <+98>:  pop    %r14
> >    0xffffffff818f0e94 <+100>: pop    %rbp
> >    0xffffffff818f0e95 <+101>: ret
> >
> > Reviewed-by: Kees Cook <keescook@chromium.org>
> > Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>
> > Acked-by: Song Liu <song@kernel.org>
> > Acked-by: Jiri Olsa <jolsa@kernel.org>
> > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > Signed-off-by: KP Singh <kpsingh@kernel.org>
> > ---
> >  include/linux/bpf_lsm.h   |  5 +++++
> >  include/linux/lsm_hooks.h | 13 ++++++++++++-
> >  kernel/bpf/trampoline.c   | 24 ++++++++++++++++++++++++
> >  security/bpf/hooks.c      | 25 ++++++++++++++++++++++++-
> >  security/security.c       |  3 ++-
> >  5 files changed, 67 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
> > index 1de7ece5d36d..5bbc31ac948c 100644
> > --- a/include/linux/bpf_lsm.h
> > +++ b/include/linux/bpf_lsm.h
> > @@ -29,6 +29,7 @@ int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,
> >
> >  bool bpf_lsm_is_sleepable_hook(u32 btf_id);
> >  bool bpf_lsm_is_trusted(const struct bpf_prog *prog);
> > +void bpf_lsm_toggle_hook(void *addr, bool value);
> >
> >  static inline struct bpf_storage_blob *bpf_inode(
> >       const struct inode *inode)
> > @@ -78,6 +79,10 @@ static inline void bpf_lsm_find_cgroup_shim(const struct bpf_prog *prog,
> >  {
> >  }
> >
> > +static inline void bpf_lsm_toggle_hook(void *addr, bool value)
> > +{
> > +}
> > +
> >  #endif /* CONFIG_BPF_LSM */
> >
> >  #endif /* _LINUX_BPF_LSM_H */
> > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > index ba63d8b54448..e95f0a5cb409 100644
> > --- a/include/linux/lsm_hooks.h
> > +++ b/include/linux/lsm_hooks.h
> > @@ -110,11 +110,14 @@ struct lsm_id {
> >   * @scalls: The beginning of the array of static calls assigned to this hook.
> >   * @hook: The callback for the hook.
> >   * @lsm: The name of the lsm that owns this hook.
> > + * @default_state: The state of the LSM hook when initialized. If set to false,
> > + * the static key guarding the hook will be set to disabled.
> >   */
> >  struct security_hook_list {
> >       struct lsm_static_call  *scalls;
> >       union security_list_options     hook;
> >       const struct lsm_id             *lsmid;
> > +     bool                            default_enabled;
>
> Ugh.  We've already got an lsm_static_call::active field, I don't want
> to see another enable/active/etc. flag unless there is absolutely no way
> this works otherwise.

The field default_enabled is used at the time of initialization. The
lsm_static_call::active is a static key which we really cannot use at
initialization time from the various LSMs directly. I don't see a way
out of this one IMHO.

>
> >  } __randomize_layout;
> >
> >  /*
> > @@ -164,7 +167,15 @@ static inline struct xattr *lsm_get_xattr_slot(struct xattr *xattrs,
> >  #define LSM_HOOK_INIT(NAME, CALLBACK)                        \
> >       {                                               \
> >               .scalls = static_calls_table.NAME,      \
> > -             .hook = { .NAME = CALLBACK }            \
> > +             .hook = { .NAME = CALLBACK },           \
> > +             .default_enabled = true                 \
> > +     }
> > +
> > +#define LSM_HOOK_INIT_DISABLED(NAME, CALLBACK)               \
> > +     {                                               \
> > +             .scalls = static_calls_table.NAME,      \
> > +             .hook = { .NAME = CALLBACK },           \

[...]

               static_branch_disable(scalls->active);
> > +             }
> > +     }
> > +}
>
> More ugh.  If we're going to solve things this way, let's make it a
> proper LSM interface and not a BPF LSM specific hack; I *really* don't
> want to see individual LSMs managing the lsm_static_call or
> security_hook_list entries.
>

Fair, and that makes the implementation much simpler too. I created a
security_hook_toggle function in security.c which implements this
functionality.

- KP

> > diff --git a/security/security.c b/security/security.c
> > index e05d2157c95a..40d83da87f68 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -406,7 +406,8 @@ static void __init lsm_static_call_init(struct security_hook_list *hl)
> >                       __static_call_update(scall->key, scall->trampoline,
> >                                            hl->hook.lsm_callback);
> >                       scall->hl = hl;
> > -                     static_branch_enable(scall->active);
> > +                     if (hl->default_enabled)
> > +                             static_branch_enable(scall->active);
> >                       return;
> >               }
> >               scall++;
> > --
> > 2.43.0.594.gd9cf4e227d-goog
>
> --
> paul-moore.com

      reply	other threads:[~2024-05-05 16:25 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
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 [this message]

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=CACYkzJ7Pn+hA0yT0UeZEuwSr+ryytw5--Q0nUb+G+fWY5QMhRA@mail.gmail.com \
    --to=kpsingh@kernel.org \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=casey@schaufler-ca.com \
    --cc=daniel@iogearbox.net \
    --cc=jolsa@kernel.org \
    --cc=keescook@chromium.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=paul@paul-moore.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.