| Message ID | 20231006204701.549230-5-kpsingh@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Reduce overhead of LSMs with static calls | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for build for s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-13 | success | Logs for test_progs on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-11 | success | Logs for test_progs on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-9 | success | Logs for test_maps on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-4 | success | Logs for build for x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for build for aarch64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-5 | success | Logs for set-matrix |
| bpf/vmtest-bpf-next-VM_Test-10 | success | Logs for test_progs on aarch64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-6 | success | Logs for test_maps on aarch64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-3 | success | Logs for build for x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-12 | success | Logs for test_progs on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-7 | success | Logs for test_maps on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-18 | success | Logs for test_progs_no_alu32_parallel on aarch64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-14 | success | Logs for test_progs_no_alu32 on aarch64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-8 | fail | Logs for test_maps on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-15 | success | Logs for test_progs_no_alu32 on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-16 | success | Logs for test_progs_no_alu32 on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-19 | success | Logs for test_progs_no_alu32_parallel on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-17 | success | Logs for test_progs_no_alu32 on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-20 | success | Logs for test_progs_no_alu32_parallel on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-21 | success | Logs for test_progs_parallel on aarch64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-22 | success | Logs for test_progs_parallel on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-23 | success | Logs for test_progs_parallel on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-24 | success | Logs for test_verifier on aarch64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-25 | success | Logs for test_verifier on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-26 | success | Logs for test_verifier on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-27 | success | Logs for test_verifier on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-28 | success | Logs for veristat |
| bpf/vmtest-bpf-next-PR | success | PR summary |
| bpf/vmtest-bpf-next-VM_Test-0 | success | Logs for ShellCheck |
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 ;-) > > static inline struct bpf_storage_blob *bpf_inode( > const struct inode *inode) > @@ -78,6 +79,10 @@ static inline void bpf_lsm_find_cgroup_shim(const struct bpf_prog *prog, > { > } > > +static inline void bpf_lsm_toggle_hook(void *addr, bool value) > +{ > +} > + > #endif /* CONFIG_BPF_LSM */ > > #endif /* _LINUX_BPF_LSM_H */ > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index c77a1859214d..57ffe4eb6d30 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -97,11 +97,14 @@ struct lsm_static_calls_table { > * @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 char *lsm; > + bool default_state; > } __randomize_layout; > > /* > @@ -151,7 +154,15 @@ static inline struct xattr *lsm_get_xattr_slot(struct xattr *xattrs, > #define LSM_HOOK_INIT(NAME, CALLBACK) \ > { \ > .scalls = static_calls_table.NAME, \ > - .hook = { .NAME = CALLBACK } \ > + .hook = { .NAME = CALLBACK }, \ > + .default_state = true \ > + } > + > +#define LSM_HOOK_INIT_DISABLED(NAME, CALLBACK) \ > + { \ > + .scalls = static_calls_table.NAME, \ > + .hook = { .NAME = CALLBACK }, \ > + .default_state = false \ > } > > extern char *lsm_names; > diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c > index e97aeda3a86b..44788e2eaa1b 100644 > --- a/kernel/bpf/trampoline.c > +++ b/kernel/bpf/trampoline.c > @@ -13,6 +13,7 @@ > #include <linux/bpf_verifier.h> > #include <linux/bpf_lsm.h> > #include <linux/delay.h> > +#include <linux/bpf_lsm.h> > > /* dummy _ops. The verifier will operate on target program's ops. */ > const struct bpf_verifier_ops bpf_extension_verifier_ops = { > @@ -510,6 +511,21 @@ static enum bpf_tramp_prog_type bpf_attach_type_to_tramp(struct bpf_prog *prog) > } > } > > +static void bpf_trampoline_toggle_lsm(struct bpf_trampoline *tr, > + enum bpf_tramp_prog_type kind) > +{ > + struct bpf_tramp_link *link; > + bool found = false; > + > + hlist_for_each_entry(link, &tr->progs_hlist[kind], tramp_hlist) { > + if (link->link.prog->type == BPF_PROG_TYPE_LSM) { > + found = true; > + break; > + } > + } > + bpf_lsm_toggle_hook(tr->func.addr, found); > +} > + > static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_trampoline *tr) > { > enum bpf_tramp_prog_type kind; > @@ -549,6 +565,10 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_tr > > hlist_add_head(&link->tramp_hlist, &tr->progs_hlist[kind]); > tr->progs_cnt[kind]++; > + > + if (link->link.prog->type == BPF_PROG_TYPE_LSM) > + bpf_trampoline_toggle_lsm(tr, kind); > + > err = bpf_trampoline_update(tr, true /* lock_direct_mutex */); > if (err) { > hlist_del_init(&link->tramp_hlist); > @@ -582,6 +602,10 @@ static int __bpf_trampoline_unlink_prog(struct bpf_tramp_link *link, struct bpf_ > } > hlist_del_init(&link->tramp_hlist); > tr->progs_cnt[kind]--; > + > + if (link->link.prog->type == BPF_PROG_TYPE_LSM) > + bpf_trampoline_toggle_lsm(tr, kind); > + > return bpf_trampoline_update(tr, true /* lock_direct_mutex */); > } > > diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c > index cfaf1d0e6a5f..47e1a4777ec9 100644 > --- a/security/bpf/hooks.c > +++ b/security/bpf/hooks.c > @@ -8,7 +8,7 @@ > > static struct security_hook_list bpf_lsm_hooks[] __ro_after_init = { > #define LSM_HOOK(RET, DEFAULT, NAME, ...) \ > - LSM_HOOK_INIT(NAME, bpf_lsm_##NAME), > + LSM_HOOK_INIT_DISABLED(NAME, bpf_lsm_##NAME), > #include <linux/lsm_hook_defs.h> > #undef LSM_HOOK > LSM_HOOK_INIT(inode_free_security, bpf_inode_storage_free), > @@ -32,3 +32,26 @@ DEFINE_LSM(bpf) = { > .init = bpf_lsm_init, > .blobs = &bpf_lsm_blob_sizes > }; > + > +void bpf_lsm_toggle_hook(void *addr, bool value) > +{ > + struct lsm_static_call *scalls; > + struct security_hook_list *h; > + int i, j; > + > + for (i = 0; i < ARRAY_SIZE(bpf_lsm_hooks); i++) { > + h = &bpf_lsm_hooks[i]; > + if (h->hook.lsm_callback != addr) > + continue; > + > + for (j = 0; j < MAX_LSM_COUNT; j++) { > + scalls = &h->scalls[j]; > + if (scalls->hl != &bpf_lsm_hooks[i]) > + continue; > + if (value) > + static_branch_enable(scalls->active); > + else > + static_branch_disable(scalls->active); > + } > + } > +} > diff --git a/security/security.c b/security/security.c > index ce4c0a9107ea..f45e875b6d93 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -382,7 +382,8 @@ static void __init lsm_static_call_init(struct security_hook_list *hl) > __static_call_update(scall->key, scall->trampoline, > hl->hook.lsm_callback); > scall->hl = hl; > - static_branch_enable(scall->active); > + if (hl->default_state) > + static_branch_enable(scall->active); > return; > } > scall++; > -- > 2.42.0.609.gbb76f46606-goog > >
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. - KP > > > > > static inline struct bpf_storage_blob *bpf_inode( > > const struct inode *inode) > > @@ -78,6 +79,10 @@ static inline void bpf_lsm_find_cgroup_shim(const struct bpf_prog *prog, > > { > > } > > > > +static inline void bpf_lsm_toggle_hook(void *addr, bool value) > > +{ > > +} > > + > > #endif /* CONFIG_BPF_LSM */ > > > > #endif /* _LINUX_BPF_LSM_H */ > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > > index c77a1859214d..57ffe4eb6d30 100644 > > --- a/include/linux/lsm_hooks.h > > +++ b/include/linux/lsm_hooks.h > > @@ -97,11 +97,14 @@ struct lsm_static_calls_table { > > * @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 char *lsm; > > + bool default_state; > > } __randomize_layout; > > > > /* > > @@ -151,7 +154,15 @@ static inline struct xattr *lsm_get_xattr_slot(struct xattr *xattrs, > > #define LSM_HOOK_INIT(NAME, CALLBACK) \ > > { \ > > .scalls = static_calls_table.NAME, \ > > - .hook = { .NAME = CALLBACK } \ > > + .hook = { .NAME = CALLBACK }, \ > > + .default_state = true \ > > + } > > + > > +#define LSM_HOOK_INIT_DISABLED(NAME, CALLBACK) \ > > + { \ > > + .scalls = static_calls_table.NAME, \ > > + .hook = { .NAME = CALLBACK }, \ > > + .default_state = false \ > > } > > > > extern char *lsm_names; > > diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c > > index e97aeda3a86b..44788e2eaa1b 100644 > > --- a/kernel/bpf/trampoline.c > > +++ b/kernel/bpf/trampoline.c > > @@ -13,6 +13,7 @@ > > #include <linux/bpf_verifier.h> > > #include <linux/bpf_lsm.h> > > #include <linux/delay.h> > > +#include <linux/bpf_lsm.h> > > > > /* dummy _ops. The verifier will operate on target program's ops. */ > > const struct bpf_verifier_ops bpf_extension_verifier_ops = { > > @@ -510,6 +511,21 @@ static enum bpf_tramp_prog_type bpf_attach_type_to_tramp(struct bpf_prog *prog) > > } > > } > > > > +static void bpf_trampoline_toggle_lsm(struct bpf_trampoline *tr, > > + enum bpf_tramp_prog_type kind) > > +{ > > + struct bpf_tramp_link *link; > > + bool found = false; > > + > > + hlist_for_each_entry(link, &tr->progs_hlist[kind], tramp_hlist) { > > + if (link->link.prog->type == BPF_PROG_TYPE_LSM) { > > + found = true; > > + break; > > + } > > + } > > + bpf_lsm_toggle_hook(tr->func.addr, found); > > +} > > + > > static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_trampoline *tr) > > { > > enum bpf_tramp_prog_type kind; > > @@ -549,6 +565,10 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_tr > > > > hlist_add_head(&link->tramp_hlist, &tr->progs_hlist[kind]); > > tr->progs_cnt[kind]++; > > + > > + if (link->link.prog->type == BPF_PROG_TYPE_LSM) > > + bpf_trampoline_toggle_lsm(tr, kind); > > + > > err = bpf_trampoline_update(tr, true /* lock_direct_mutex */); > > if (err) { > > hlist_del_init(&link->tramp_hlist); > > @@ -582,6 +602,10 @@ static int __bpf_trampoline_unlink_prog(struct bpf_tramp_link *link, struct bpf_ > > } > > hlist_del_init(&link->tramp_hlist); > > tr->progs_cnt[kind]--; > > + > > + if (link->link.prog->type == BPF_PROG_TYPE_LSM) > > + bpf_trampoline_toggle_lsm(tr, kind); > > + > > return bpf_trampoline_update(tr, true /* lock_direct_mutex */); > > } > > > > diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c > > index cfaf1d0e6a5f..47e1a4777ec9 100644 > > --- a/security/bpf/hooks.c > > +++ b/security/bpf/hooks.c > > @@ -8,7 +8,7 @@ > > > > static struct security_hook_list bpf_lsm_hooks[] __ro_after_init = { > > #define LSM_HOOK(RET, DEFAULT, NAME, ...) \ > > - LSM_HOOK_INIT(NAME, bpf_lsm_##NAME), > > + LSM_HOOK_INIT_DISABLED(NAME, bpf_lsm_##NAME), > > #include <linux/lsm_hook_defs.h> > > #undef LSM_HOOK > > LSM_HOOK_INIT(inode_free_security, bpf_inode_storage_free), > > @@ -32,3 +32,26 @@ DEFINE_LSM(bpf) = { > > .init = bpf_lsm_init, > > .blobs = &bpf_lsm_blob_sizes > > }; > > + > > +void bpf_lsm_toggle_hook(void *addr, bool value) > > +{ > > + struct lsm_static_call *scalls; > > + struct security_hook_list *h; > > + int i, j; > > + > > + for (i = 0; i < ARRAY_SIZE(bpf_lsm_hooks); i++) { > > + h = &bpf_lsm_hooks[i]; > > + if (h->hook.lsm_callback != addr) > > + continue; > > + > > + for (j = 0; j < MAX_LSM_COUNT; j++) { > > + scalls = &h->scalls[j]; > > + if (scalls->hl != &bpf_lsm_hooks[i]) > > + continue; > > + if (value) > > + static_branch_enable(scalls->active); > > + else > > + static_branch_disable(scalls->active); > > + } > > + } > > +} > > diff --git a/security/security.c b/security/security.c > > index ce4c0a9107ea..f45e875b6d93 100644 > > --- a/security/security.c > > +++ b/security/security.c > > @@ -382,7 +382,8 @@ static void __init lsm_static_call_init(struct security_hook_list *hl) > > __static_call_update(scall->key, scall->trampoline, > > hl->hook.lsm_callback); > > scall->hl = hl; > > - static_branch_enable(scall->active); > > + if (hl->default_state) > > + static_branch_enable(scall->active); > > return; > > } > > scall++; > > -- > > 2.42.0.609.gbb76f46606-goog > > > >
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
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); static inline struct bpf_storage_blob *bpf_inode( const struct inode *inode) @@ -78,6 +79,10 @@ static inline void bpf_lsm_find_cgroup_shim(const struct bpf_prog *prog, { } +static inline void bpf_lsm_toggle_hook(void *addr, bool value) +{ +} + #endif /* CONFIG_BPF_LSM */ #endif /* _LINUX_BPF_LSM_H */ diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index c77a1859214d..57ffe4eb6d30 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -97,11 +97,14 @@ struct lsm_static_calls_table { * @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 char *lsm; + bool default_state; } __randomize_layout; /* @@ -151,7 +154,15 @@ static inline struct xattr *lsm_get_xattr_slot(struct xattr *xattrs, #define LSM_HOOK_INIT(NAME, CALLBACK) \ { \ .scalls = static_calls_table.NAME, \ - .hook = { .NAME = CALLBACK } \ + .hook = { .NAME = CALLBACK }, \ + .default_state = true \ + } + +#define LSM_HOOK_INIT_DISABLED(NAME, CALLBACK) \ + { \ + .scalls = static_calls_table.NAME, \ + .hook = { .NAME = CALLBACK }, \ + .default_state = false \ } extern char *lsm_names; diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c index e97aeda3a86b..44788e2eaa1b 100644 --- a/kernel/bpf/trampoline.c +++ b/kernel/bpf/trampoline.c @@ -13,6 +13,7 @@ #include <linux/bpf_verifier.h> #include <linux/bpf_lsm.h> #include <linux/delay.h> +#include <linux/bpf_lsm.h> /* dummy _ops. The verifier will operate on target program's ops. */ const struct bpf_verifier_ops bpf_extension_verifier_ops = { @@ -510,6 +511,21 @@ static enum bpf_tramp_prog_type bpf_attach_type_to_tramp(struct bpf_prog *prog) } } +static void bpf_trampoline_toggle_lsm(struct bpf_trampoline *tr, + enum bpf_tramp_prog_type kind) +{ + struct bpf_tramp_link *link; + bool found = false; + + hlist_for_each_entry(link, &tr->progs_hlist[kind], tramp_hlist) { + if (link->link.prog->type == BPF_PROG_TYPE_LSM) { + found = true; + break; + } + } + bpf_lsm_toggle_hook(tr->func.addr, found); +} + static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_trampoline *tr) { enum bpf_tramp_prog_type kind; @@ -549,6 +565,10 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_tr hlist_add_head(&link->tramp_hlist, &tr->progs_hlist[kind]); tr->progs_cnt[kind]++; + + if (link->link.prog->type == BPF_PROG_TYPE_LSM) + bpf_trampoline_toggle_lsm(tr, kind); + err = bpf_trampoline_update(tr, true /* lock_direct_mutex */); if (err) { hlist_del_init(&link->tramp_hlist); @@ -582,6 +602,10 @@ static int __bpf_trampoline_unlink_prog(struct bpf_tramp_link *link, struct bpf_ } hlist_del_init(&link->tramp_hlist); tr->progs_cnt[kind]--; + + if (link->link.prog->type == BPF_PROG_TYPE_LSM) + bpf_trampoline_toggle_lsm(tr, kind); + return bpf_trampoline_update(tr, true /* lock_direct_mutex */); } diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c index cfaf1d0e6a5f..47e1a4777ec9 100644 --- a/security/bpf/hooks.c +++ b/security/bpf/hooks.c @@ -8,7 +8,7 @@ static struct security_hook_list bpf_lsm_hooks[] __ro_after_init = { #define LSM_HOOK(RET, DEFAULT, NAME, ...) \ - LSM_HOOK_INIT(NAME, bpf_lsm_##NAME), + LSM_HOOK_INIT_DISABLED(NAME, bpf_lsm_##NAME), #include <linux/lsm_hook_defs.h> #undef LSM_HOOK LSM_HOOK_INIT(inode_free_security, bpf_inode_storage_free), @@ -32,3 +32,26 @@ DEFINE_LSM(bpf) = { .init = bpf_lsm_init, .blobs = &bpf_lsm_blob_sizes }; + +void bpf_lsm_toggle_hook(void *addr, bool value) +{ + struct lsm_static_call *scalls; + struct security_hook_list *h; + int i, j; + + for (i = 0; i < ARRAY_SIZE(bpf_lsm_hooks); i++) { + h = &bpf_lsm_hooks[i]; + if (h->hook.lsm_callback != addr) + continue; + + for (j = 0; j < MAX_LSM_COUNT; j++) { + scalls = &h->scalls[j]; + if (scalls->hl != &bpf_lsm_hooks[i]) + continue; + if (value) + static_branch_enable(scalls->active); + else + static_branch_disable(scalls->active); + } + } +} diff --git a/security/security.c b/security/security.c index ce4c0a9107ea..f45e875b6d93 100644 --- a/security/security.c +++ b/security/security.c @@ -382,7 +382,8 @@ static void __init lsm_static_call_init(struct security_hook_list *hl) __static_call_update(scall->key, scall->trampoline, hl->hook.lsm_callback); scall->hl = hl; - static_branch_enable(scall->active); + if (hl->default_state) + static_branch_enable(scall->active); return; } scall++;