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@xxxxxxxxx> 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@xxxxxxxxxxxx> > > > Reviewed-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx> > > > Acked-by: Song Liu <song@xxxxxxxxxx> > > > Signed-off-by: KP Singh <kpsingh@xxxxxxxxxx> > > > > small nit, but looks good > > > > Acked-by: Jiri Olsa <jolsa@xxxxxxxxxx> > > > > 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
