Re: [PATCH v12 5/5] bpf: Only enable BPF LSM hooks when an LSM program is attached
From: Paul Moore <paul@paul-moore.com>
Date: 2024-06-11 01:05:49
Also in:
bpf
On May 15, 2024 KP Singh [off-list ref] 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 <redacted> Acked-by: Casey Schaufler <casey@schaufler-ca.com> Signed-off-by: KP Singh <kpsingh@kernel.org> --- include/linux/lsm_hooks.h | 30 ++++++++++++++++++++++++++++- kernel/bpf/trampoline.c | 40 +++++++++++++++++++++++++++++++++++---- security/bpf/hooks.c | 2 +- security/security.c | 35 +++++++++++++++++++++++++++++++++- 4 files changed, 100 insertions(+), 7 deletions(-)
...
quoted hunk ↗ jump to hunk
diff --git a/security/security.c b/security/security.c index 9654ca074aed..2f8bcacf1fb4 100644 --- a/security/security.c +++ b/security/security.c@@ -885,6 +887,37 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u32 *uctx_len, return rc; } +/** + * security_toggle_hook - Toggle the state of the LSM hook. + * @hook_addr: The address of the hook to be toggled. + * @state: Whether to enable for disable the hook. + * + * Returns 0 on success, -EINVAL if the address is not found. + */ +int security_toggle_hook(void *hook_addr, bool state) +{ + struct lsm_static_call *scalls = ((void *)&static_calls_table);
GCC (v14.1.1 if that matters) is complaining about casting randomized structs. Looking quickly at the two structs, lsm_static_call and lsm_static_calls_table, I suspect the cast is harmless even if the randstruct case, but I would like to see some sort of fix for this so I don't get spammed by GCC every time I do a build. On the other hand, if this cast really is a problem in the randstruct case we obviously need to fix that. Either way, resolve this and make sure you test with GCC/randstruct enabled.
+ unsigned long num_entries =
+ (sizeof(static_calls_table) / sizeof(struct lsm_static_call));
+ int i;
+
+ for (i = 0; i < num_entries; i++) {
+
+ if (!scalls[i].hl || !scalls[i].hl->runtime)
+ continue;
+
+ if (scalls[i].hl->hook.lsm_func_addr != hook_addr)
+ continue;
+
+ if (state)
+ static_branch_enable(scalls[i].active);
+ else
+ static_branch_disable(scalls[i].active);
+ return 0;
+ }
+ return -EINVAL;
+}
+
/*
* The default value of the LSM hook is defined in linux/lsm_hook_defs.h and
* can be accessed with:
--
2.45.0.rc1.225.g2a3ae87e7f-goog-- paul-moore.com