| Message ID | 20240629084331.3807368-4-kpsingh@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | Reduce overhead of LSMs with static calls | expand |
On Jun 29, 2024 KP Singh <kpsingh@kernel.org> wrote: > > LSM hooks are currently invoked from a linked list as indirect calls > which are invoked using retpolines as a mitigation for speculative > attacks (Branch History / Target injection) and add extra overhead which > is especially bad in kernel hot paths: > > security_file_ioctl: > 0xff...0320 <+0>: endbr64 > 0xff...0324 <+4>: push %rbp > 0xff...0325 <+5>: push %r15 > 0xff...0327 <+7>: push %r14 > 0xff...0329 <+9>: push %rbx > 0xff...032a <+10>: mov %rdx,%rbx > 0xff...032d <+13>: mov %esi,%ebp > 0xff...032f <+15>: mov %rdi,%r14 > 0xff...0332 <+18>: mov $0xff...7030,%r15 > 0xff...0339 <+25>: mov (%r15),%r15 > 0xff...033c <+28>: test %r15,%r15 > 0xff...033f <+31>: je 0xff...0358 <security_file_ioctl+56> > 0xff...0341 <+33>: mov 0x18(%r15),%r11 > 0xff...0345 <+37>: mov %r14,%rdi > 0xff...0348 <+40>: mov %ebp,%esi > 0xff...034a <+42>: mov %rbx,%rdx > > 0xff...034d <+45>: call 0xff...2e0 <__x86_indirect_thunk_array+352> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > Indirect calls that use retpolines leading to overhead, not just due > to extra instruction but also branch misses. > > 0xff...0352 <+50>: test %eax,%eax > 0xff...0354 <+52>: je 0xff...0339 <security_file_ioctl+25> > 0xff...0356 <+54>: jmp 0xff...035a <security_file_ioctl+58> > 0xff...0358 <+56>: xor %eax,%eax > 0xff...035a <+58>: pop %rbx > 0xff...035b <+59>: pop %r14 > 0xff...035d <+61>: pop %r15 > 0xff...035f <+63>: pop %rbp > 0xff...0360 <+64>: jmp 0xff...47c4 <__x86_return_thunk> > > The indirect calls are not really needed as one knows the addresses of > enabled LSM callbacks at boot time and only the order can possibly > change at boot time with the lsm= kernel command line parameter. > > An array of static calls is defined per LSM hook and the static calls > are updated at boot time once the order has been determined. > > A static key guards whether an LSM static call is enabled or not, > without this static key, for LSM hooks that return an int, the presence > of the hook that returns a default value can create side-effects which > has resulted in bugs [1]. I believe there have been improvements in the LSM framework since the original problems were reported and I'm not entirely sure if this is still a problem. In particular I believe that commit 260017f31a8c ("lsm: use default hook return value in call_int_hook()") and 61df7b828204 ("lsm: fixup the inode xattr capability handling") should fix the more obvious problems. I'd like to know if you are aware of any others? If not, the text above should be adjusted and we should reconsider patch 5/5. If there are other problems I'd like to better understand them as there may be an independent solution for those particular problems. > With the hook now exposed as a static call, one can see that the > retpolines are no longer there and the LSM callbacks are invoked > directly: > > security_file_ioctl: > 0xff...0ca0 <+0>: endbr64 > 0xff...0ca4 <+4>: nopl 0x0(%rax,%rax,1) > 0xff...0ca9 <+9>: push %rbp > 0xff...0caa <+10>: push %r14 > 0xff...0cac <+12>: push %rbx > 0xff...0cad <+13>: mov %rdx,%rbx > 0xff...0cb0 <+16>: mov %esi,%ebp > 0xff...0cb2 <+18>: mov %rdi,%r14 > 0xff...0cb5 <+21>: jmp 0xff...0cc7 <security_file_ioctl+39> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > Static key enabled for SELinux > > 0xffffffff818f0cb7 <+23>: jmp 0xff...0cde <security_file_ioctl+62> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > Static key enabled for BPF LSM. This is something that is changed to > default to false to avoid the existing side effect issues of BPF LSM > [1] in a subsequent patch. > > 0xff...0cb9 <+25>: xor %eax,%eax > 0xff...0cbb <+27>: xchg %ax,%ax > 0xff...0cbd <+29>: pop %rbx > 0xff...0cbe <+30>: pop %r14 > 0xff...0cc0 <+32>: pop %rbp > 0xff...0cc1 <+33>: cs jmp 0xff...0000 <__x86_return_thunk> > 0xff...0cc7 <+39>: endbr64 > 0xff...0ccb <+43>: mov %r14,%rdi > 0xff...0cce <+46>: mov %ebp,%esi > 0xff...0cd0 <+48>: mov %rbx,%rdx > 0xff...0cd3 <+51>: call 0xff...3230 <selinux_file_ioctl> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > Direct call to SELinux. > > 0xff...0cd8 <+56>: test %eax,%eax > 0xff...0cda <+58>: jne 0xff...0cbd <security_file_ioctl+29> > 0xff...0cdc <+60>: jmp 0xff...0cb7 <security_file_ioctl+23> > 0xff...0cde <+62>: endbr64 > 0xff...0ce2 <+66>: mov %r14,%rdi > 0xff...0ce5 <+69>: mov %ebp,%esi > 0xff...0ce7 <+71>: mov %rbx,%rdx > 0xff...0cea <+74>: call 0xff...e220 <bpf_lsm_file_ioctl> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > Direct call to BPF LSM. > > 0xff...0cef <+79>: test %eax,%eax > 0xff...0cf1 <+81>: jne 0xff...0cbd <security_file_ioctl+29> > 0xff...0cf3 <+83>: jmp 0xff...0cb9 <security_file_ioctl+25> > 0xff...0cf5 <+85>: endbr64 > 0xff...0cf9 <+89>: mov %r14,%rdi > 0xff...0cfc <+92>: mov %ebp,%esi > 0xff...0cfe <+94>: mov %rbx,%rdx > 0xff...0d01 <+97>: pop %rbx > 0xff...0d02 <+98>: pop %r14 > 0xff...0d04 <+100>: pop %rbp > 0xff...0d05 <+101>: ret > 0xff...0d06 <+102>: int3 > 0xff...0d07 <+103>: int3 > 0xff...0d08 <+104>: int3 > 0xff...0d09 <+105>: int3 > > While this patch uses static_branch_unlikely indicating that an LSM hook > is likely to be not present. In most cases this is still a better choice > as even when an LSM with one hook is added, empty slots are created for > all LSM hooks (especially when many LSMs that do not initialize most > hooks are present on the system). > > There are some hooks that don't use the call_int_hook and > call_void_hook. I think you mean "or" and not "and", yes? > These hooks are updated to use a new macro called > lsm_for_each_hook where the lsm_callback is directly invoked as an > indirect call. These are updated in a subsequent patch to also use > static calls. > > Below are results of the relevant Unixbench system benchmarks with BPF LSM > and SELinux enabled with default policies enabled with and without these > patches. > > Benchmark Delta(%): (+ is better) > =============================================================================== > Execl Throughput +1.9356 > File Write 1024 bufsize 2000 maxblocks +6.5953 > Pipe Throughput +9.5499 > Pipe-based Context Switching +3.0209 > Process Creation +2.3246 > Shell Scripts (1 concurrent) +1.4975 > System Call Overhead +2.7815 > System Benchmarks Index Score (Partial Only): +3.4859 > > In the best case, some syscalls like eventfd_create benefitted to about ~10%. > > [1] https://lore.kernel.org/linux-security-module/20220609234601.2026362-1-kpsingh@kernel.org/ > > Reviewed-by: Casey Schaufler <casey@schaufler-ca.com> > Reviewed-by: Kees Cook <keescook@chromium.org> > Acked-by: Song Liu <song@kernel.org> > Acked-by: Andrii Nakryiko <andrii@kernel.org> > Signed-off-by: KP Singh <kpsingh@kernel.org> > --- > include/linux/lsm_hooks.h | 72 ++++++++++++-- > security/security.c | 198 ++++++++++++++++++++++++++------------ > 2 files changed, 197 insertions(+), 73 deletions(-) ... > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index efd4a0655159..a66ca68485a2 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -30,19 +30,66 @@ > #include <linux/init.h> > #include <linux/rculist.h> > #include <linux/xattr.h> > +#include <linux/static_call.h> > +#include <linux/unroll.h> > +#include <linux/jump_label.h> > +#include <linux/lsm_count.h> > + > +#define SECURITY_HOOK_ACTIVE_KEY(HOOK, IDX) security_hook_active_##HOOK##_##IDX > + > +/* > + * Identifier for the LSM static calls. > + * HOOK is an LSM hook as defined in linux/lsm_hookdefs.h > + * IDX is the index of the static call. 0 <= NUM < MAX_LSM_COUNT > + */ > +#define LSM_STATIC_CALL(HOOK, IDX) lsm_static_call_##HOOK##_##IDX > + > +/* > + * Call the macro M for each LSM hook MAX_LSM_COUNT times. > + */ > +#define LSM_LOOP_UNROLL(M, ...) \ > +do { \ > + UNROLL(MAX_LSM_COUNT, M, __VA_ARGS__) \ > +} while (0) > + > +#define LSM_DEFINE_UNROLL(M, ...) UNROLL(MAX_LSM_COUNT, M, __VA_ARGS__) All of the macros above (SECURITY_HOOK_ACTIVE_KEY, LSM_STATIC_CALL, LSM_LOOP_UNROLL, and LSM_DEFINE_UNROLL) are only used in security/security.c, yes? If so, is there a reason why they are defined here and not in security/security.c? > diff --git a/security/security.c b/security/security.c > index 9c3fb2f60e2a..e0ec185cf125 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -112,6 +113,51 @@ static __initconst const char *const builtin_lsm_order = CONFIG_LSM; > static __initdata struct lsm_info **ordered_lsms; > static __initdata struct lsm_info *exclusive; > > + > +#ifdef CONFIG_HAVE_STATIC_CALL > +#define LSM_HOOK_TRAMP(NAME, NUM) \ > + &STATIC_CALL_TRAMP(LSM_STATIC_CALL(NAME, NUM)) > +#else > +#define LSM_HOOK_TRAMP(NAME, NUM) NULL > +#endif > + > +/* > + * Define static calls and static keys for each LSM hook. > + */ > + > +#define DEFINE_LSM_STATIC_CALL(NUM, NAME, RET, ...) \ > + DEFINE_STATIC_CALL_NULL(LSM_STATIC_CALL(NAME, NUM), \ > + *((RET(*)(__VA_ARGS__))NULL)); \ > + DEFINE_STATIC_KEY_FALSE(SECURITY_HOOK_ACTIVE_KEY(NAME, NUM)); > + > +#define LSM_HOOK(RET, DEFAULT, NAME, ...) \ > + LSM_DEFINE_UNROLL(DEFINE_LSM_STATIC_CALL, NAME, RET, __VA_ARGS__) > +#include <linux/lsm_hook_defs.h> > +#undef LSM_HOOK > +#undef DEFINE_LSM_STATIC_CALL If you end up respinning the patchset, drop the two blank lines before the DEFINE_LSM_STATIC_CALL and LSM_HOOK definitions. > @@ -856,29 +916,43 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u32 *uctx_len, > * call_int_hook: > * This is a hook that returns a value. > */ > +#define __CALL_STATIC_VOID(NUM, HOOK, ...) \ > +do { \ > + if (static_branch_unlikely(&SECURITY_HOOK_ACTIVE_KEY(HOOK, NUM))) { \ > + static_call(LSM_STATIC_CALL(HOOK, NUM))(__VA_ARGS__); \ > + } \ > +} while (0); > > -#define call_void_hook(FUNC, ...) \ > - do { \ > - struct security_hook_list *P; \ > - \ > - hlist_for_each_entry(P, &security_hook_heads.FUNC, list) \ > - P->hook.FUNC(__VA_ARGS__); \ > +#define call_void_hook(HOOK, ...) \ > + do { \ > + LSM_LOOP_UNROLL(__CALL_STATIC_VOID, HOOK, __VA_ARGS__); \ > } while (0) > > -#define call_int_hook(FUNC, ...) ({ \ > - int RC = LSM_RET_DEFAULT(FUNC); \ > - do { \ > - struct security_hook_list *P; \ > - \ > - hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \ > - RC = P->hook.FUNC(__VA_ARGS__); \ > - if (RC != LSM_RET_DEFAULT(FUNC)) \ > - break; \ > - } \ > - } while (0); \ > - RC; \ > + > +#define __CALL_STATIC_INT(NUM, R, HOOK, LABEL, ...) \ > +do { \ > + if (static_branch_unlikely(&SECURITY_HOOK_ACTIVE_KEY(HOOK, NUM))) { \ > + R = static_call(LSM_STATIC_CALL(HOOK, NUM))(__VA_ARGS__); \ > + if (R != LSM_RET_DEFAULT(HOOK)) \ > + goto LABEL; \ > + } \ > +} while (0); > + > +#define call_int_hook(HOOK, ...) \ > +({ \ > + __label__ out; \ This is another only-if-you-respin, consider /out/OUT/ so it is more consistent. > + int RC = LSM_RET_DEFAULT(HOOK); \ > + \ > + LSM_LOOP_UNROLL(__CALL_STATIC_INT, RC, HOOK, out, __VA_ARGS__); \ > +out: \ > + RC; \ > }) > > +#define lsm_for_each_hook(scall, NAME) \ > + for (scall = static_calls_table.NAME; \ > + scall - static_calls_table.NAME < MAX_LSM_COUNT; scall++) \ > + if (static_key_enabled(&scall->active->key)) This is probably a stupid question, but why use static_key_enabled() here instead of static_branch_unlikely() as in the call_XXX_macros? -- paul-moore.com
On Wed, Jul 3, 2024 at 2:07 AM Paul Moore <paul@paul-moore.com> wrote: > > On Jun 29, 2024 KP Singh <kpsingh@kernel.org> wrote: > > > > LSM hooks are currently invoked from a linked list as indirect calls > > which are invoked using retpolines as a mitigation for speculative > > attacks (Branch History / Target injection) and add extra overhead which > > is especially bad in kernel hot paths: [...] > should fix the more obvious problems. I'd like to know if you are > aware of any others? If not, the text above should be adjusted and > we should reconsider patch 5/5. If there are other problems I'd > like to better understand them as there may be an independent > solution for those particular problems. We did have problems with some other hooks but I was unable to dig up specific examples though, it's been a while. More broadly speaking, a default decision is still a decision. Whereas the intent from the BPF LSM is not to make a default decision unless a BPF program is loaded. I am quite worried about the holes this leaves open, subtle bugs (security or crashes) we have not caught yet and PATCH 5/5 engineers away the problem of the "default decision". > > > With the hook now exposed as a static call, one can see that the > > retpolines are no longer there and the LSM callbacks are invoked > > directly: > > > > security_file_ioctl: > > 0xff...0ca0 <+0>: endbr64 > > 0xff...0ca4 <+4>: nopl 0x0(%rax,%rax,1) > > 0xff...0ca9 <+9>: push %rbp > > 0xff...0caa <+10>: push %r14 > > 0xff...0cac <+12>: push %rbx > > 0xff...0cad <+13>: mov %rdx,%rbx > > 0xff...0cb0 <+16>: mov %esi,%ebp > > 0xff...0cb2 <+18>: mov %rdi,%r14 > > 0xff...0cb5 <+21>: jmp 0xff...0cc7 <security_file_ioctl+39> > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > Static key enabled for SELinux > > > > 0xffffffff818f0cb7 <+23>: jmp 0xff...0cde <security_file_ioctl+62> > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > > Static key enabled for BPF LSM. This is something that is changed to > > default to false to avoid the existing side effect issues of BPF LSM > > [1] in a subsequent patch. > > > > 0xff...0cb9 <+25>: xor %eax,%eax > > 0xff...0cbb <+27>: xchg %ax,%ax > > 0xff...0cbd <+29>: pop %rbx > > 0xff...0cbe <+30>: pop %r14 > > 0xff...0cc0 <+32>: pop %rbp > > 0xff...0cc1 <+33>: cs jmp 0xff...0000 <__x86_return_thunk> > > 0xff...0cc7 <+39>: endbr64 > > 0xff...0ccb <+43>: mov %r14,%rdi > > 0xff...0cce <+46>: mov %ebp,%esi > > 0xff...0cd0 <+48>: mov %rbx,%rdx > > 0xff...0cd3 <+51>: call 0xff...3230 <selinux_file_ioctl> > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > Direct call to SELinux. > > > > 0xff...0cd8 <+56>: test %eax,%eax > > 0xff...0cda <+58>: jne 0xff...0cbd <security_file_ioctl+29> > > 0xff...0cdc <+60>: jmp 0xff...0cb7 <security_file_ioctl+23> > > 0xff...0cde <+62>: endbr64 > > 0xff...0ce2 <+66>: mov %r14,%rdi > > 0xff...0ce5 <+69>: mov %ebp,%esi > > 0xff...0ce7 <+71>: mov %rbx,%rdx > > 0xff...0cea <+74>: call 0xff...e220 <bpf_lsm_file_ioctl> > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > Direct call to BPF LSM. > > > > 0xff...0cef <+79>: test %eax,%eax > > 0xff...0cf1 <+81>: jne 0xff...0cbd <security_file_ioctl+29> > > 0xff...0cf3 <+83>: jmp 0xff...0cb9 <security_file_ioctl+25> > > 0xff...0cf5 <+85>: endbr64 > > 0xff...0cf9 <+89>: mov %r14,%rdi > > 0xff...0cfc <+92>: mov %ebp,%esi > > 0xff...0cfe <+94>: mov %rbx,%rdx > > 0xff...0d01 <+97>: pop %rbx > > 0xff...0d02 <+98>: pop %r14 > > 0xff...0d04 <+100>: pop %rbp > > 0xff...0d05 <+101>: ret > > 0xff...0d06 <+102>: int3 > > 0xff...0d07 <+103>: int3 > > 0xff...0d08 <+104>: int3 > > 0xff...0d09 <+105>: int3 > > > > While this patch uses static_branch_unlikely indicating that an LSM hook > > is likely to be not present. In most cases this is still a better choice > > as even when an LSM with one hook is added, empty slots are created for > > all LSM hooks (especially when many LSMs that do not initialize most > > hooks are present on the system). > > > > There are some hooks that don't use the call_int_hook and > > call_void_hook. > > I think you mean "or" and not "and", yes? Yep, thanks, fixed. > > > These hooks are updated to use a new macro called > > lsm_for_each_hook where the lsm_callback is directly invoked as an > > indirect call. These are updated in a subsequent patch to also use > > static calls. > > > > Below are results of the relevant Unixbench system benchmarks with BPF LSM > > and SELinux enabled with default policies enabled with and without these > > patches. > > > > Benchmark Delta(%): (+ is better) > > =============================================================================== > > Execl Throughput +1.9356 > > File Write 1024 bufsize 2000 maxblocks +6.5953 > > Pipe Throughput +9.5499 > > Pipe-based Context Switching +3.0209 > > Process Creation +2.3246 > > Shell Scripts (1 concurrent) +1.4975 > > System Call Overhead +2.7815 > > System Benchmarks Index Score (Partial Only): +3.4859 > > > > In the best case, some syscalls like eventfd_create benefitted to about ~10%. > > > > [1] https://lore.kernel.org/linux-security-module/20220609234601.2026362-1-kpsingh@kernel.org/ > > > > Reviewed-by: Casey Schaufler <casey@schaufler-ca.com> > > Reviewed-by: Kees Cook <keescook@chromium.org> > > Acked-by: Song Liu <song@kernel.org> > > Acked-by: Andrii Nakryiko <andrii@kernel.org> > > Signed-off-by: KP Singh <kpsingh@kernel.org> > > --- > > include/linux/lsm_hooks.h | 72 ++++++++++++-- > > security/security.c | 198 ++++++++++++++++++++++++++------------ > > 2 files changed, 197 insertions(+), 73 deletions(-) > > ... > > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > > index efd4a0655159..a66ca68485a2 100644 > > --- a/include/linux/lsm_hooks.h > > +++ b/include/linux/lsm_hooks.h > > @@ -30,19 +30,66 @@ > > #include <linux/init.h> > > #include <linux/rculist.h> > > #include <linux/xattr.h> > > +#include <linux/static_call.h> > > +#include <linux/unroll.h> > > +#include <linux/jump_label.h> > > +#include <linux/lsm_count.h> > > + > > +#define SECURITY_HOOK_ACTIVE_KEY(HOOK, IDX) security_hook_active_##HOOK##_##IDX > > + > > +/* > > + * Identifier for the LSM static calls. > > + * HOOK is an LSM hook as defined in linux/lsm_hookdefs.h > > + * IDX is the index of the static call. 0 <= NUM < MAX_LSM_COUNT > > + */ > > +#define LSM_STATIC_CALL(HOOK, IDX) lsm_static_call_##HOOK##_##IDX > > + > > +/* > > + * Call the macro M for each LSM hook MAX_LSM_COUNT times. > > + */ > > +#define LSM_LOOP_UNROLL(M, ...) \ > > +do { \ > > + UNROLL(MAX_LSM_COUNT, M, __VA_ARGS__) \ > > +} while (0) > > + > > +#define LSM_DEFINE_UNROLL(M, ...) UNROLL(MAX_LSM_COUNT, M, __VA_ARGS__) > > All of the macros above (SECURITY_HOOK_ACTIVE_KEY, LSM_STATIC_CALL, > LSM_LOOP_UNROLL, and LSM_DEFINE_UNROLL) are only used in > security/security.c, yes? If so, is there a reason why they are > defined here and not in security/security.c? Fair point, fixed. > > > diff --git a/security/security.c b/security/security.c > > index 9c3fb2f60e2a..e0ec185cf125 100644 > > --- a/security/security.c > > +++ b/security/security.c > > @@ -112,6 +113,51 @@ static __initconst const char *const builtin_lsm_order = CONFIG_LSM; > > static __initdata struct lsm_info **ordered_lsms; > > static __initdata struct lsm_info *exclusive; > > > > + > > +#ifdef CONFIG_HAVE_STATIC_CALL > > +#define LSM_HOOK_TRAMP(NAME, NUM) \ > > + &STATIC_CALL_TRAMP(LSM_STATIC_CALL(NAME, NUM)) > > +#else > > +#define LSM_HOOK_TRAMP(NAME, NUM) NULL > > +#endif > > + > > +/* > > + * Define static calls and static keys for each LSM hook. > > + */ > > + > > +#define DEFINE_LSM_STATIC_CALL(NUM, NAME, RET, ...) \ > > + DEFINE_STATIC_CALL_NULL(LSM_STATIC_CALL(NAME, NUM), \ > > + *((RET(*)(__VA_ARGS__))NULL)); \ > > + DEFINE_STATIC_KEY_FALSE(SECURITY_HOOK_ACTIVE_KEY(NAME, NUM)); > > + > > +#define LSM_HOOK(RET, DEFAULT, NAME, ...) \ > > + LSM_DEFINE_UNROLL(DEFINE_LSM_STATIC_CALL, NAME, RET, __VA_ARGS__) > > +#include <linux/lsm_hook_defs.h> > > +#undef LSM_HOOK > > +#undef DEFINE_LSM_STATIC_CALL > > If you end up respinning the patchset, drop the two blank lines > before the DEFINE_LSM_STATIC_CALL and LSM_HOOK definitions. Done. > > > @@ -856,29 +916,43 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u32 *uctx_len, > > * call_int_hook: > > * This is a hook that returns a value. > > */ > > +#define __CALL_STATIC_VOID(NUM, HOOK, ...) \ > > +do { \ > > + if (static_branch_unlikely(&SECURITY_HOOK_ACTIVE_KEY(HOOK, NUM))) { \ > > + static_call(LSM_STATIC_CALL(HOOK, NUM))(__VA_ARGS__); \ > > + } \ > > +} while (0); > > > > -#define call_void_hook(FUNC, ...) \ > > - do { \ > > - struct security_hook_list *P; \ > > - \ > > - hlist_for_each_entry(P, &security_hook_heads.FUNC, list) \ > > - P->hook.FUNC(__VA_ARGS__); \ > > +#define call_void_hook(HOOK, ...) \ > > + do { \ > > + LSM_LOOP_UNROLL(__CALL_STATIC_VOID, HOOK, __VA_ARGS__); \ > > } while (0) > > > > -#define call_int_hook(FUNC, ...) ({ \ > > - int RC = LSM_RET_DEFAULT(FUNC); \ > > - do { \ > > - struct security_hook_list *P; \ > > - \ > > - hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \ > > - RC = P->hook.FUNC(__VA_ARGS__); \ > > - if (RC != LSM_RET_DEFAULT(FUNC)) \ > > - break; \ > > - } \ > > - } while (0); \ > > - RC; \ > > + > > +#define __CALL_STATIC_INT(NUM, R, HOOK, LABEL, ...) \ > > +do { \ > > + if (static_branch_unlikely(&SECURITY_HOOK_ACTIVE_KEY(HOOK, NUM))) { \ > > + R = static_call(LSM_STATIC_CALL(HOOK, NUM))(__VA_ARGS__); \ > > + if (R != LSM_RET_DEFAULT(HOOK)) \ > > + goto LABEL; \ > > + } \ > > +} while (0); > > + > > +#define call_int_hook(HOOK, ...) \ > > +({ \ > > + __label__ out; \ > > This is another only-if-you-respin, consider /out/OUT/ so it is more > consistent. Done. I will do a re-spin as we do have some fixes. > > > + int RC = LSM_RET_DEFAULT(HOOK); \ > > + \ > > + LSM_LOOP_UNROLL(__CALL_STATIC_INT, RC, HOOK, out, __VA_ARGS__); \ > > +out: \ > > + RC; \ > > }) > > > > +#define lsm_for_each_hook(scall, NAME) \ > > + for (scall = static_calls_table.NAME; \ > > + scall - static_calls_table.NAME < MAX_LSM_COUNT; scall++) \ > > + if (static_key_enabled(&scall->active->key)) > > This is probably a stupid question, but why use static_key_enabled() > here instead of static_branch_unlikely() as in the call_XXX_macros? The static_key_enabled is a check for the key being enabled, whereas the static_branch_likely is for laying out the right assembly code (jump tables etc.) and based on the value of the static key, here we are not using the static calls or even keys, rather we are following back from direct calls to indirect calls and don't really need any jump tables in the slow path. We also get rid of this macro in the subsequent patch, but we might need to keep it around if we leave security_getselfattr alone. - KP > > -- > paul-moore.com
On Wed, Jul 3, 2024 at 12:55 PM KP Singh <kpsingh@kernel.org> wrote: > On Wed, Jul 3, 2024 at 2:07 AM Paul Moore <paul@paul-moore.com> wrote: > > > > On Jun 29, 2024 KP Singh <kpsingh@kernel.org> wrote: > > > > > > LSM hooks are currently invoked from a linked list as indirect calls > > > which are invoked using retpolines as a mitigation for speculative > > > attacks (Branch History / Target injection) and add extra overhead which > > > is especially bad in kernel hot paths: > > [...] > > > should fix the more obvious problems. I'd like to know if you are > > aware of any others? If not, the text above should be adjusted and > > we should reconsider patch 5/5. If there are other problems I'd > > like to better understand them as there may be an independent > > solution for those particular problems. > > We did have problems with some other hooks but I was unable to dig up > specific examples though, it's been a while. More broadly speaking, a > default decision is still a decision. Whereas the intent from the BPF > LSM is not to make a default decision unless a BPF program is loaded. > I am quite worried about the holes this leaves open, subtle bugs > (security or crashes) we have not caught yet and PATCH 5/5 engineers away > the problem of the "default decision". The inode/xattr problem you originally mentioned wasn't really rooted in a "bad" default return value, it was really an issue with how the LSM hook was structured due to some legacy design assumptions made well before the initial stacking patches were merged. That should be fixed now[1] and given that the inode/xattr set/remove hooks were unique in this regard (individual LSMs were responsible for performing the capabilities checks) I don't expect this to be a general problem. There were also some issues caused by the fact that we were defining the default return value in multiple places and these values had gone out of sync in a number of hooks. We've also fixed this problem by only defining the default return value once for each hook, solving all of those problems. I'm not aware of any other existing problems relating to the LSM hook default values, if there are any, we need to fix them independent of this patchset. The LSM framework should function properly if the "default" values are used. [1] I just realized that commit 61df7b828204 doesn't properly update the removexattr implementations for SELinux and Smack, expect a patch for that soon. The current code is okay, it just does some unnecessary work (see the setxattr changes to get an idea of the changes needed). > > > +#define lsm_for_each_hook(scall, NAME) \ > > > + for (scall = static_calls_table.NAME; \ > > > + scall - static_calls_table.NAME < MAX_LSM_COUNT; scall++) \ > > > + if (static_key_enabled(&scall->active->key)) > > > > This is probably a stupid question, but why use static_key_enabled() > > here instead of static_branch_unlikely() as in the call_XXX_macros? > > The static_key_enabled is a check for the key being enabled, whereas > the static_branch_likely is for laying out the right assembly code > (jump tables etc.) and based on the value of the static key, here we > are not using the static calls or even keys, rather we are following > back from direct calls to indirect calls and don't really need any > jump tables in the slow path. Gotcha, thanks for the explanation.
On Wed, Jul 3, 2024 at 10:56 PM Paul Moore <paul@paul-moore.com> wrote: > > On Wed, Jul 3, 2024 at 12:55 PM KP Singh <kpsingh@kernel.org> wrote: > > On Wed, Jul 3, 2024 at 2:07 AM Paul Moore <paul@paul-moore.com> wrote: > > > > > > On Jun 29, 2024 KP Singh <kpsingh@kernel.org> wrote: > > > > > > > > LSM hooks are currently invoked from a linked list as indirect calls > > > > which are invoked using retpolines as a mitigation for speculative > > > > attacks (Branch History / Target injection) and add extra overhead which > > > > is especially bad in kernel hot paths: > > > > [...] > > > > > should fix the more obvious problems. I'd like to know if you are > > > aware of any others? If not, the text above should be adjusted and > > > we should reconsider patch 5/5. If there are other problems I'd > > > like to better understand them as there may be an independent > > > solution for those particular problems. > > > > We did have problems with some other hooks but I was unable to dig up > > specific examples though, it's been a while. More broadly speaking, a > > default decision is still a decision. Whereas the intent from the BPF > > LSM is not to make a default decision unless a BPF program is loaded. > > I am quite worried about the holes this leaves open, subtle bugs > > (security or crashes) we have not caught yet and PATCH 5/5 engineers away > > the problem of the "default decision". > > The inode/xattr problem you originally mentioned wasn't really rooted > in a "bad" default return value, it was really an issue with how the > LSM hook was structured due to some legacy design assumptions made > well before the initial stacking patches were merged. That should be > fixed now[1] and given that the inode/xattr set/remove hooks were > unique in this regard (individual LSMs were responsible for performing > the capabilities checks) I don't expect this to be a general problem. > > There were also some issues caused by the fact that we were defining > the default return value in multiple places and these values had gone > out of sync in a number of hooks. We've also fixed this problem by > only defining the default return value once for each hook, solving all > of those problems. I don't see how this solves problems or prevents any future problems with side-effects. I have always been uncomfortable with an extraneous function being called with a side effect ever since we merged BPF LSM with default callback. We have found one bug due to this, not all the bugs. > > I'm not aware of any other existing problems relating to the LSM hook > default values, if there are any, we need to fix them independent of > this patchset. The LSM framework should function properly if the > "default" values are used. Patch 5 eliminates the possibilities of errors and subtle bugs all together. The problem with subtle bugs is, well, they are subtle, if you and I knew of the bugs, we would fix all of them, but we don't. I really feel we ought to eliminate the class of issues and not just whack-a-mole when we see the bugs. The LSM framework ought to function with default values is a nice goal, but a weaker position than what we have where we make these subtle bugs impossible. If you feel this should not be a part of the framework, I would still like to revert back to implementation where we kept the toggling logic contained to the BPF LSM. - KP > > [1] I just realized that commit 61df7b828204 doesn't properly update > the removexattr implementations for SELinux and Smack, expect a patch > for that soon. The current code is okay, it just does some > unnecessary work (see the setxattr changes to get an idea of the > changes needed). > > > > > +#define lsm_for_each_hook(scall, NAME) \ > > > > + for (scall = static_calls_table.NAME; \ > > > > + scall - static_calls_table.NAME < MAX_LSM_COUNT; scall++) \ > > > > + if (static_key_enabled(&scall->active->key)) > > > > > > This is probably a stupid question, but why use static_key_enabled() > > > here instead of static_branch_unlikely() as in the call_XXX_macros? > > > > The static_key_enabled is a check for the key being enabled, whereas > > the static_branch_likely is for laying out the right assembly code > > (jump tables etc.) and based on the value of the static key, here we > > are not using the static calls or even keys, rather we are following > > back from direct calls to indirect calls and don't really need any > > jump tables in the slow path. > > Gotcha, thanks for the explanation. > > -- > paul-moore.com
On Wed, Jul 3, 2024 at 6:22 PM KP Singh <kpsingh@kernel.org> wrote: > On Wed, Jul 3, 2024 at 10:56 PM Paul Moore <paul@paul-moore.com> wrote: > > On Wed, Jul 3, 2024 at 12:55 PM KP Singh <kpsingh@kernel.org> wrote: > > > On Wed, Jul 3, 2024 at 2:07 AM Paul Moore <paul@paul-moore.com> wrote: > > > > > > > > On Jun 29, 2024 KP Singh <kpsingh@kernel.org> wrote: > > > > > > > > > > LSM hooks are currently invoked from a linked list as indirect calls > > > > > which are invoked using retpolines as a mitigation for speculative > > > > > attacks (Branch History / Target injection) and add extra overhead which > > > > > is especially bad in kernel hot paths: > > > > > > [...] > > > > > > > should fix the more obvious problems. I'd like to know if you are > > > > aware of any others? If not, the text above should be adjusted and > > > > we should reconsider patch 5/5. If there are other problems I'd > > > > like to better understand them as there may be an independent > > > > solution for those particular problems. > > > > > > We did have problems with some other hooks but I was unable to dig up > > > specific examples though, it's been a while. More broadly speaking, a > > > default decision is still a decision. Whereas the intent from the BPF > > > LSM is not to make a default decision unless a BPF program is loaded. > > > I am quite worried about the holes this leaves open, subtle bugs > > > (security or crashes) we have not caught yet and PATCH 5/5 engineers away > > > the problem of the "default decision". > > > > The inode/xattr problem you originally mentioned wasn't really rooted > > in a "bad" default return value, it was really an issue with how the > > LSM hook was structured due to some legacy design assumptions made > > well before the initial stacking patches were merged. That should be > > fixed now[1] and given that the inode/xattr set/remove hooks were > > unique in this regard (individual LSMs were responsible for performing > > the capabilities checks) I don't expect this to be a general problem. > > > > There were also some issues caused by the fact that we were defining > > the default return value in multiple places and these values had gone > > out of sync in a number of hooks. We've also fixed this problem by > > only defining the default return value once for each hook, solving all > > of those problems. > > I don't see how this solves problems or prevents any future problems > with side-effects. I have always been uncomfortable with an extraneous > function being called with a side effect ever since we merged BPF LSM > with default callback. We have found one bug due to this, not all the > bugs. You've got to give me something more concrete than that. If you can't provide any concrete examples, start with providing a basic concept with far more detail than just "side-effects". > > I'm not aware of any other existing problems relating to the LSM hook > > default values, if there are any, we need to fix them independent of > > this patchset. The LSM framework should function properly if the > > "default" values are used. > > Patch 5 eliminates the possibilities of errors and subtle bugs all > together. The problem with subtle bugs is, well, they are subtle, if > you and I knew of the bugs, we would fix all of them, but we don't. I > really feel we ought to eliminate the class of issues and not just > whack-a-mole when we see the bugs. Here's the thing, I don't really like patch 5/5. To be honest, I don't really like a lot of this patchset. From my perspective, the complexity of the code is likely going to mean more maintenance headaches down the road, but Linus hath spoken so we're doing this (although "this" is still a bit undefined as far as I'm concerned). If you want me to merge patch 5/5 you've got to give me something real and convincing that can't be fixed by any other means. My current opinion is that you're trying to use a previously fixed bug to scare and/or coerce the merging of some changes I don't really want to merge. If you want me to take patch 5/5, you've got to give me a reason that is far more compelling that what you've written thus far.
On Thu, Jul 4, 2024 at 12:52 AM Paul Moore <paul@paul-moore.com> wrote: > > On Wed, Jul 3, 2024 at 6:22 PM KP Singh <kpsingh@kernel.org> wrote: > > On Wed, Jul 3, 2024 at 10:56 PM Paul Moore <paul@paul-moore.com> wrote: > > > On Wed, Jul 3, 2024 at 12:55 PM KP Singh <kpsingh@kernel.org> wrote: > > > > On Wed, Jul 3, 2024 at 2:07 AM Paul Moore <paul@paul-moore.com> wrote: > > > > > > > > > > On Jun 29, 2024 KP Singh <kpsingh@kernel.org> wrote: > > > > > > > > > > > > LSM hooks are currently invoked from a linked list as indirect calls > > > > > > which are invoked using retpolines as a mitigation for speculative > > > > > > attacks (Branch History / Target injection) and add extra overhead which > > > > > > is especially bad in kernel hot paths: > > > > > > > > [...] > > > > > > > > > should fix the more obvious problems. I'd like to know if you are > > > > > aware of any others? If not, the text above should be adjusted and > > > > > we should reconsider patch 5/5. If there are other problems I'd > > > > > like to better understand them as there may be an independent > > > > > solution for those particular problems. > > > > > > > > We did have problems with some other hooks but I was unable to dig up > > > > specific examples though, it's been a while. More broadly speaking, a > > > > default decision is still a decision. Whereas the intent from the BPF > > > > LSM is not to make a default decision unless a BPF program is loaded. > > > > I am quite worried about the holes this leaves open, subtle bugs > > > > (security or crashes) we have not caught yet and PATCH 5/5 engineers away > > > > the problem of the "default decision". > > > > > > The inode/xattr problem you originally mentioned wasn't really rooted > > > in a "bad" default return value, it was really an issue with how the > > > LSM hook was structured due to some legacy design assumptions made > > > well before the initial stacking patches were merged. That should be > > > fixed now[1] and given that the inode/xattr set/remove hooks were > > > unique in this regard (individual LSMs were responsible for performing > > > the capabilities checks) I don't expect this to be a general problem. > > > > > > There were also some issues caused by the fact that we were defining > > > the default return value in multiple places and these values had gone > > > out of sync in a number of hooks. We've also fixed this problem by > > > only defining the default return value once for each hook, solving all > > > of those problems. > > > > I don't see how this solves problems or prevents any future problems > > with side-effects. I have always been uncomfortable with an extraneous > > function being called with a side effect ever since we merged BPF LSM > > with default callback. We have found one bug due to this, not all the > > bugs. > > You've got to give me something more concrete than that. If you can't > provide any concrete examples, start with providing a basic concept > with far more detail than just "side-effects". > > > > I'm not aware of any other existing problems relating to the LSM hook > > > default values, if there are any, we need to fix them independent of > > > this patchset. The LSM framework should function properly if the > > > "default" values are used. > > > > Patch 5 eliminates the possibilities of errors and subtle bugs all > > together. The problem with subtle bugs is, well, they are subtle, if > > you and I knew of the bugs, we would fix all of them, but we don't. I > > really feel we ought to eliminate the class of issues and not just > > whack-a-mole when we see the bugs. > > Here's the thing, I don't really like patch 5/5. To be honest, I > don't really like a lot of this patchset. From my perspective, the > complexity of the code is likely going to mean more maintenance > headaches down the road, but Linus hath spoken so we're doing this > (although "this" is still a bit undefined as far as I'm concerned). > If you want me to merge patch 5/5 you've got to give me something real > and convincing that can't be fixed by any other means. My current > opinion is that you're trying to use a previously fixed bug to scare > and/or coerce the merging of some changes I don't really want to > merge. If you want me to take patch 5/5, you've got to give me a > reason that is far more compelling that what you've written thus far. Paul, I am not scaring you, I am providing a solution that saves us from headaches with side-effects and bugs in the future. It's safer by design. You say you have not reviewed it carefully, but you did ask me to move the function from the BPF LSM layer to an LSM API, and we had a bunch of discussion around naming in the subsequent revisions. https://lore.kernel.org/bpf/f7e8a16b0815d9d901e019934d684c5f@paul-moore.com/ My reasons are: 1. It's safer, no side effects, guaranteed to be not buggy. Neither you, nor me, can guarantee that a default value will be safe in the LSM layer. I request others (Casey, Kees) for their opinion here too. 2. Performance, no extra function call. If you still don't like it, it's your call, I would still like to keep most of the logic local to the BPF LSM as proposed in https://lore.kernel.org/bpf/f7e8a16b0815d9d901e019934d684c5f@paul-moore.com/ - KP > > -- > paul-moore.com
On 7/3/2024 4:08 PM, KP Singh wrote: > On Thu, Jul 4, 2024 at 12:52 AM Paul Moore <paul@paul-moore.com> wrote: >> On Wed, Jul 3, 2024 at 6:22 PM KP Singh <kpsingh@kernel.org> wrote: >>> On Wed, Jul 3, 2024 at 10:56 PM Paul Moore <paul@paul-moore.com> wrote: >>>> On Wed, Jul 3, 2024 at 12:55 PM KP Singh <kpsingh@kernel.org> wrote: >>>>> On Wed, Jul 3, 2024 at 2:07 AM Paul Moore <paul@paul-moore.com> wrote: >>>>>> On Jun 29, 2024 KP Singh <kpsingh@kernel.org> wrote: >>>>>>> LSM hooks are currently invoked from a linked list as indirect calls >>>>>>> which are invoked using retpolines as a mitigation for speculative >>>>>>> attacks (Branch History / Target injection) and add extra overhead which >>>>>>> is especially bad in kernel hot paths: >>>>> [...] >>>>> >>>>>> should fix the more obvious problems. I'd like to know if you are >>>>>> aware of any others? If not, the text above should be adjusted and >>>>>> we should reconsider patch 5/5. If there are other problems I'd >>>>>> like to better understand them as there may be an independent >>>>>> solution for those particular problems. >>>>> We did have problems with some other hooks but I was unable to dig up >>>>> specific examples though, it's been a while. More broadly speaking, a >>>>> default decision is still a decision. Whereas the intent from the BPF >>>>> LSM is not to make a default decision unless a BPF program is loaded. >>>>> I am quite worried about the holes this leaves open, subtle bugs >>>>> (security or crashes) we have not caught yet and PATCH 5/5 engineers away >>>>> the problem of the "default decision". >>>> The inode/xattr problem you originally mentioned wasn't really rooted >>>> in a "bad" default return value, it was really an issue with how the >>>> LSM hook was structured due to some legacy design assumptions made >>>> well before the initial stacking patches were merged. That should be >>>> fixed now[1] and given that the inode/xattr set/remove hooks were >>>> unique in this regard (individual LSMs were responsible for performing >>>> the capabilities checks) I don't expect this to be a general problem. >>>> >>>> There were also some issues caused by the fact that we were defining >>>> the default return value in multiple places and these values had gone >>>> out of sync in a number of hooks. We've also fixed this problem by >>>> only defining the default return value once for each hook, solving all >>>> of those problems. >>> I don't see how this solves problems or prevents any future problems >>> with side-effects. I have always been uncomfortable with an extraneous >>> function being called with a side effect ever since we merged BPF LSM >>> with default callback. We have found one bug due to this, not all the >>> bugs. >> You've got to give me something more concrete than that. If you can't >> provide any concrete examples, start with providing a basic concept >> with far more detail than just "side-effects". >> >>>> I'm not aware of any other existing problems relating to the LSM hook >>>> default values, if there are any, we need to fix them independent of >>>> this patchset. The LSM framework should function properly if the >>>> "default" values are used. >>> Patch 5 eliminates the possibilities of errors and subtle bugs all >>> together. The problem with subtle bugs is, well, they are subtle, if >>> you and I knew of the bugs, we would fix all of them, but we don't. I >>> really feel we ought to eliminate the class of issues and not just >>> whack-a-mole when we see the bugs. >> Here's the thing, I don't really like patch 5/5. To be honest, I >> don't really like a lot of this patchset. From my perspective, the >> complexity of the code is likely going to mean more maintenance >> headaches down the road, but Linus hath spoken so we're doing this >> (although "this" is still a bit undefined as far as I'm concerned). >> If you want me to merge patch 5/5 you've got to give me something real >> and convincing that can't be fixed by any other means. My current >> opinion is that you're trying to use a previously fixed bug to scare >> and/or coerce the merging of some changes I don't really want to >> merge. If you want me to take patch 5/5, you've got to give me a >> reason that is far more compelling that what you've written thus far. > Paul, I am not scaring you, I am providing a solution that saves us > from headaches with side-effects and bugs in the future. It's safer by > design. > > You say you have not reviewed it carefully, but you did ask me to move > the function from the BPF LSM layer to an LSM API, and we had a bunch > of discussion around naming in the subsequent revisions. > > https://lore.kernel.org/bpf/f7e8a16b0815d9d901e019934d684c5f@paul-moore.com/ > > My reasons are: > > 1. It's safer, no side effects, guaranteed to be not buggy. Neither > you, nor me, can guarantee that a default value will be safe in the > LSM layer. I request others (Casey, Kees) for their opinion here too. > 2. Performance, no extra function call. I want to be very careful about the comments I make on this patch set. I can't say that I trust any fix for the BPF LSM layer. My natural inclination is to isolate the fix to the area that has the problem, that is, BPF. I have a hard time accepting the notion that the implementation will really fix all possible bugs in the future. The pace at which eBPF is evolving gives me the heebee geebees when I think of it as a mechanism for implementing security modules. My biggest concern is that we may be trying too hard for perfection. I see a situation where we're not moving forward because there are two reasonable solutions and rather than running with either we're desperately looking for a compelling reason to pick one over the other. > > If you still don't like it, it's your call, I would still like to keep > most of the logic local to the BPF LSM as proposed in > https://lore.kernel.org/bpf/f7e8a16b0815d9d901e019934d684c5f@paul-moore.com/ > > - KP > >> -- >> paul-moore.com
On Thu, Jul 4, 2024 at 2:04 AM Casey Schaufler <casey@schaufler-ca.com> wrote: > > On 7/3/2024 4:08 PM, KP Singh wrote: > > On Thu, Jul 4, 2024 at 12:52 AM Paul Moore <paul@paul-moore.com> wrote: > >> On Wed, Jul 3, 2024 at 6:22 PM KP Singh <kpsingh@kernel.org> wrote: > >>> On Wed, Jul 3, 2024 at 10:56 PM Paul Moore <paul@paul-moore.com> wrote: > >>>> On Wed, Jul 3, 2024 at 12:55 PM KP Singh <kpsingh@kernel.org> wrote: > >>>>> On Wed, Jul 3, 2024 at 2:07 AM Paul Moore <paul@paul-moore.com> wrote: > >>>>>> On Jun 29, 2024 KP Singh <kpsingh@kernel.org> wrote: > >>>>>>> LSM hooks are currently invoked from a linked list as indirect calls > >>>>>>> which are invoked using retpolines as a mitigation for speculative > >>>>>>> attacks (Branch History / Target injection) and add extra overhead which > >>>>>>> is especially bad in kernel hot paths: > >>>>> [...] > >>>>> > >>>>>> should fix the more obvious problems. I'd like to know if you are > >>>>>> aware of any others? If not, the text above should be adjusted and > >>>>>> we should reconsider patch 5/5. If there are other problems I'd > >>>>>> like to better understand them as there may be an independent > >>>>>> solution for those particular problems. > >>>>> We did have problems with some other hooks but I was unable to dig up > >>>>> specific examples though, it's been a while. More broadly speaking, a > >>>>> default decision is still a decision. Whereas the intent from the BPF > >>>>> LSM is not to make a default decision unless a BPF program is loaded. > >>>>> I am quite worried about the holes this leaves open, subtle bugs > >>>>> (security or crashes) we have not caught yet and PATCH 5/5 engineers away > >>>>> the problem of the "default decision". > >>>> The inode/xattr problem you originally mentioned wasn't really rooted > >>>> in a "bad" default return value, it was really an issue with how the > >>>> LSM hook was structured due to some legacy design assumptions made > >>>> well before the initial stacking patches were merged. That should be > >>>> fixed now[1] and given that the inode/xattr set/remove hooks were > >>>> unique in this regard (individual LSMs were responsible for performing > >>>> the capabilities checks) I don't expect this to be a general problem. > >>>> > >>>> There were also some issues caused by the fact that we were defining > >>>> the default return value in multiple places and these values had gone > >>>> out of sync in a number of hooks. We've also fixed this problem by > >>>> only defining the default return value once for each hook, solving all > >>>> of those problems. > >>> I don't see how this solves problems or prevents any future problems > >>> with side-effects. I have always been uncomfortable with an extraneous > >>> function being called with a side effect ever since we merged BPF LSM > >>> with default callback. We have found one bug due to this, not all the > >>> bugs. > >> You've got to give me something more concrete than that. If you can't > >> provide any concrete examples, start with providing a basic concept > >> with far more detail than just "side-effects". > >> > >>>> I'm not aware of any other existing problems relating to the LSM hook > >>>> default values, if there are any, we need to fix them independent of > >>>> this patchset. The LSM framework should function properly if the > >>>> "default" values are used. > >>> Patch 5 eliminates the possibilities of errors and subtle bugs all > >>> together. The problem with subtle bugs is, well, they are subtle, if > >>> you and I knew of the bugs, we would fix all of them, but we don't. I > >>> really feel we ought to eliminate the class of issues and not just > >>> whack-a-mole when we see the bugs. > >> Here's the thing, I don't really like patch 5/5. To be honest, I > >> don't really like a lot of this patchset. From my perspective, the > >> complexity of the code is likely going to mean more maintenance > >> headaches down the road, but Linus hath spoken so we're doing this > >> (although "this" is still a bit undefined as far as I'm concerned). > >> If you want me to merge patch 5/5 you've got to give me something real > >> and convincing that can't be fixed by any other means. My current > >> opinion is that you're trying to use a previously fixed bug to scare > >> and/or coerce the merging of some changes I don't really want to > >> merge. If you want me to take patch 5/5, you've got to give me a > >> reason that is far more compelling that what you've written thus far. > > Paul, I am not scaring you, I am providing a solution that saves us > > from headaches with side-effects and bugs in the future. It's safer by > > design. > > > > You say you have not reviewed it carefully, but you did ask me to move > > the function from the BPF LSM layer to an LSM API, and we had a bunch > > of discussion around naming in the subsequent revisions. > > > > https://lore.kernel.org/bpf/f7e8a16b0815d9d901e019934d684c5f@paul-moore.com/ > > > > My reasons are: > > > > 1. It's safer, no side effects, guaranteed to be not buggy. Neither > > you, nor me, can guarantee that a default value will be safe in the > > LSM layer. I request others (Casey, Kees) for their opinion here too. > > 2. Performance, no extra function call. > > I want to be very careful about the comments I make on this patch set. > I can't say that I trust any fix for the BPF LSM layer. My natural > inclination is to isolate the fix to the area that has the problem, > that is, BPF. I have a hard time accepting the notion that the implementation > will really fix all possible bugs in the future. The pace at which eBPF > is evolving gives me the heebee geebees when I think of it as a mechanism > for implementing security modules. > > My biggest concern is that we may be trying too hard for perfection. > I see a situation where we're not moving forward because there are two > reasonable solutions and rather than running with either we're desperately > looking for a compelling reason to pick one over the other. I am fine with either, if you folks prefer security_toggle_hook to be in BPF only / limited to BPF. I can revert back to what we had in v9, the changes to the LSM layer are then very minimal. - KP > > > > > If you still don't like it, it's your call, I would still like to keep > > most of the logic local to the BPF LSM as proposed in > > https://lore.kernel.org/bpf/f7e8a16b0815d9d901e019934d684c5f@paul-moore.com/ > > > > - KP > > > >> -- > >> paul-moore.com
On Thu, Jul 4, 2024 at 2:24 AM KP Singh <kpsingh@kernel.org> wrote: > > On Thu, Jul 4, 2024 at 2:04 AM Casey Schaufler <casey@schaufler-ca.com> wrote: > > > > On 7/3/2024 4:08 PM, KP Singh wrote: > > > On Thu, Jul 4, 2024 at 12:52 AM Paul Moore <paul@paul-moore.com> wrote: > > >> On Wed, Jul 3, 2024 at 6:22 PM KP Singh <kpsingh@kernel.org> wrote: > > >>> On Wed, Jul 3, 2024 at 10:56 PM Paul Moore <paul@paul-moore.com> wrote: > > >>>> On Wed, Jul 3, 2024 at 12:55 PM KP Singh <kpsingh@kernel.org> wrote: > > >>>>> On Wed, Jul 3, 2024 at 2:07 AM Paul Moore <paul@paul-moore.com> wrote: > > >>>>>> On Jun 29, 2024 KP Singh <kpsingh@kernel.org> wrote: > > >>>>>>> LSM hooks are currently invoked from a linked list as indirect calls > > >>>>>>> which are invoked using retpolines as a mitigation for speculative > > >>>>>>> attacks (Branch History / Target injection) and add extra overhead which > > >>>>>>> is especially bad in kernel hot paths: > > >>>>> [...] > > >>>>> > > >>>>>> should fix the more obvious problems. I'd like to know if you are > > >>>>>> aware of any others? If not, the text above should be adjusted and > > >>>>>> we should reconsider patch 5/5. If there are other problems I'd > > >>>>>> like to better understand them as there may be an independent > > >>>>>> solution for those particular problems. > > >>>>> We did have problems with some other hooks but I was unable to dig up > > >>>>> specific examples though, it's been a while. More broadly speaking, a > > >>>>> default decision is still a decision. Whereas the intent from the BPF > > >>>>> LSM is not to make a default decision unless a BPF program is loaded. > > >>>>> I am quite worried about the holes this leaves open, subtle bugs > > >>>>> (security or crashes) we have not caught yet and PATCH 5/5 engineers away > > >>>>> the problem of the "default decision". > > >>>> The inode/xattr problem you originally mentioned wasn't really rooted > > >>>> in a "bad" default return value, it was really an issue with how the > > >>>> LSM hook was structured due to some legacy design assumptions made > > >>>> well before the initial stacking patches were merged. That should be > > >>>> fixed now[1] and given that the inode/xattr set/remove hooks were > > >>>> unique in this regard (individual LSMs were responsible for performing > > >>>> the capabilities checks) I don't expect this to be a general problem. > > >>>> > > >>>> There were also some issues caused by the fact that we were defining > > >>>> the default return value in multiple places and these values had gone > > >>>> out of sync in a number of hooks. We've also fixed this problem by > > >>>> only defining the default return value once for each hook, solving all > > >>>> of those problems. > > >>> I don't see how this solves problems or prevents any future problems > > >>> with side-effects. I have always been uncomfortable with an extraneous > > >>> function being called with a side effect ever since we merged BPF LSM > > >>> with default callback. We have found one bug due to this, not all the > > >>> bugs. > > >> You've got to give me something more concrete than that. If you can't > > >> provide any concrete examples, start with providing a basic concept > > >> with far more detail than just "side-effects". > > >> > > >>>> I'm not aware of any other existing problems relating to the LSM hook > > >>>> default values, if there are any, we need to fix them independent of > > >>>> this patchset. The LSM framework should function properly if the > > >>>> "default" values are used. > > >>> Patch 5 eliminates the possibilities of errors and subtle bugs all > > >>> together. The problem with subtle bugs is, well, they are subtle, if > > >>> you and I knew of the bugs, we would fix all of them, but we don't. I > > >>> really feel we ought to eliminate the class of issues and not just > > >>> whack-a-mole when we see the bugs. > > >> Here's the thing, I don't really like patch 5/5. To be honest, I > > >> don't really like a lot of this patchset. From my perspective, the > > >> complexity of the code is likely going to mean more maintenance > > >> headaches down the road, but Linus hath spoken so we're doing this > > >> (although "this" is still a bit undefined as far as I'm concerned). > > >> If you want me to merge patch 5/5 you've got to give me something real > > >> and convincing that can't be fixed by any other means. My current > > >> opinion is that you're trying to use a previously fixed bug to scare > > >> and/or coerce the merging of some changes I don't really want to > > >> merge. If you want me to take patch 5/5, you've got to give me a > > >> reason that is far more compelling that what you've written thus far. > > > Paul, I am not scaring you, I am providing a solution that saves us > > > from headaches with side-effects and bugs in the future. It's safer by > > > design. > > > > > > You say you have not reviewed it carefully, but you did ask me to move > > > the function from the BPF LSM layer to an LSM API, and we had a bunch > > > of discussion around naming in the subsequent revisions. > > > > > > https://lore.kernel.org/bpf/f7e8a16b0815d9d901e019934d684c5f@paul-moore.com/ > > > > > > My reasons are: > > > > > > 1. It's safer, no side effects, guaranteed to be not buggy. Neither > > > you, nor me, can guarantee that a default value will be safe in the > > > LSM layer. I request others (Casey, Kees) for their opinion here too. > > > 2. Performance, no extra function call. > > > > I want to be very careful about the comments I make on this patch set. > > I can't say that I trust any fix for the BPF LSM layer. My natural > > inclination is to isolate the fix to the area that has the problem, > > that is, BPF. I have a hard time accepting the notion that the implementation > > will really fix all possible bugs in the future. The pace at which eBPF > > is evolving gives me the heebee geebees when I think of it as a mechanism > > for implementing security modules. > > > > My biggest concern is that we may be trying too hard for perfection. > > I see a situation where we're not moving forward because there are two > > reasonable solutions and rather than running with either we're desperately > > looking for a compelling reason to pick one over the other. > > I am fine with either, if you folks prefer security_toggle_hook to be > in BPF only / limited to BPF. I can revert back to what we had in v9, > the changes to the LSM layer are then very minimal. Now if you folks really don't want any changes to the core LSM layer, that too is doable, the patch below accomplishes that. So, here's where we are at, while the LSM framework is comfortable with saying that default values and empty callbacks are fine, that's not what BPF LSM is comfortable doing. Your concerns around changes to the LSM layer should be addressed by the propose patch below: diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h index 1de7ece5d36d..5a2ab1067095 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) @@ -52,6 +53,10 @@ static inline bool bpf_lsm_is_sleepable_hook(u32 btf_id) return false; } +static inline void bpf_lsm_toggle_hook(void *addr, bool value) +{ +} + static inline bool bpf_lsm_is_trusted(const struct bpf_prog *prog) { return false; diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c index f8302a5ca400..bc59025b3d46 100644 --- a/kernel/bpf/trampoline.c +++ b/kernel/bpf/trampoline.c @@ -523,6 +523,22 @@ 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; @@ -562,6 +578,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); @@ -595,6 +615,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 57b9ffd53c98..9ca3db6d2b07 100644 --- a/security/bpf/hooks.c +++ b/security/bpf/hooks.c @@ -16,6 +16,29 @@ static struct security_hook_list bpf_lsm_hooks[] __ro_after_init = { LSM_HOOK_INIT(task_free, bpf_task_storage_free), }; +void bpf_lsm_toggle_hook(void *addr, bool enable) +{ + 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_func_addr != addr) + continue; + + for (j = 0; j < MAX_LSM_COUNT; j++) { + scalls = &h->scalls[j]; + if (scalls->hl != &bpf_lsm_hooks[i]) + continue; + if (enable) + static_branch_enable(scalls->active); + else + static_branch_disable(scalls->active); + } + } +} + static const struct lsm_id bpf_lsmid = { .name = "bpf", .id = LSM_ID_BPF, @@ -23,8 +46,14 @@ static const struct lsm_id bpf_lsmid = { static int __init bpf_lsm_init(void) { + int i; + security_add_hooks(bpf_lsm_hooks, ARRAY_SIZE(bpf_lsm_hooks), &bpf_lsmid); + + for (i = 0; i < ARRAY_SIZE(bpf_lsm_hooks); i++) + bpf_lsm_toggle_hook(bpf_lsm_hooks[i].hook.lsm_func_addr, false); + pr_info("LSM support for eBPF active\n"); return 0; } - KP > > - KP > > > > > > > > > If you still don't like it, it's your call, I would still like to keep > > > most of the logic local to the BPF LSM as proposed in > > > https://lore.kernel.org/bpf/f7e8a16b0815d9d901e019934d684c5f@paul-moore.com/ > > > > > > - KP > > > > > >> -- > > >> paul-moore.com
On Wed, Jul 3, 2024 at 7:08 PM KP Singh <kpsingh@kernel.org> wrote: > On Thu, Jul 4, 2024 at 12:52 AM Paul Moore <paul@paul-moore.com> wrote: > > On Wed, Jul 3, 2024 at 6:22 PM KP Singh <kpsingh@kernel.org> wrote: > > > On Wed, Jul 3, 2024 at 10:56 PM Paul Moore <paul@paul-moore.com> wrote: > > > > On Wed, Jul 3, 2024 at 12:55 PM KP Singh <kpsingh@kernel.org> wrote: > > > > > On Wed, Jul 3, 2024 at 2:07 AM Paul Moore <paul@paul-moore.com> wrote: > > > > > > On Jun 29, 2024 KP Singh <kpsingh@kernel.org> wrote: > > > > > > > > > > > > > > LSM hooks are currently invoked from a linked list as indirect calls > > > > > > > which are invoked using retpolines as a mitigation for speculative > > > > > > > attacks (Branch History / Target injection) and add extra overhead which > > > > > > > is especially bad in kernel hot paths: ... > > > > I'm not aware of any other existing problems relating to the LSM hook > > > > default values, if there are any, we need to fix them independent of > > > > this patchset. The LSM framework should function properly if the > > > > "default" values are used. > > > > > > Patch 5 eliminates the possibilities of errors and subtle bugs all > > > together. The problem with subtle bugs is, well, they are subtle, if > > > you and I knew of the bugs, we would fix all of them, but we don't. I > > > really feel we ought to eliminate the class of issues and not just > > > whack-a-mole when we see the bugs. > > > > Here's the thing, I don't really like patch 5/5. To be honest, I > > don't really like a lot of this patchset. From my perspective, the > > complexity of the code is likely going to mean more maintenance > > headaches down the road, but Linus hath spoken so we're doing this > > (although "this" is still a bit undefined as far as I'm concerned). > > If you want me to merge patch 5/5 you've got to give me something real > > and convincing that can't be fixed by any other means. My current > > opinion is that you're trying to use a previously fixed bug to scare > > and/or coerce the merging of some changes I don't really want to > > merge. If you want me to take patch 5/5, you've got to give me a > > reason that is far more compelling that what you've written thus far. > > Paul, I am not scaring you, I am providing a solution that saves us > from headaches with side-effects and bugs in the future. It's safer by > design. Perhaps I wasn't clear enough in my previous emails; instead of trying to convince me that your solution is literally the best possible thing to ever touch the kernel, convince me that there is a problem we need to fix. Right now, I'm not convinced there is a bug that requires all of the extra code in patch 5/5 (all of which have the potential to introduce new bugs). As mentioned previously, the bugs that typically have been used as examples of unwanted side effects with the LSM hooks have been resolved, both in the specific and general case. If you want me to add more code/functionality to fix a bug, you must first demonstrate the bug exists and the risk is real; you have not done that as far as I'm concerned. > You say you have not reviewed it carefully ... That may have been true of previous versions of this patchset, but I did not say that about this current patchset. > ... but you did ask me to move > the function from the BPF LSM layer to an LSM API, and we had a bunch > of discussion around naming in the subsequent revisions. > > https://lore.kernel.org/bpf/f7e8a16b0815d9d901e019934d684c5f@paul-moore.com/ That discussion predates commit 61df7b828204 ("lsm: fixup the inode xattr capability handling") which is currently in the lsm/dev branch, marked for stable, and will go up to Linus during the upcoming merge window. > My reasons are: > > 1. It's safer, no side effects, guaranteed to be not buggy. Neither > you, nor me, can guarantee that a default value will be safe in the > LSM layer. In the first sentence above you "guarantee" that your code is not buggy and then follow that up with a second sentence discussing how no one can guarantee source code safety. Regardless of whatever point you were trying to make here, I maintain that *all* patches have the potential for bugs, even those that are attempting to fix bugs. With that in mind, if you want me to merge more code to fix a bug (class), a bug that I've mentioned several times now that I believe we've already fixed, you first MUST convince me that the bug (class) still exists. You have not done that. > 2. Performance, no extra function call. Convince me the bug still exists first and then we can discuss the merits of whatever solutions are proposed.
On Fri, Jul 5, 2024 at 8:07 PM Paul Moore <paul@paul-moore.com> wrote: > > On Wed, Jul 3, 2024 at 7:08 PM KP Singh <kpsingh@kernel.org> wrote: > > On Thu, Jul 4, 2024 at 12:52 AM Paul Moore <paul@paul-moore.com> wrote: > > > On Wed, Jul 3, 2024 at 6:22 PM KP Singh <kpsingh@kernel.org> wrote: > > > > On Wed, Jul 3, 2024 at 10:56 PM Paul Moore <paul@paul-moore.com> wrote: > > > > > On Wed, Jul 3, 2024 at 12:55 PM KP Singh <kpsingh@kernel.org> wrote: > > > > > > On Wed, Jul 3, 2024 at 2:07 AM Paul Moore <paul@paul-moore.com> wrote: > > > > > > > On Jun 29, 2024 KP Singh <kpsingh@kernel.org> wrote: > > > > > > > > > > > > > > > > LSM hooks are currently invoked from a linked list as indirect calls > > > > > > > > which are invoked using retpolines as a mitigation for speculative > > > > > > > > attacks (Branch History / Target injection) and add extra overhead which > > > > > > > > is especially bad in kernel hot paths: > > ... > > > > > > I'm not aware of any other existing problems relating to the LSM hook > > > > > default values, if there are any, we need to fix them independent of > > > > > this patchset. The LSM framework should function properly if the > > > > > "default" values are used. > > > > > > > > Patch 5 eliminates the possibilities of errors and subtle bugs all > > > > together. The problem with subtle bugs is, well, they are subtle, if > > > > you and I knew of the bugs, we would fix all of them, but we don't. I > > > > really feel we ought to eliminate the class of issues and not just > > > > whack-a-mole when we see the bugs. > > > > > > Here's the thing, I don't really like patch 5/5. To be honest, I > > > don't really like a lot of this patchset. From my perspective, the > > > complexity of the code is likely going to mean more maintenance > > > headaches down the road, but Linus hath spoken so we're doing this > > > (although "this" is still a bit undefined as far as I'm concerned). > > > If you want me to merge patch 5/5 you've got to give me something real > > > and convincing that can't be fixed by any other means. My current > > > opinion is that you're trying to use a previously fixed bug to scare > > > and/or coerce the merging of some changes I don't really want to > > > merge. If you want me to take patch 5/5, you've got to give me a > > > reason that is far more compelling that what you've written thus far. > > > > Paul, I am not scaring you, I am providing a solution that saves us > > from headaches with side-effects and bugs in the future. It's safer by > > design. > > Perhaps I wasn't clear enough in my previous emails; instead of trying > to convince me that your solution is literally the best possible thing > to ever touch the kernel, convince me that there is a problem we need > to fix. Right now, I'm not convinced there is a bug that requires all > of the extra code in patch 5/5 (all of which have the potential to > introduce new bugs). As mentioned previously, the bugs that typically > have been used as examples of unwanted side effects with the LSM hooks > have been resolved, both in the specific and general case. If you > want me to add more code/functionality to fix a bug, you must first > demonstrate the bug exists and the risk is real; you have not done > that as far as I'm concerned. > > > You say you have not reviewed it carefully ... > > That may have been true of previous versions of this patchset, but I > did not say that about this current patchset. > > > ... but you did ask me to move > > the function from the BPF LSM layer to an LSM API, and we had a bunch > > of discussion around naming in the subsequent revisions. > > > > https://lore.kernel.org/bpf/f7e8a16b0815d9d901e019934d684c5f@paul-moore.com/ > > That discussion predates commit 61df7b828204 ("lsm: fixup the inode > xattr capability handling") which is currently in the lsm/dev branch, > marked for stable, and will go up to Linus during the upcoming merge > window. > > > My reasons are: > > > > 1. It's safer, no side effects, guaranteed to be not buggy. Neither > > you, nor me, can guarantee that a default value will be safe in the > > LSM layer. > > In the first sentence above you "guarantee" that your code is not > buggy and then follow that up with a second sentence discussing how no > one can guarantee source code safety. Regardless of whatever point > you were trying to make here, I maintain that *all* patches have the > potential for bugs, even those that are attempting to fix bugs. WithD > that in mind, if you want me to merge more code to fix a bug (class), > a bug that I've mentioned several times now that I believe we've > already fixed, you first MUST convince me that the bug (class) still > exists. You have not done that. > Paul, I am talking about eliminating a class of bugs, but you don't seem to get the point and you are fixated on the very instance of this bug class. > > 2. Performance, no extra function call. > > Convince me the bug still exists first and then we can discuss the > merits of whatever solutions are proposed. This is independent of the bug! The extra function calls have performance overhead and as the BPF LSM maintainer I am not okay with these extraneous calls when I have a clear way of solving it. As I said, If you don't want to modify the core LSM layer, it's okay, I still want to go with changes local to the BPF LSM, If you really don't agree with the changes local to the BPF LSM, we can have it go via the BPF tree and seek Linus' help to resolve the conflict. diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h index 1de7ece5d36d..5a2ab1067095 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) @@ -52,6 +53,10 @@ static inline bool bpf_lsm_is_sleepable_hook(u32 btf_id) return false; } +static inline void bpf_lsm_toggle_hook(void *addr, bool value) +{ +} + static inline bool bpf_lsm_is_trusted(const struct bpf_prog *prog) { return false; diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c index f8302a5ca400..bc59025b3d46 100644 --- a/kernel/bpf/trampoline.c +++ b/kernel/bpf/trampoline.c @@ -523,6 +523,22 @@ 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; @@ -562,6 +578,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); @@ -595,6 +615,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 57b9ffd53c98..9ca3db6d2b07 100644 --- a/security/bpf/hooks.c +++ b/security/bpf/hooks.c @@ -16,6 +16,29 @@ static struct security_hook_list bpf_lsm_hooks[] __ro_after_init = { LSM_HOOK_INIT(task_free, bpf_task_storage_free), }; +void bpf_lsm_toggle_hook(void *addr, bool enable) +{ + 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_func_addr != addr) + continue; + + for (j = 0; j < MAX_LSM_COUNT; j++) { + scalls = &h->scalls[j]; + if (scalls->hl != &bpf_lsm_hooks[i]) + continue; + if (enable) + static_branch_enable(scalls->active); + else + static_branch_disable(scalls->active); + } + } +} + static const struct lsm_id bpf_lsmid = { .name = "bpf", .id = LSM_ID_BPF, @@ -23,8 +46,14 @@ static const struct lsm_id bpf_lsmid = { static int __init bpf_lsm_init(void) { + int i; + security_add_hooks(bpf_lsm_hooks, ARRAY_SIZE(bpf_lsm_hooks), &bpf_lsmid); + + for (i = 0; i < ARRAY_SIZE(bpf_lsm_hooks); i++) + bpf_lsm_toggle_hook(bpf_lsm_hooks[i].hook.lsm_func_addr, false); + pr_info("LSM support for eBPF active\n"); return 0; } > > -- > paul-moore.com
On Fri, Jul 05, 2024 at 09:34:20PM +0200, KP Singh wrote: > On Fri, Jul 5, 2024 at 8:07 PM Paul Moore <paul@paul-moore.com> wrote: > > > > On Wed, Jul 3, 2024 at 7:08 PM KP Singh <kpsingh@kernel.org> wrote: > > > On Thu, Jul 4, 2024 at 12:52 AM Paul Moore <paul@paul-moore.com> wrote: > > > > On Wed, Jul 3, 2024 at 6:22 PM KP Singh <kpsingh@kernel.org> wrote: > > > > > On Wed, Jul 3, 2024 at 10:56 PM Paul Moore <paul@paul-moore.com> wrote: > > > > > > On Wed, Jul 3, 2024 at 12:55 PM KP Singh <kpsingh@kernel.org> wrote: > > > > > > > On Wed, Jul 3, 2024 at 2:07 AM Paul Moore <paul@paul-moore.com> wrote: > > > > > > > > On Jun 29, 2024 KP Singh <kpsingh@kernel.org> wrote: > > > > > > > > > > > > > > > > > > LSM hooks are currently invoked from a linked list as indirect calls > > > > > > > > > which are invoked using retpolines as a mitigation for speculative > > > > > > > > > attacks (Branch History / Target injection) and add extra overhead which > > > > > > > > > is especially bad in kernel hot paths: > > > > ... > > > > > > > > I'm not aware of any other existing problems relating to the LSM hook > > > > > > default values, if there are any, we need to fix them independent of > > > > > > this patchset. The LSM framework should function properly if the > > > > > > "default" values are used. > > > > > > > > > > Patch 5 eliminates the possibilities of errors and subtle bugs all > > > > > together. The problem with subtle bugs is, well, they are subtle, if > > > > > you and I knew of the bugs, we would fix all of them, but we don't. I > > > > > really feel we ought to eliminate the class of issues and not just > > > > > whack-a-mole when we see the bugs. > > > > > > > > Here's the thing, I don't really like patch 5/5. To be honest, I > > > > don't really like a lot of this patchset. From my perspective, the > > > > complexity of the code is likely going to mean more maintenance > > > > headaches down the road, but Linus hath spoken so we're doing this > > > > (although "this" is still a bit undefined as far as I'm concerned). > > > > If you want me to merge patch 5/5 you've got to give me something real > > > > and convincing that can't be fixed by any other means. My current > > > > opinion is that you're trying to use a previously fixed bug to scare > > > > and/or coerce the merging of some changes I don't really want to > > > > merge. If you want me to take patch 5/5, you've got to give me a > > > > reason that is far more compelling that what you've written thus far. > > > > > > Paul, I am not scaring you, I am providing a solution that saves us > > > from headaches with side-effects and bugs in the future. It's safer by > > > design. > > > > Perhaps I wasn't clear enough in my previous emails; instead of trying > > to convince me that your solution is literally the best possible thing > > to ever touch the kernel, convince me that there is a problem we need > > to fix. Right now, I'm not convinced there is a bug that requires all > > of the extra code in patch 5/5 (all of which have the potential to > > introduce new bugs). As mentioned previously, the bugs that typically > > have been used as examples of unwanted side effects with the LSM hooks > > have been resolved, both in the specific and general case. If you > > want me to add more code/functionality to fix a bug, you must first > > demonstrate the bug exists and the risk is real; you have not done > > that as far as I'm concerned. > > > > > You say you have not reviewed it carefully ... > > > > That may have been true of previous versions of this patchset, but I > > did not say that about this current patchset. > > > > > ... but you did ask me to move > > > the function from the BPF LSM layer to an LSM API, and we had a bunch > > > of discussion around naming in the subsequent revisions. > > > > > > https://lore.kernel.org/bpf/f7e8a16b0815d9d901e019934d684c5f@paul-moore.com/ > > > > That discussion predates commit 61df7b828204 ("lsm: fixup the inode > > xattr capability handling") which is currently in the lsm/dev branch, > > marked for stable, and will go up to Linus during the upcoming merge > > window. > > > > > My reasons are: > > > > > > 1. It's safer, no side effects, guaranteed to be not buggy. Neither > > > you, nor me, can guarantee that a default value will be safe in the > > > LSM layer. > > > > In the first sentence above you "guarantee" that your code is not > > buggy and then follow that up with a second sentence discussing how no > > one can guarantee source code safety. Regardless of whatever point > > you were trying to make here, I maintain that *all* patches have the > > potential for bugs, even those that are attempting to fix bugs. WithD > > that in mind, if you want me to merge more code to fix a bug (class), > > a bug that I've mentioned several times now that I believe we've > > already fixed, you first MUST convince me that the bug (class) still > > exists. You have not done that. > > > > Paul, I am talking about eliminating a class of bugs, but you don't > seem to get the point and you are fixated on the very instance of this > bug class. Let's take this one step at a time. I think patches 1-4 are fine and stand alone and solve a specific problem without creating any new immediate problems. After 1-4 is accepted, we can come back around to what patch 5 is trying to do, and work on whatever issues may remain at that time.
On Fri, Jul 5, 2024 at 3:34 PM KP Singh <kpsingh@kernel.org> wrote: > On Fri, Jul 5, 2024 at 8:07 PM Paul Moore <paul@paul-moore.com> wrote: > > On Wed, Jul 3, 2024 at 7:08 PM KP Singh <kpsingh@kernel.org> wrote: > > > On Thu, Jul 4, 2024 at 12:52 AM Paul Moore <paul@paul-moore.com> wrote: > > > > On Wed, Jul 3, 2024 at 6:22 PM KP Singh <kpsingh@kernel.org> wrote: > > > > > On Wed, Jul 3, 2024 at 10:56 PM Paul Moore <paul@paul-moore.com> wrote: > > > > > > On Wed, Jul 3, 2024 at 12:55 PM KP Singh <kpsingh@kernel.org> wrote: > > > > > > > On Wed, Jul 3, 2024 at 2:07 AM Paul Moore <paul@paul-moore.com> wrote: > > > > > > > > On Jun 29, 2024 KP Singh <kpsingh@kernel.org> wrote: > > > > > > > > > > > > > > > > > > LSM hooks are currently invoked from a linked list as indirect calls > > > > > > > > > which are invoked using retpolines as a mitigation for speculative > > > > > > > > > attacks (Branch History / Target injection) and add extra overhead which > > > > > > > > > is especially bad in kernel hot paths: > > > > ... > > > > > > > > I'm not aware of any other existing problems relating to the LSM hook > > > > > > default values, if there are any, we need to fix them independent of > > > > > > this patchset. The LSM framework should function properly if the > > > > > > "default" values are used. > > > > > > > > > > Patch 5 eliminates the possibilities of errors and subtle bugs all > > > > > together. The problem with subtle bugs is, well, they are subtle, if > > > > > you and I knew of the bugs, we would fix all of them, but we don't. I > > > > > really feel we ought to eliminate the class of issues and not just > > > > > whack-a-mole when we see the bugs. > > > > > > > > Here's the thing, I don't really like patch 5/5. To be honest, I > > > > don't really like a lot of this patchset. From my perspective, the > > > > complexity of the code is likely going to mean more maintenance > > > > headaches down the road, but Linus hath spoken so we're doing this > > > > (although "this" is still a bit undefined as far as I'm concerned). > > > > If you want me to merge patch 5/5 you've got to give me something real > > > > and convincing that can't be fixed by any other means. My current > > > > opinion is that you're trying to use a previously fixed bug to scare > > > > and/or coerce the merging of some changes I don't really want to > > > > merge. If you want me to take patch 5/5, you've got to give me a > > > > reason that is far more compelling that what you've written thus far. > > > > > > Paul, I am not scaring you, I am providing a solution that saves us > > > from headaches with side-effects and bugs in the future. It's safer by > > > design. > > > > Perhaps I wasn't clear enough in my previous emails; instead of trying > > to convince me that your solution is literally the best possible thing > > to ever touch the kernel, convince me that there is a problem we need > > to fix. Right now, I'm not convinced there is a bug that requires all > > of the extra code in patch 5/5 (all of which have the potential to > > introduce new bugs). As mentioned previously, the bugs that typically > > have been used as examples of unwanted side effects with the LSM hooks > > have been resolved, both in the specific and general case. If you > > want me to add more code/functionality to fix a bug, you must first > > demonstrate the bug exists and the risk is real; you have not done > > that as far as I'm concerned. > > > > > You say you have not reviewed it carefully ... > > > > That may have been true of previous versions of this patchset, but I > > did not say that about this current patchset. > > > > > ... but you did ask me to move > > > the function from the BPF LSM layer to an LSM API, and we had a bunch > > > of discussion around naming in the subsequent revisions. > > > > > > https://lore.kernel.org/bpf/f7e8a16b0815d9d901e019934d684c5f@paul-moore.com/ > > > > That discussion predates commit 61df7b828204 ("lsm: fixup the inode > > xattr capability handling") which is currently in the lsm/dev branch, > > marked for stable, and will go up to Linus during the upcoming merge > > window. > > > > > My reasons are: > > > > > > 1. It's safer, no side effects, guaranteed to be not buggy. Neither > > > you, nor me, can guarantee that a default value will be safe in the > > > LSM layer. > > > > In the first sentence above you "guarantee" that your code is not > > buggy and then follow that up with a second sentence discussing how no > > one can guarantee source code safety. Regardless of whatever point > > you were trying to make here, I maintain that *all* patches have the > > potential for bugs, even those that are attempting to fix bugs. WithD > > that in mind, if you want me to merge more code to fix a bug (class), > > a bug that I've mentioned several times now that I believe we've > > already fixed, you first MUST convince me that the bug (class) still > > exists. You have not done that. > > Paul, I am talking about eliminating a class of bugs, but you don't > seem to get the point and you are fixated on the very instance of this > bug class. I do understand that you are trying to eliminate a class of bugs, the point I'm trying to make is that I believe we have addressed that already with the patches I've previously cited. > > > 2. Performance, no extra function call. > > > > Convince me the bug still exists first and then we can discuss the > > merits of whatever solutions are proposed. > > This is independent of the bug! Correctness first, maintainability second, performance third. That's my current priority and I feel the maintainability hit doesn't justify the performance win at this point in time. Besides, we're already expecting a big performance boost simply by moving to static_calls. > As I said, If you don't want to modify the core LSM layer, it's okay, > I still want to go with changes local to the BPF LSM, If you really > don't agree with the changes local to the BPF LSM, we can have it go > via the BPF tree and seek Linus' help to resolve the conflict. As the BPF maintainer you are always free to do whatever you like within the scope of the LSM you maintain so long as it does not touch or otherwise impact any of the other LSMs or the LSM framework. If you do affect the other LSMs, or the LSM framework, you need to get an ACK from the associated maintainer. That's pretty much how Linux kernel development works.
On Fri, Jul 5, 2024 at 8:17 PM Kees Cook <kees@kernel.org> wrote: > > Let's take this one step at a time. I think patches 1-4 are fine and > stand alone and solve a specific problem without creating any new > immediate problems. > > After 1-4 is accepted, we can come back around to what patch 5 is trying > to do, and work on whatever issues may remain at that time. Kees, whatever technical review you may have for any proposed patches is always welcome, and if you want to provide process advice, solicited or otherwise, to people posting patches that is up to you. However, after your last comments regarding the management of the LSM tree, I want to make it clear that I'm not interested in your opinions on what should be merged into the LSM tree at this point in time.
On Sat, Jul 6, 2024 at 6:40 AM Paul Moore <paul@paul-moore.com> wrote: > > On Fri, Jul 5, 2024 at 3:34 PM KP Singh <kpsingh@kernel.org> wrote: > > On Fri, Jul 5, 2024 at 8:07 PM Paul Moore <paul@paul-moore.com> wrote: > > > On Wed, Jul 3, 2024 at 7:08 PM KP Singh <kpsingh@kernel.org> wrote: [...] > > > > Paul, I am talking about eliminating a class of bugs, but you don't > > seem to get the point and you are fixated on the very instance of this > > bug class. > > I do understand that you are trying to eliminate a class of bugs, the > point I'm trying to make is that I believe we have addressed that > already with the patches I've previously cited. The class I am referring to is useless hooks returning a default value and imposing a denial / enforcement when they are not supposed to. If you think this class of issues is not relevant to the overall LSM, sure. I would still like BPF LSM to not add default callbacks as I have always maintained since day 1: https://lwn.net/ml/linux-kernel/20200224171305.GA21886@chromium.org/ BPF LSM does not want to provide a default decision until a BPF LSM policy program is loaded, > > > > > 2. Performance, no extra function call. > > > > > > Convince me the bug still exists first and then we can discuss the > > > merits of whatever solutions are proposed. > > > > This is independent of the bug! > > Correctness first, maintainability second, performance third. That's > my current priority and I feel the maintainability hit doesn't justify > the performance win at this point in time. Besides, we're already > expecting a big performance boost simply by moving to static_calls. > > > As I said, If you don't want to modify the core LSM layer, it's okay, > > I still want to go with changes local to the BPF LSM, If you really > > don't agree with the changes local to the BPF LSM, we can have it go > > via the BPF tree and seek Linus' help to resolve the conflict. > > As the BPF maintainer you are always free to do whatever you like > within the scope of the LSM you maintain so long as it does not touch > or otherwise impact any of the other LSMs or the LSM framework. If > you do affect the other LSMs, or the LSM framework, you need to get an > ACK from the associated maintainer. That's pretty much how Linux > kernel development works. Okay, then let's not make an LSM API, I will handle it within the BPF LSM. The patch I proposed should not affect any other LSMs and is self contained within BPF LSM: https://lore.kernel.org/bpf/CACYkzJ6jADoGNuPP3-1wkk-kV7NOQh+eFkU5KEDEZgq9qNNEfg@mail.gmail.com/ > > -- > paul-moore.com
On Mon, Jul 8, 2024 at 6:04 AM KP Singh <kpsingh@kernel.org> wrote: > On Sat, Jul 6, 2024 at 6:40 AM Paul Moore <paul@paul-moore.com> wrote: > > On Fri, Jul 5, 2024 at 3:34 PM KP Singh <kpsingh@kernel.org> wrote: > > > On Fri, Jul 5, 2024 at 8:07 PM Paul Moore <paul@paul-moore.com> wrote: > > > > On Wed, Jul 3, 2024 at 7:08 PM KP Singh <kpsingh@kernel.org> wrote: > > [...] > > > > > > > Paul, I am talking about eliminating a class of bugs, but you don't > > > seem to get the point and you are fixated on the very instance of this > > > bug class. > > > > I do understand that you are trying to eliminate a class of bugs, the > > point I'm trying to make is that I believe we have addressed that > > already with the patches I've previously cited. > > The class I am referring to is useless hooks returning a default value > and imposing a denial / enforcement when they are not supposed to. If a LSM hook's default value were to result in an undesirable behavior within the kernel that would be an issue regardless of what LSMs were involved and we would either need to modify the hook and/or the default value. I am not convinced that we can adequately solve this entire class of problems simply by allowing one LSM, or even arbitrary combinations of LSMs, to disable or otherwise disconnect themselves from the framework. > > As the BPF maintainer you are always free to do whatever you like > > within the scope of the LSM you maintain so long as it does not touch > > or otherwise impact any of the other LSMs or the LSM framework. If > > you do affect the other LSMs, or the LSM framework, you need to get an > > ACK from the associated maintainer. That's pretty much how Linux > > kernel development works. > > Okay, then let's not make an LSM API, I will handle it within the BPF LSM. > > The patch I proposed should not affect any other LSMs and is self > contained within BPF LSM: > > https://lore.kernel.org/bpf/CACYkzJ6jADoGNuPP3-1wkk-kV7NOQh+eFkU5KEDEZgq9qNNEfg@mail.gmail.com/ The code changes may be self-contained within the BPF LSM, but it does appear that the bpf_lsm_toggle_hook() function directly manipulates the LSM framework hook state which is not something we want to allow - none of the individual LSMs should be directly manipulating the LSM hook state/configuration. Although perhaps I'm missing or not factoring in some context around the patch linked above and that isn't the case? While I had issues with Kees' comments, at a high level his suggestion of dropping the last patch and moving forward is something you should consider as I don't see this a good path forward with all of the approaches that have been discussed thus far.
On Mon, Jul 8, 2024 at 2:52 PM Paul Moore <paul@paul-moore.com> wrote: > > On Mon, Jul 8, 2024 at 6:04 AM KP Singh <kpsingh@kernel.org> wrote: > > On Sat, Jul 6, 2024 at 6:40 AM Paul Moore <paul@paul-moore.com> wrote: > > > On Fri, Jul 5, 2024 at 3:34 PM KP Singh <kpsingh@kernel.org> wrote: > > > > On Fri, Jul 5, 2024 at 8:07 PM Paul Moore <paul@paul-moore.com> wrote: > > > > > On Wed, Jul 3, 2024 at 7:08 PM KP Singh <kpsingh@kernel.org> wrote: > > > > [...] > > > > > > > > > > Paul, I am talking about eliminating a class of bugs, but you don't > > > > seem to get the point and you are fixated on the very instance of this > > > > bug class. > > > > > > I do understand that you are trying to eliminate a class of bugs, the > > > point I'm trying to make is that I believe we have addressed that > > > already with the patches I've previously cited. > > > > The class I am referring to is useless hooks returning a default value > > and imposing a denial / enforcement when they are not supposed to. > > If a LSM hook's default value were to result in an undesirable > behavior within the kernel that would be an issue regardless of what > LSMs were involved and we would either need to modify the hook and/or > the default value. I am not convinced that we can adequately solve > this entire class of problems simply by allowing one LSM, or even > arbitrary combinations of LSMs, to disable or otherwise disconnect > themselves from the framework. > > > > As the BPF maintainer you are always free to do whatever you like > > > within the scope of the LSM you maintain so long as it does not touch > > > or otherwise impact any of the other LSMs or the LSM framework. If > > > you do affect the other LSMs, or the LSM framework, you need to get an > > > ACK from the associated maintainer. That's pretty much how Linux > > > kernel development works. > > > > Okay, then let's not make an LSM API, I will handle it within the BPF LSM. > > > > The patch I proposed should not affect any other LSMs and is self > > contained within BPF LSM: > > > > https://lore.kernel.org/bpf/CACYkzJ6jADoGNuPP3-1wkk-kV7NOQh+eFkU5KEDEZgq9qNNEfg@mail.gmail.com/ > > The code changes may be self-contained within the BPF LSM, but it does > appear that the bpf_lsm_toggle_hook() function directly manipulates > the LSM framework hook state which is not something we want to allow - > none of the individual LSMs should be directly manipulating the LSM > hook state/configuration. Although perhaps I'm missing or not > factoring in some context around the patch linked above and that isn't > the case? I think you are ignoring my point that BPF does not want to add extraneous function calls which at the least result in extra overhead. You have ignored the fact that BPF LSM never wanted these empty callbacks and you still continue to ignore it. Sigh, I will drop it now and will propose it as a separate patch so that we can at least unblock the static call series. - KP > While I had issues with Kees' comments, at a high level his suggestion > of dropping the last patch and moving forward is something you should > consider as I don't see this a good path forward with all of the > approaches that have been discussed thus far. > > -- > paul-moore.com
On Mon, Jul 8, 2024 at 9:52 AM KP Singh <kpsingh@kernel.org> wrote: > On Mon, Jul 8, 2024 at 2:52 PM Paul Moore <paul@paul-moore.com> wrote: > > On Mon, Jul 8, 2024 at 6:04 AM KP Singh <kpsingh@kernel.org> wrote: > > > On Sat, Jul 6, 2024 at 6:40 AM Paul Moore <paul@paul-moore.com> wrote: > > > > On Fri, Jul 5, 2024 at 3:34 PM KP Singh <kpsingh@kernel.org> wrote: > > > > > On Fri, Jul 5, 2024 at 8:07 PM Paul Moore <paul@paul-moore.com> wrote: > > > > > > On Wed, Jul 3, 2024 at 7:08 PM KP Singh <kpsingh@kernel.org> wrote: ... > I think you are ignoring my point that BPF does not want to add > extraneous function calls which at the least result in extra overhead. I haven't been ignoring you on that point, see my previous comment: "Correctness first, maintainability second, performance third. That's my current priority and I feel the maintainability hit doesn't justify the performance win at this point in time. Besides, we're already expecting a big performance boost simply by moving to static_calls." https://lore.kernel.org/linux-security-module/CAHC9VhQQkWxMT3KguOOK7W8cbY-cdeYTJSuh=tSDV4jsqp6s6g@mail.gmail.com/ > You have ignored the fact that BPF LSM never wanted these empty > callbacks and you still continue to ignore it. Sigh, I will drop it > now and will propose it as a separate patch so that we can at least > unblock the static call series. I didn't comment on that because it isn't very relevant at this point in time, what matters is the current status quo and the proposed change. In this particular case I'm not going to debate decisions made by previous maintainers, my focus is on what we currently have in-tree and what/how/why people want to change. You've got a path forward with the bulk of this patchset, if you want to scuttle it over the last patch in the series that is up to you, but in my opinion that seems like a lost opportunity. -- paul-moore.com
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index efd4a0655159..a66ca68485a2 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -30,19 +30,66 @@ #include <linux/init.h> #include <linux/rculist.h> #include <linux/xattr.h> +#include <linux/static_call.h> +#include <linux/unroll.h> +#include <linux/jump_label.h> +#include <linux/lsm_count.h> + +#define SECURITY_HOOK_ACTIVE_KEY(HOOK, IDX) security_hook_active_##HOOK##_##IDX + +/* + * Identifier for the LSM static calls. + * HOOK is an LSM hook as defined in linux/lsm_hookdefs.h + * IDX is the index of the static call. 0 <= NUM < MAX_LSM_COUNT + */ +#define LSM_STATIC_CALL(HOOK, IDX) lsm_static_call_##HOOK##_##IDX + +/* + * Call the macro M for each LSM hook MAX_LSM_COUNT times. + */ +#define LSM_LOOP_UNROLL(M, ...) \ +do { \ + UNROLL(MAX_LSM_COUNT, M, __VA_ARGS__) \ +} while (0) + +#define LSM_DEFINE_UNROLL(M, ...) UNROLL(MAX_LSM_COUNT, M, __VA_ARGS__) union security_list_options { #define LSM_HOOK(RET, DEFAULT, NAME, ...) RET (*NAME)(__VA_ARGS__); #include "lsm_hook_defs.h" #undef LSM_HOOK + void *lsm_func_addr; }; -struct security_hook_heads { - #define LSM_HOOK(RET, DEFAULT, NAME, ...) struct hlist_head NAME; - #include "lsm_hook_defs.h" - #undef LSM_HOOK +/* + * @key: static call key as defined by STATIC_CALL_KEY + * @trampoline: static call trampoline as defined by STATIC_CALL_TRAMP + * @hl: The security_hook_list as initialized by the owning LSM. + * @active: Enabled when the static call has an LSM hook associated. + */ +struct lsm_static_call { + struct static_call_key *key; + void *trampoline; + struct security_hook_list *hl; + /* this needs to be true or false based on what the key defaults to */ + struct static_key_false *active; } __randomize_layout; +/* + * Table of the static calls for each LSM hook. + * Once the LSMs are initialized, their callbacks will be copied to these + * tables such that the calls are filled backwards (from last to first). + * This way, we can jump directly to the first used static call, and execute + * all of them after. This essentially makes the entry point + * dynamic to adapt the number of static calls to the number of callbacks. + */ +struct lsm_static_calls_table { + #define LSM_HOOK(RET, DEFAULT, NAME, ...) \ + struct lsm_static_call NAME[MAX_LSM_COUNT]; + #include <linux/lsm_hook_defs.h> + #undef LSM_HOOK +} __packed __randomize_layout; + /** * struct lsm_id - Identify a Linux Security Module. * @lsm: name of the LSM, must be approved by the LSM maintainers @@ -58,10 +105,14 @@ struct lsm_id { /* * Security module hook list structure. * For use with generic list macros for common operations. + * + * struct security_hook_list - Contents of a cacheable, mappable object. + * @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. */ struct security_hook_list { - struct hlist_node list; - struct hlist_head *head; + struct lsm_static_call *scalls; union security_list_options hook; const struct lsm_id *lsmid; } __randomize_layout; @@ -111,10 +162,12 @@ static inline struct xattr *lsm_get_xattr_slot(struct xattr *xattrs, * care of the common case and reduces the amount of * text involved. */ -#define LSM_HOOK_INIT(HEAD, HOOK) \ - { .head = &security_hook_heads.HEAD, .hook = { .HEAD = HOOK } } +#define LSM_HOOK_INIT(NAME, HOOK) \ + { \ + .scalls = static_calls_table.NAME, \ + .hook = { .NAME = HOOK } \ + } -extern struct security_hook_heads security_hook_heads; extern char *lsm_names; extern void security_add_hooks(struct security_hook_list *hooks, int count, @@ -152,5 +205,6 @@ extern struct lsm_info __start_early_lsm_info[], __end_early_lsm_info[]; __aligned(sizeof(unsigned long)) extern int lsm_inode_alloc(struct inode *inode); +extern struct lsm_static_calls_table static_calls_table __ro_after_init; #endif /* ! __LINUX_LSM_HOOKS_H */ diff --git a/security/security.c b/security/security.c index 9c3fb2f60e2a..e0ec185cf125 100644 --- a/security/security.c +++ b/security/security.c @@ -30,6 +30,8 @@ #include <linux/overflow.h> #include <net/flow.h> #include <net/sock.h> +#include <linux/static_call.h> +#include <linux/jump_label.h> /* How many LSMs were built into the kernel? */ #define LSM_COUNT (__end_lsm_info - __start_lsm_info) @@ -93,7 +95,6 @@ const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX + 1] = { [LOCKDOWN_CONFIDENTIALITY_MAX] = "confidentiality", }; -struct security_hook_heads security_hook_heads __ro_after_init; static BLOCKING_NOTIFIER_HEAD(blocking_lsm_notifier_chain); static struct kmem_cache *lsm_file_cache; @@ -112,6 +113,51 @@ static __initconst const char *const builtin_lsm_order = CONFIG_LSM; static __initdata struct lsm_info **ordered_lsms; static __initdata struct lsm_info *exclusive; + +#ifdef CONFIG_HAVE_STATIC_CALL +#define LSM_HOOK_TRAMP(NAME, NUM) \ + &STATIC_CALL_TRAMP(LSM_STATIC_CALL(NAME, NUM)) +#else +#define LSM_HOOK_TRAMP(NAME, NUM) NULL +#endif + +/* + * Define static calls and static keys for each LSM hook. + */ + +#define DEFINE_LSM_STATIC_CALL(NUM, NAME, RET, ...) \ + DEFINE_STATIC_CALL_NULL(LSM_STATIC_CALL(NAME, NUM), \ + *((RET(*)(__VA_ARGS__))NULL)); \ + DEFINE_STATIC_KEY_FALSE(SECURITY_HOOK_ACTIVE_KEY(NAME, NUM)); + +#define LSM_HOOK(RET, DEFAULT, NAME, ...) \ + LSM_DEFINE_UNROLL(DEFINE_LSM_STATIC_CALL, NAME, RET, __VA_ARGS__) +#include <linux/lsm_hook_defs.h> +#undef LSM_HOOK +#undef DEFINE_LSM_STATIC_CALL + +/* + * Initialise a table of static calls for each LSM hook. + * DEFINE_STATIC_CALL_NULL invocation above generates a key (STATIC_CALL_KEY) + * and a trampoline (STATIC_CALL_TRAMP) which are used to call + * __static_call_update when updating the static call. + */ +struct lsm_static_calls_table static_calls_table __ro_after_init = { +#define INIT_LSM_STATIC_CALL(NUM, NAME) \ + (struct lsm_static_call) { \ + .key = &STATIC_CALL_KEY(LSM_STATIC_CALL(NAME, NUM)), \ + .trampoline = LSM_HOOK_TRAMP(NAME, NUM), \ + .active = &SECURITY_HOOK_ACTIVE_KEY(NAME, NUM), \ + }, +#define LSM_HOOK(RET, DEFAULT, NAME, ...) \ + .NAME = { \ + LSM_DEFINE_UNROLL(INIT_LSM_STATIC_CALL, NAME) \ + }, +#include <linux/lsm_hook_defs.h> +#undef LSM_HOOK +#undef INIT_LSM_STATIC_CALL +}; + static __initdata bool debug; #define init_debug(...) \ do { \ @@ -172,7 +218,7 @@ static void __init append_ordered_lsm(struct lsm_info *lsm, const char *from) if (exists_ordered_lsm(lsm)) return; - if (WARN(last_lsm == LSM_COUNT, "%s: out of LSM slots!?\n", from)) + if (WARN(last_lsm == LSM_COUNT, "%s: out of LSM static calls!?\n", from)) return; /* Enable this LSM, if it is not already set. */ @@ -352,6 +398,25 @@ static void __init ordered_lsm_parse(const char *order, const char *origin) kfree(sep); } +static void __init lsm_static_call_init(struct security_hook_list *hl) +{ + struct lsm_static_call *scall = hl->scalls; + int i; + + for (i = 0; i < MAX_LSM_COUNT; i++) { + /* Update the first static call that is not used yet */ + if (!scall->hl) { + __static_call_update(scall->key, scall->trampoline, + hl->hook.lsm_func_addr); + scall->hl = hl; + static_branch_enable(scall->active); + return; + } + scall++; + } + panic("%s - Ran out of static slots.\n", __func__); +} + static void __init lsm_early_cred(struct cred *cred); static void __init lsm_early_task(struct task_struct *task); @@ -432,11 +497,6 @@ int __init early_security_init(void) { struct lsm_info *lsm; -#define LSM_HOOK(RET, DEFAULT, NAME, ...) \ - INIT_HLIST_HEAD(&security_hook_heads.NAME); -#include "linux/lsm_hook_defs.h" -#undef LSM_HOOK - for (lsm = __start_early_lsm_info; lsm < __end_early_lsm_info; lsm++) { if (!lsm->enabled) lsm->enabled = &lsm_enabled_true; @@ -564,7 +624,7 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count, for (i = 0; i < count; i++) { hooks[i].lsmid = lsmid; - hlist_add_tail_rcu(&hooks[i].list, hooks[i].head); + lsm_static_call_init(&hooks[i]); } /* @@ -856,29 +916,43 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u32 *uctx_len, * call_int_hook: * This is a hook that returns a value. */ +#define __CALL_STATIC_VOID(NUM, HOOK, ...) \ +do { \ + if (static_branch_unlikely(&SECURITY_HOOK_ACTIVE_KEY(HOOK, NUM))) { \ + static_call(LSM_STATIC_CALL(HOOK, NUM))(__VA_ARGS__); \ + } \ +} while (0); -#define call_void_hook(FUNC, ...) \ - do { \ - struct security_hook_list *P; \ - \ - hlist_for_each_entry(P, &security_hook_heads.FUNC, list) \ - P->hook.FUNC(__VA_ARGS__); \ +#define call_void_hook(HOOK, ...) \ + do { \ + LSM_LOOP_UNROLL(__CALL_STATIC_VOID, HOOK, __VA_ARGS__); \ } while (0) -#define call_int_hook(FUNC, ...) ({ \ - int RC = LSM_RET_DEFAULT(FUNC); \ - do { \ - struct security_hook_list *P; \ - \ - hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \ - RC = P->hook.FUNC(__VA_ARGS__); \ - if (RC != LSM_RET_DEFAULT(FUNC)) \ - break; \ - } \ - } while (0); \ - RC; \ + +#define __CALL_STATIC_INT(NUM, R, HOOK, LABEL, ...) \ +do { \ + if (static_branch_unlikely(&SECURITY_HOOK_ACTIVE_KEY(HOOK, NUM))) { \ + R = static_call(LSM_STATIC_CALL(HOOK, NUM))(__VA_ARGS__); \ + if (R != LSM_RET_DEFAULT(HOOK)) \ + goto LABEL; \ + } \ +} while (0); + +#define call_int_hook(HOOK, ...) \ +({ \ + __label__ out; \ + int RC = LSM_RET_DEFAULT(HOOK); \ + \ + LSM_LOOP_UNROLL(__CALL_STATIC_INT, RC, HOOK, out, __VA_ARGS__); \ +out: \ + RC; \ }) +#define lsm_for_each_hook(scall, NAME) \ + for (scall = static_calls_table.NAME; \ + scall - static_calls_table.NAME < MAX_LSM_COUNT; scall++) \ + if (static_key_enabled(&scall->active->key)) + /* Security operations */ /** @@ -1113,7 +1187,7 @@ int security_settime64(const struct timespec64 *ts, const struct timezone *tz) */ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages) { - struct security_hook_list *hp; + struct lsm_static_call *scall; int cap_sys_admin = 1; int rc; @@ -1124,8 +1198,8 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages) * agree that it should be set it will. If any module * thinks it should not be set it won't. */ - hlist_for_each_entry(hp, &security_hook_heads.vm_enough_memory, list) { - rc = hp->hook.vm_enough_memory(mm, pages); + lsm_for_each_hook(scall, vm_enough_memory) { + rc = scall->hl->hook.vm_enough_memory(mm, pages); if (rc <= 0) { cap_sys_admin = 0; break; @@ -1272,13 +1346,12 @@ int security_fs_context_dup(struct fs_context *fc, struct fs_context *src_fc) int security_fs_context_parse_param(struct fs_context *fc, struct fs_parameter *param) { - struct security_hook_list *hp; + struct lsm_static_call *scall; int trc; int rc = -ENOPARAM; - hlist_for_each_entry(hp, &security_hook_heads.fs_context_parse_param, - list) { - trc = hp->hook.fs_context_parse_param(fc, param); + lsm_for_each_hook(scall, fs_context_parse_param) { + trc = scall->hl->hook.fs_context_parse_param(fc, param); if (trc == 0) rc = 0; else if (trc != -ENOPARAM) @@ -1508,12 +1581,11 @@ int security_sb_set_mnt_opts(struct super_block *sb, unsigned long kern_flags, unsigned long *set_kern_flags) { - struct security_hook_list *hp; + struct lsm_static_call *scall; int rc = mnt_opts ? -EOPNOTSUPP : LSM_RET_DEFAULT(sb_set_mnt_opts); - hlist_for_each_entry(hp, &security_hook_heads.sb_set_mnt_opts, - list) { - rc = hp->hook.sb_set_mnt_opts(sb, mnt_opts, kern_flags, + lsm_for_each_hook(scall, sb_set_mnt_opts) { + rc = scall->hl->hook.sb_set_mnt_opts(sb, mnt_opts, kern_flags, set_kern_flags); if (rc != LSM_RET_DEFAULT(sb_set_mnt_opts)) break; @@ -1708,7 +1780,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir, const struct qstr *qstr, const initxattrs initxattrs, void *fs_data) { - struct security_hook_list *hp; + struct lsm_static_call *scall; struct xattr *new_xattrs = NULL; int ret = -EOPNOTSUPP, xattr_count = 0; @@ -1726,9 +1798,8 @@ int security_inode_init_security(struct inode *inode, struct inode *dir, return -ENOMEM; } - hlist_for_each_entry(hp, &security_hook_heads.inode_init_security, - list) { - ret = hp->hook.inode_init_security(inode, dir, qstr, new_xattrs, + lsm_for_each_hook(scall, inode_init_security) { + ret = scall->hl->hook.inode_init_security(inode, dir, qstr, new_xattrs, &xattr_count); if (ret && ret != -EOPNOTSUPP) goto out; @@ -3560,10 +3631,10 @@ int security_task_prctl(int option, unsigned long arg2, unsigned long arg3, { int thisrc; int rc = LSM_RET_DEFAULT(task_prctl); - struct security_hook_list *hp; + struct lsm_static_call *scall; - hlist_for_each_entry(hp, &security_hook_heads.task_prctl, list) { - thisrc = hp->hook.task_prctl(option, arg2, arg3, arg4, arg5); + lsm_for_each_hook(scall, task_prctl) { + thisrc = scall->hl->hook.task_prctl(option, arg2, arg3, arg4, arg5); if (thisrc != LSM_RET_DEFAULT(task_prctl)) { rc = thisrc; if (thisrc != 0) @@ -3969,7 +4040,7 @@ EXPORT_SYMBOL(security_d_instantiate); int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx, u32 __user *size, u32 flags) { - struct security_hook_list *hp; + struct lsm_static_call *scall; struct lsm_ctx lctx = { .id = LSM_ID_UNDEF, }; u8 __user *base = (u8 __user *)uctx; u32 entrysize; @@ -4007,13 +4078,13 @@ int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx, * In the usual case gather all the data from the LSMs. * In the single case only get the data from the LSM specified. */ - hlist_for_each_entry(hp, &security_hook_heads.getselfattr, list) { - if (single && lctx.id != hp->lsmid->id) + lsm_for_each_hook(scall, getselfattr) { + if (single && lctx.id != scall->hl->lsmid->id) continue; entrysize = left; if (base) uctx = (struct lsm_ctx __user *)(base + total); - rc = hp->hook.getselfattr(attr, uctx, &entrysize, flags); + rc = scall->hl->hook.getselfattr(attr, uctx, &entrysize, flags); if (rc == -EOPNOTSUPP) { rc = 0; continue; @@ -4062,7 +4133,7 @@ int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx, int security_setselfattr(unsigned int attr, struct lsm_ctx __user *uctx, u32 size, u32 flags) { - struct security_hook_list *hp; + struct lsm_static_call *scall; struct lsm_ctx *lctx; int rc = LSM_RET_DEFAULT(setselfattr); u64 required_len; @@ -4085,9 +4156,9 @@ int security_setselfattr(unsigned int attr, struct lsm_ctx __user *uctx, goto free_out; } - hlist_for_each_entry(hp, &security_hook_heads.setselfattr, list) - if ((hp->lsmid->id) == lctx->id) { - rc = hp->hook.setselfattr(attr, lctx, size, flags); + lsm_for_each_hook(scall, setselfattr) + if ((scall->hl->lsmid->id) == lctx->id) { + rc = scall->hl->hook.setselfattr(attr, lctx, size, flags); break; } @@ -4110,12 +4181,12 @@ int security_setselfattr(unsigned int attr, struct lsm_ctx __user *uctx, int security_getprocattr(struct task_struct *p, int lsmid, const char *name, char **value) { - struct security_hook_list *hp; + struct lsm_static_call *scall; - hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) { - if (lsmid != 0 && lsmid != hp->lsmid->id) + lsm_for_each_hook(scall, getprocattr) { + if (lsmid != 0 && lsmid != scall->hl->lsmid->id) continue; - return hp->hook.getprocattr(p, name, value); + return scall->hl->hook.getprocattr(p, name, value); } return LSM_RET_DEFAULT(getprocattr); } @@ -4134,12 +4205,12 @@ int security_getprocattr(struct task_struct *p, int lsmid, const char *name, */ int security_setprocattr(int lsmid, const char *name, void *value, size_t size) { - struct security_hook_list *hp; + struct lsm_static_call *scall; - hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) { - if (lsmid != 0 && lsmid != hp->lsmid->id) + lsm_for_each_hook(scall, setprocattr) { + if (lsmid != 0 && lsmid != scall->hl->lsmid->id) continue; - return hp->hook.setprocattr(name, value, size); + return scall->hl->hook.setprocattr(name, value, size); } return LSM_RET_DEFAULT(setprocattr); } @@ -5257,7 +5328,7 @@ int security_xfrm_state_pol_flow_match(struct xfrm_state *x, struct xfrm_policy *xp, const struct flowi_common *flic) { - struct security_hook_list *hp; + struct lsm_static_call *scall; int rc = LSM_RET_DEFAULT(xfrm_state_pol_flow_match); /* @@ -5269,9 +5340,8 @@ int security_xfrm_state_pol_flow_match(struct xfrm_state *x, * For speed optimization, we explicitly break the loop rather than * using the macro */ - hlist_for_each_entry(hp, &security_hook_heads.xfrm_state_pol_flow_match, - list) { - rc = hp->hook.xfrm_state_pol_flow_match(x, xp, flic); + lsm_for_each_hook(scall, xfrm_state_pol_flow_match) { + rc = scall->hl->hook.xfrm_state_pol_flow_match(x, xp, flic); break; } return rc;
LSM hooks are currently invoked from a linked list as indirect calls which are invoked using retpolines as a mitigation for speculative attacks (Branch History / Target injection) and add extra overhead which is especially bad in kernel hot paths: security_file_ioctl: 0xff...0320 <+0>: endbr64 0xff...0324 <+4>: push %rbp 0xff...0325 <+5>: push %r15 0xff...0327 <+7>: push %r14 0xff...0329 <+9>: push %rbx 0xff...032a <+10>: mov %rdx,%rbx 0xff...032d <+13>: mov %esi,%ebp 0xff...032f <+15>: mov %rdi,%r14 0xff...0332 <+18>: mov $0xff...7030,%r15 0xff...0339 <+25>: mov (%r15),%r15 0xff...033c <+28>: test %r15,%r15 0xff...033f <+31>: je 0xff...0358 <security_file_ioctl+56> 0xff...0341 <+33>: mov 0x18(%r15),%r11 0xff...0345 <+37>: mov %r14,%rdi 0xff...0348 <+40>: mov %ebp,%esi 0xff...034a <+42>: mov %rbx,%rdx 0xff...034d <+45>: call 0xff...2e0 <__x86_indirect_thunk_array+352> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Indirect calls that use retpolines leading to overhead, not just due to extra instruction but also branch misses. 0xff...0352 <+50>: test %eax,%eax 0xff...0354 <+52>: je 0xff...0339 <security_file_ioctl+25> 0xff...0356 <+54>: jmp 0xff...035a <security_file_ioctl+58> 0xff...0358 <+56>: xor %eax,%eax 0xff...035a <+58>: pop %rbx 0xff...035b <+59>: pop %r14 0xff...035d <+61>: pop %r15 0xff...035f <+63>: pop %rbp 0xff...0360 <+64>: jmp 0xff...47c4 <__x86_return_thunk> The indirect calls are not really needed as one knows the addresses of enabled LSM callbacks at boot time and only the order can possibly change at boot time with the lsm= kernel command line parameter. An array of static calls is defined per LSM hook and the static calls are updated at boot time once the order has been determined. A static key guards whether an LSM static call is enabled or not, without this static key, for LSM hooks that return an int, the presence of the hook that returns a default value can create side-effects which has resulted in bugs [1]. With the hook now exposed as a static call, one can see that the retpolines are no longer there and the LSM callbacks are invoked directly: security_file_ioctl: 0xff...0ca0 <+0>: endbr64 0xff...0ca4 <+4>: nopl 0x0(%rax,%rax,1) 0xff...0ca9 <+9>: push %rbp 0xff...0caa <+10>: push %r14 0xff...0cac <+12>: push %rbx 0xff...0cad <+13>: mov %rdx,%rbx 0xff...0cb0 <+16>: mov %esi,%ebp 0xff...0cb2 <+18>: mov %rdi,%r14 0xff...0cb5 <+21>: jmp 0xff...0cc7 <security_file_ioctl+39> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Static key enabled for SELinux 0xffffffff818f0cb7 <+23>: jmp 0xff...0cde <security_file_ioctl+62> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Static key enabled for BPF LSM. This is something that is changed to default to false to avoid the existing side effect issues of BPF LSM [1] in a subsequent patch. 0xff...0cb9 <+25>: xor %eax,%eax 0xff...0cbb <+27>: xchg %ax,%ax 0xff...0cbd <+29>: pop %rbx 0xff...0cbe <+30>: pop %r14 0xff...0cc0 <+32>: pop %rbp 0xff...0cc1 <+33>: cs jmp 0xff...0000 <__x86_return_thunk> 0xff...0cc7 <+39>: endbr64 0xff...0ccb <+43>: mov %r14,%rdi 0xff...0cce <+46>: mov %ebp,%esi 0xff...0cd0 <+48>: mov %rbx,%rdx 0xff...0cd3 <+51>: call 0xff...3230 <selinux_file_ioctl> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Direct call to SELinux. 0xff...0cd8 <+56>: test %eax,%eax 0xff...0cda <+58>: jne 0xff...0cbd <security_file_ioctl+29> 0xff...0cdc <+60>: jmp 0xff...0cb7 <security_file_ioctl+23> 0xff...0cde <+62>: endbr64 0xff...0ce2 <+66>: mov %r14,%rdi 0xff...0ce5 <+69>: mov %ebp,%esi 0xff...0ce7 <+71>: mov %rbx,%rdx 0xff...0cea <+74>: call 0xff...e220 <bpf_lsm_file_ioctl> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Direct call to BPF LSM. 0xff...0cef <+79>: test %eax,%eax 0xff...0cf1 <+81>: jne 0xff...0cbd <security_file_ioctl+29> 0xff...0cf3 <+83>: jmp 0xff...0cb9 <security_file_ioctl+25> 0xff...0cf5 <+85>: endbr64 0xff...0cf9 <+89>: mov %r14,%rdi 0xff...0cfc <+92>: mov %ebp,%esi 0xff...0cfe <+94>: mov %rbx,%rdx 0xff...0d01 <+97>: pop %rbx 0xff...0d02 <+98>: pop %r14 0xff...0d04 <+100>: pop %rbp 0xff...0d05 <+101>: ret 0xff...0d06 <+102>: int3 0xff...0d07 <+103>: int3 0xff...0d08 <+104>: int3 0xff...0d09 <+105>: int3 While this patch uses static_branch_unlikely indicating that an LSM hook is likely to be not present. In most cases this is still a better choice as even when an LSM with one hook is added, empty slots are created for all LSM hooks (especially when many LSMs that do not initialize most hooks are present on the system). There are some hooks that don't use the call_int_hook and call_void_hook. These hooks are updated to use a new macro called lsm_for_each_hook where the lsm_callback is directly invoked as an indirect call. These are updated in a subsequent patch to also use static calls. Below are results of the relevant Unixbench system benchmarks with BPF LSM and SELinux enabled with default policies enabled with and without these patches. Benchmark Delta(%): (+ is better) =============================================================================== Execl Throughput +1.9356 File Write 1024 bufsize 2000 maxblocks +6.5953 Pipe Throughput +9.5499 Pipe-based Context Switching +3.0209 Process Creation +2.3246 Shell Scripts (1 concurrent) +1.4975 System Call Overhead +2.7815 System Benchmarks Index Score (Partial Only): +3.4859 In the best case, some syscalls like eventfd_create benefitted to about ~10%. [1] https://lore.kernel.org/linux-security-module/20220609234601.2026362-1-kpsingh@kernel.org/ Reviewed-by: Casey Schaufler <casey@schaufler-ca.com> Reviewed-by: Kees Cook <keescook@chromium.org> Acked-by: Song Liu <song@kernel.org> Acked-by: Andrii Nakryiko <andrii@kernel.org> Signed-off-by: KP Singh <kpsingh@kernel.org> --- include/linux/lsm_hooks.h | 72 ++++++++++++-- security/security.c | 198 ++++++++++++++++++++++++++------------ 2 files changed, 197 insertions(+), 73 deletions(-)