[PATCH v13 5/5] bpf: Only enable BPF LSM hooks when an LSM program is attached

[KP Singh]

On Wed, Jul 3, 2024 at 2:07 AM Paul Moore <paul at paul-moore.com> wrote:
>
> On Jun 29, 2024 KP Singh <kpsingh at kernel.org> wrote:
> >
> > BPF LSM hooks have side-effects (even when a default value's 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:
> >    0xff...0e30 <+0>:  endbr64
> >    0xff...0e34 <+4>:  nopl   0x0(%rax,%rax,1)
> >    0xff...0e39 <+9>:  push   %rbp
> >    0xff...0e3a <+10>: push   %r14
> >    0xff...0e3c <+12>: push   %rbx
> >    0xff...0e3d <+13>: mov    %rdx,%rbx
> >    0xff...0e40 <+16>: mov    %esi,%ebp
> >    0xff...0e42 <+18>: mov    %rdi,%r14
> >    0xff...0e45 <+21>: jmp    0xff...0e57 <security_file_ioctl+39>
> >                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >
> >    Static key enabled for SELinux
> >
> >    0xff...0e47 <+23>: xchg   %ax,%ax
> >                       ^^^^^^^^^^^^^^
> >
> >    Static key disabled for BPF. This gets patched when a BPF LSM
> >    program is attached
> >
> >    0xff...0e49 <+25>: xor    %eax,%eax
> >    0xff...0e4b <+27>: xchg   %ax,%ax
> >    0xff...0e4d <+29>: pop    %rbx
> >    0xff...0e4e <+30>: pop    %r14
> >    0xff...0e50 <+32>: pop    %rbp
> >    0xff...0e51 <+33>: cs jmp 0xff...0000 <__x86_return_thunk>
> >    0xff...0e57 <+39>: endbr64
> >    0xff...0e5b <+43>: mov    %r14,%rdi
> >    0xff...0e5e <+46>: mov    %ebp,%esi
> >    0xff...0e60 <+48>: mov    %rbx,%rdx
> >    0xff...0e63 <+51>: call   0xff...33c0 <selinux_file_ioctl>
> >    0xff...0e68 <+56>: test   %eax,%eax
> >    0xff...0e6a <+58>: jne    0xff...0e4d <security_file_ioctl+29>
> >    0xff...0e6c <+60>: jmp    0xff...0e47 <security_file_ioctl+23>
> >    0xff...0e6e <+62>: endbr64
> >    0xff...0e72 <+66>: mov    %r14,%rdi
> >    0xff...0e75 <+69>: mov    %ebp,%esi
> >    0xff...0e77 <+71>: mov    %rbx,%rdx
> >    0xff...0e7a <+74>: call   0xff...e3b0 <bpf_lsm_file_ioctl>
> >    0xff...0e7f <+79>: test   %eax,%eax
> >    0xff...0e81 <+81>: jne    0xff...0e4d <security_file_ioctl+29>
> >    0xff...0e83 <+83>: jmp    0xff...0e49 <security_file_ioctl+25>
> >    0xff...0e85 <+85>: endbr64
> >    0xff...0e89 <+89>: mov    %r14,%rdi
> >    0xff...0e8c <+92>: mov    %ebp,%esi
> >    0xff...0e8e <+94>: mov    %rbx,%rdx
> >    0xff...0e91 <+97>: pop    %rbx
> >    0xff...0e92 <+98>: pop    %r14
> >    0xff...0e94 <+100>:        pop    %rbp
> >    0xff...0e95 <+101>:        ret
> >
> > This patch enables this by providing a LSM_HOOK_INIT_RUNTIME variant
> > that allows the LSMs to opt-in to hooks which can be toggled at runtime
> > which with security_toogle_hook.
> >
> > Reviewed-by: Kees Cook <keescook at chromium.org>
> > Acked-by: Casey Schaufler <casey at schaufler-ca.com>
> > Signed-off-by: KP Singh <kpsingh at kernel.org>
> > ---
> >  include/linux/lsm_hooks.h | 30 ++++++++++++++++++++++++++++-
> >  kernel/bpf/trampoline.c   | 40 +++++++++++++++++++++++++++++++++++----
> >  security/bpf/hooks.c      |  2 +-
> >  security/security.c       | 36 ++++++++++++++++++++++++++++++++++-
> >  4 files changed, 101 insertions(+), 7 deletions(-)
>
> I didn't look at this one too closely, see my previous comments in
> patch 3/5, but I did catch one typo, see below ...
>
> > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > index a66ca68485a2..dbe0f40f7f67 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                            runtime;
> >  } __randomize_layout;
>
> The comment header doesn't match the struct fields, "default_state" vs
> "runtime".

Good catch, apologies for the omission.

>
> --
> paul-moore.com