Re: [PATCH v13 5/5] bpf: Only enable BPF LSM hooks when an LSM program is attached
From: KP Singh <kpsingh@kernel.org>
Date: 2024-07-03 16:55:39
Also in:
bpf
On Wed, Jul 3, 2024 at 2:07 AM Paul Moore [off-list ref] wrote:
On Jun 29, 2024 KP Singh [off-list ref] wrote:quoted
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 | 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 ...quoted
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