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

[Jiri Olsa <olsajiri@gmail.com>]

On Thu, Nov 02, 2023 at 01:46:14AM +0100, KP Singh wrote:
> On Mon, Oct 9, 2023 at 12:11 PM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Fri, Oct 06, 2023 at 10:47:00PM +0200, KP Singh 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>
> > > Signed-off-by: KP Singh <kpsingh@kernel.org>
> >
> > small nit, but looks good
> >
> > Acked-by: Jiri Olsa <jolsa@kernel.org>
> >
> > jirka
> >
> >
> > > ---
> > >  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);
> >
> > nit, this could be static, unless there are future plans ;-)
> 
> Actually, this is called from trampoline.c and cannot be static.

ah you're right, I missed that

jirka