| Message ID | 20230120000818.1324170-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 Fri, Jan 20, 2023 at 01:08:17AM +0100, KP Singh wrote: > 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 think this patch has too many logic changes in it. There are basically two things going on here: - replace list with unrolled calls - replace calls with static calls I see why it was merged, since some of the logic that would be added for step 1 would be immediate replaced, but I think it might make things a bit more clear. There is likely a few intermediate steps here too, to rename things, etc. > 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 > security_for_each_hook where the lsm_callback is directly invoked as an > indirect call. Currently, there are no performance sensitive hooks that > use the security_for_each_hook macro. However, if, some performance > sensitive hooks are discovered, these can be updated to use > static calls with loop unrolling as well using a custom macro. > > [1] https://lore.kernel.org/linux-security-module/20220609234601.2026362-1-kpsingh@kernel.org/ > > Signed-off-by: KP Singh <kpsingh@kernel.org> > --- > include/linux/lsm_hooks.h | 83 +++++++++++++-- > security/security.c | 216 ++++++++++++++++++++++++-------------- > 2 files changed, 211 insertions(+), 88 deletions(-) > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index 0a5ba81f7367..c82d15a4ef50 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -28,6 +28,26 @@ > #include <linux/security.h> > #include <linux/init.h> > #include <linux/rculist.h> > +#include <linux/static_call.h> > +#include <linux/unroll.h> > +#include <linux/jump_label.h> > + > +/* Include the generated MAX_LSM_COUNT */ > +#include <generated/lsm_count.h> > + > +#define SECURITY_HOOK_ENABLED_KEY(HOOK, IDX) security_enabled_key_##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_UNROLL(M, ...) UNROLL(MAX_LSM_COUNT, M, __VA_ARGS__) I think this should be: #define LSM_UNROLL(M, ...) do { \ UNROLL(MAX_LSM_COUNT, M, __VA_ARGS__); \ } while (0) or maybe UNROLL needs the do/while. > > /** > * union security_list_options - Linux Security Module hook function list > @@ -1657,21 +1677,48 @@ union security_list_options { > #define LSM_HOOK(RET, DEFAULT, NAME, ...) RET (*NAME)(__VA_ARGS__); > #include "lsm_hook_defs.h" > #undef LSM_HOOK > + void *lsm_callback; > }; > > -struct security_hook_heads { > - #define LSM_HOOK(RET, DEFAULT, NAME, ...) struct hlist_head NAME; > - #include "lsm_hook_defs.h" > +/* > + * @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. > + * @enabled_key: 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; > + struct static_key *enabled_key; > +}; > + > +/* > + * 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 > } __randomize_layout; > > /* > * 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 char *lsm; > } __randomize_layout; > @@ -1701,10 +1748,12 @@ struct lsm_blob_sizes { > * 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, CALLBACK) \ > + { \ > + .scalls = static_calls_table.NAME, \ > + .hook = { .NAME = CALLBACK } \ > + } > > -extern struct security_hook_heads security_hook_heads; > extern char *lsm_names; > > extern void security_add_hooks(struct security_hook_list *hooks, int count, > @@ -1756,10 +1805,21 @@ extern struct lsm_info __start_early_lsm_info[], __end_early_lsm_info[]; > static inline void security_delete_hooks(struct security_hook_list *hooks, > int count) Hey Paul, can we get rid of CONFIG_SECURITY_SELINUX_DISABLE yet? It's been deprecated for years.... > { > - int i; > + struct lsm_static_call *scalls; > + int i, j; > + > + for (i = 0; i < count; i++) { > + scalls = hooks[i].scalls; > + for (j = 0; j < MAX_LSM_COUNT; j++) { > + if (scalls[j].hl != &hooks[i]) > + continue; > > - for (i = 0; i < count; i++) > - hlist_del_rcu(&hooks[i].list); > + static_key_disable(scalls[j].enabled_key); > + __static_call_update(scalls[j].key, > + scalls[j].trampoline, NULL); > + scalls[j].hl = NULL; > + } > + } > } > #endif /* CONFIG_SECURITY_SELINUX_DISABLE */ > > @@ -1771,5 +1831,6 @@ static inline void security_delete_hooks(struct security_hook_list *hooks, > #endif /* CONFIG_SECURITY_WRITABLE_HOOKS */ > > 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 d1571900a8c7..e54d5ba187d1 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -29,6 +29,8 @@ > #include <linux/string.h> > #include <linux/msg.h> > #include <net/flow.h> > +#include <linux/static_call.h> > +#include <linux/jump_label.h> > > #define MAX_LSM_EVM_XATTR 2 > > @@ -74,7 +76,6 @@ const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = { > [LOCKDOWN_CONFIDENTIALITY_MAX] = "confidentiality", > }; > > -struct security_hook_heads security_hook_heads __lsm_ro_after_init; > static BLOCKING_NOTIFIER_HEAD(blocking_lsm_notifier_chain); > > static struct kmem_cache *lsm_file_cache; > @@ -93,6 +94,43 @@ static __initconst const char * const builtin_lsm_order = CONFIG_LSM; > static __initdata struct lsm_info **ordered_lsms; > static __initdata struct lsm_info *exclusive; > > +/* > + * 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_ENABLED_KEY(NAME, NUM)); Hm, another place where we would benefit from having separated logic for "is it built?" and "is it enabled by default?" and we could use DEFINE_STATIC_KEY_MAYBE(). But, since we don't, I think we need to use DEFINE_STATIC_KEY_TRUE() here or else won't all the calls be out-of-line? (i.e. the default compiled state will be NOPs?) If we're trying to optimize for having LSMs, I think we should default to inline calls. (The machine code in the commit log seems to indicate that they are out of line -- it uses jumps.) > [...] > +#define __CALL_STATIC_VOID(NUM, HOOK, ...) \ > + if (static_branch_unlikely(&SECURITY_HOOK_ENABLED_KEY(HOOK, NUM))) { \ > + static_call(LSM_STATIC_CALL(HOOK, NUM))(__VA_ARGS__); \ > + } Same here -- I would expect this to be static_branch_likely() or we'll get out-of-line branches. Also, IMO, this should be: do { if (...) static_call(...); } while (0) > +#define call_void_hook(FUNC, ...) \ > + do { \ > + LSM_UNROLL(__CALL_STATIC_VOID, FUNC, __VA_ARGS__) \ > } while (0) With the do/while in LSM_UNROLL, this is more readable. > > -#define call_int_hook(FUNC, IRC, ...) ({ \ > - int RC = IRC; \ > - do { \ > - struct security_hook_list *P; \ > - \ > - hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \ > - RC = P->hook.FUNC(__VA_ARGS__); \ > - if (RC != 0) \ > - break; \ > - } \ > - } while (0); \ > - RC; \ > -}) > +#define __CALL_STATIC_INT(NUM, R, HOOK, ...) \ > + if (static_branch_unlikely(&SECURITY_HOOK_ENABLED_KEY(HOOK, NUM))) { \ > + R = static_call(LSM_STATIC_CALL(HOOK, NUM))(__VA_ARGS__); \ > + if (R != 0) \ > + goto out; \ > + } I would expect the label to be a passed argument, but maybe since it never changes, it's fine as-is? And again, I'd expect a do/while wrapper, and for it to be s_b_likely. > + > +#define call_int_hook(FUNC, IRC, ...) \ > + ({ \ > + __label__ out; \ > + int RC = IRC; \ > + do { \ > + LSM_UNROLL(__CALL_STATIC_INT, RC, FUNC, __VA_ARGS__) \ > + \ > + } while (0); \ Then this becomes: ({ int RC = IRC; LSM_UNROLL(__CALL_STATIC_INT, RC, FUNC, __VA_ARGS__); out: RC; }) > +#define security_for_each_hook(scall, NAME, expression) \ > + for (scall = static_calls_table.NAME; \ > + scall - static_calls_table.NAME < MAX_LSM_COUNT; scall++) { \ > + if (!static_key_enabled(scall->enabled_key)) \ > + continue; \ > + (expression); \ > + } Why isn't this using static_branch_enabled()? I would expect this to be: #define security_for_each_hook(scall, NAME) \ for (scall = static_calls_table.NAME; \ scall - static_calls_table.NAME < MAX_LSM_COUNT; scall++) \ if (static_branch_likely(scall->enabled_key)) > > /* Security operations */ > > @@ -859,7 +924,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; > > @@ -870,13 +935,13 @@ 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); > + security_for_each_hook(scall, vm_enough_memory, ({ > + rc = scall->hl->hook.vm_enough_memory(mm, pages); > if (rc <= 0) { > cap_sys_admin = 0; > break; > } > - } > + })); Then these replacements don't look weird. This would just be: security_for_each_hook(scall, vm_enough_memory) { rc = scall->hl->hook.vm_enough_memory(mm, pages); if (rc <= 0) { cap_sys_admin = 0; break; } } I'm excited to have this. The speed improvements are pretty nice.
Hi KP,
I love your patch! Yet something to improve:
[auto build test ERROR on bpf-next/master]
url: https://github.com/intel-lab-lkp/linux/commits/KP-Singh/kernel-Add-helper-macros-for-loop-unrolling/20230120-133309
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link: https://lore.kernel.org/r/20230120000818.1324170-4-kpsingh%40kernel.org
patch subject: [PATCH RESEND bpf-next 3/4] security: Replace indirect LSM hook calls with static calls
config: arc-randconfig-r043-20230119 (https://download.01.org/0day-ci/archive/20230120/202301201833.YM7Hr62n-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/1dfb90849b42d8af6e854cd6ae8cd96d1ebfc50a
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review KP-Singh/kernel-Add-helper-macros-for-loop-unrolling/20230120-133309
git checkout 1dfb90849b42d8af6e854cd6ae8cd96d1ebfc50a
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash kernel/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from include/linux/bpf_lsm.h:12,
from kernel/bpf/syscall.c:31:
>> include/linux/lsm_hooks.h:36:10: fatal error: generated/lsm_count.h: No such file or directory
36 | #include <generated/lsm_count.h>
| ^~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
vim +36 include/linux/lsm_hooks.h
34
35 /* Include the generated MAX_LSM_COUNT */
> 36 #include <generated/lsm_count.h>
37
Hi KP,
I love your patch! Yet something to improve:
[auto build test ERROR on bpf-next/master]
url: https://github.com/intel-lab-lkp/linux/commits/KP-Singh/kernel-Add-helper-macros-for-loop-unrolling/20230120-133309
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link: https://lore.kernel.org/r/20230120000818.1324170-4-kpsingh%40kernel.org
patch subject: [PATCH RESEND bpf-next 3/4] security: Replace indirect LSM hook calls with static calls
config: hexagon-randconfig-r041-20230119 (https://download.01.org/0day-ci/archive/20230120/202301201845.mf9dWfym-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 4196ca3278f78c6e19246e54ab0ecb364e37d66a)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/1dfb90849b42d8af6e854cd6ae8cd96d1ebfc50a
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review KP-Singh/kernel-Add-helper-macros-for-loop-unrolling/20230120-133309
git checkout 1dfb90849b42d8af6e854cd6ae8cd96d1ebfc50a
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash kernel/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from kernel/bpf/syscall.c:5:
In file included from include/linux/bpf-cgroup.h:11:
In file included from include/net/sock.h:38:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
^
In file included from kernel/bpf/syscall.c:5:
In file included from include/linux/bpf-cgroup.h:11:
In file included from include/net/sock.h:38:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
^
In file included from kernel/bpf/syscall.c:5:
In file included from include/linux/bpf-cgroup.h:11:
In file included from include/net/sock.h:38:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
In file included from kernel/bpf/syscall.c:31:
In file included from include/linux/bpf_lsm.h:12:
>> include/linux/lsm_hooks.h:36:10: fatal error: 'generated/lsm_count.h' file not found
#include <generated/lsm_count.h>
^~~~~~~~~~~~~~~~~~~~~~~
6 warnings and 1 error generated.
vim +36 include/linux/lsm_hooks.h
34
35 /* Include the generated MAX_LSM_COUNT */
> 36 #include <generated/lsm_count.h>
37
On 1/19/2023 8:36 PM, Kees Cook wrote: > On Fri, Jan 20, 2023 at 01:08:17AM +0100, KP Singh wrote: >> 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. >> >> ... > Then these replacements don't look weird. This would just be: > > security_for_each_hook(scall, vm_enough_memory) { > rc = scall->hl->hook.vm_enough_memory(mm, pages); > if (rc <= 0) { > cap_sys_admin = 0; > break; > } > } That's a whole lot easier to swallow than what was originally proposed. > > I'm excited to have this. The speed improvements are pretty nice. >
On Fri, Jan 20, 2023 at 5:36 AM Kees Cook <keescook@chromium.org> wrote: > > On Fri, Jan 20, 2023 at 01:08:17AM +0100, KP Singh wrote: > > The indirect calls are not really needed as one knows the addresses of [...] > > 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 think this patch has too many logic changes in it. There are basically > two things going on here: > > - replace list with unrolled calls > - replace calls with static calls > > I see why it was merged, since some of the logic that would be added for > step 1 would be immediate replaced, but I think it might make things a > bit more clear. > > There is likely a few intermediate steps here too, to rename things, > etc. I can try to split this patch, but I am not able to find a decent slice without duplicating a lot of work which will get squashed in the end anyways. > > > 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 > > security_for_each_hook where the lsm_callback is directly invoked as an [...] > > + * Call the macro M for each LSM hook MAX_LSM_COUNT times. > > + */ > > +#define LSM_UNROLL(M, ...) UNROLL(MAX_LSM_COUNT, M, __VA_ARGS__) > > I think this should be: > > #define LSM_UNROLL(M, ...) do { \ > UNROLL(MAX_LSM_COUNT, M, __VA_ARGS__); \ > } while (0) > > or maybe UNROLL needs the do/while. UNROLL is used for both declaration and loop unrolling. So I have split the LSM macros into LSM_UNROLL to LSM_LOOP_UNROLL (which is surrounded by a do/while) and an LSM_DEFINE_UNROLL which is not. > > > > > /** > > * union security_list_options - Linux Security Module hook function list > > @@ -1657,21 +1677,48 @@ union security_list_options { > > #define LSM_HOOK(RET, DEFAULT, NAME, ...) RET (*NAME)(__VA_ARGS__); > > #include "lsm_hook_defs.h" > > #undef LSM_HOOK > > + void *lsm_callback; > > }; > > > > -struct security_hook_heads { > > - #define LSM_HOOK(RET, DEFAULT, NAME, ...) struct hlist_head NAME; > > - #include "lsm_hook_defs.h" > > +/* > > + * @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. [...] > > + struct lsm_static_call NAME[MAX_LSM_COUNT]; > > + #include <linux/lsm_hook_defs.h> > > #undef LSM_HOOK > > } __randomize_layout; > > > > /* > > * Security module hook list structure. > > * For use with generic list macros for common operations. > > + * > > + * struct security_hook_list - Contents of a cacheable, mappable object. [...] > > -#define LSM_HOOK_INIT(HEAD, HOOK) \ > > - { .head = &security_hook_heads.HEAD, .hook = { .HEAD = HOOK } } > > +#define LSM_HOOK_INIT(NAME, CALLBACK) \ > > + { \ > > + .scalls = static_calls_table.NAME, \ > > + .hook = { .NAME = CALLBACK } \ > > + } > > > > -extern struct security_hook_heads security_hook_heads; > > extern char *lsm_names; > > > > extern void security_add_hooks(struct security_hook_list *hooks, int count, > > @@ -1756,10 +1805,21 @@ extern struct lsm_info __start_early_lsm_info[], __end_early_lsm_info[]; > > static inline void security_delete_hooks(struct security_hook_list *hooks, > > int count) > > Hey Paul, can we get rid of CONFIG_SECURITY_SELINUX_DISABLE yet? It's > been deprecated for years.... > > > { > > - int i; > > + struct lsm_static_call *scalls; > > + int i, j; > > + > > + for (i = 0; i < count; i++) { > > + scalls = hooks[i].scalls; > > + for (j = 0; j < MAX_LSM_COUNT; j++) { > > + if (scalls[j].hl != &hooks[i]) > > + continue; > > > > - for (i = 0; i < count; i++) > > - hlist_del_rcu(&hooks[i].list); > > + static_key_disable(scalls[j].enabled_key); > > + __static_call_update(scalls[j].key, > > + scalls[j].trampoline, NULL); > > + scalls[j].hl = NULL; > > + } > > + } > > } > > #endif /* CONFIG_SECURITY_SELINUX_DISABLE */ > > > > @@ -1771,5 +1831,6 @@ static inline void security_delete_hooks(struct security_hook_list *hooks, > > #endif /* CONFIG_SECURITY_WRITABLE_HOOKS */ > > > > extern int lsm_inode_alloc(struct inode *inode); > > +extern struct lsm_static_calls_table static_calls_table __ro_after_init; [...] > > > > +/* > > + * 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_ENABLED_KEY(NAME, NUM)); > > Hm, another place where we would benefit from having separated logic for > "is it built?" and "is it enabled by default?" and we could use > DEFINE_STATIC_KEY_MAYBE(). But, since we don't, I think we need to use > DEFINE_STATIC_KEY_TRUE() here or else won't all the calls be > out-of-line? (i.e. the default compiled state will be NOPs?) If we're > trying to optimize for having LSMs, I think we should default to inline > calls. (The machine code in the commit log seems to indicate that they > are out of line -- it uses jumps.) > I should have added it in the commit description, actually we are optimizing for "hot paths are less likely to have LSM hooks enabled" (eg. socket_sendmsg). But I do see that there are LSMs that have these enabled. Maybe we can put this behind a config option, possibly depending on CONFIG_EXPERT? > > [...] > > +#define __CALL_STATIC_VOID(NUM, HOOK, ...) \ > > + if (static_branch_unlikely(&SECURITY_HOOK_ENABLED_KEY(HOOK, NUM))) { \ > > + static_call(LSM_STATIC_CALL(HOOK, NUM))(__VA_ARGS__); \ > > + } > > Same here -- I would expect this to be static_branch_likely() or we'll > get out-of-line branches. Also, IMO, this should be: > > do { > if (...) > static_call(...); > } while (0) > Note we cannot really omit the semicolon here. We also use the UNROLL macro for declaring struct members which cannot assume that the MACRO to UNROLL will be terminated by a semicolon. > > > +#define call_void_hook(FUNC, ...) \ > > + do { \ > > + LSM_UNROLL(__CALL_STATIC_VOID, FUNC, __VA_ARGS__) \ > > } while (0) > > With the do/while in LSM_UNROLL, this is more readable. Agreed, done. > > > > > -#define call_int_hook(FUNC, IRC, ...) ({ \ > > - int RC = IRC; \ > > - do { \ > > - struct security_hook_list *P; \ > > - \ > > - hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \ > > - RC = P->hook.FUNC(__VA_ARGS__); \ > > - if (RC != 0) \ > > - break; \ > > - } \ > > - } while (0); \ > > - RC; \ > > -}) > > +#define __CALL_STATIC_INT(NUM, R, HOOK, ...) \ > > + if (static_branch_unlikely(&SECURITY_HOOK_ENABLED_KEY(HOOK, NUM))) { \ > > + R = static_call(LSM_STATIC_CALL(HOOK, NUM))(__VA_ARGS__); \ > > + if (R != 0) \ > > + goto out; \ > > + } > > I would expect the label to be a passed argument, but maybe since it > never changes, it's fine as-is? I think passing the label as an argument is cleaner, so I went ahead and did that. > > And again, I'd expect a do/while wrapper, and for it to be s_b_likely. The problem with the do/while wrapper here again is that UNROLL macros are terminated by semi-colons which does not work for unrolling struct member initialization, in particular for the static_calls_table where it's used to initialize the array. Now we could do what I suggested in LSM_LOOP_UNROLL and LSM_DEFINE_UNROLL and push the logic up to UNROLL into: * UNROLL_LOOP: Which expects a macro that will be surrounded by a do/while and terminated by a semicolon in the unroll body * UNROLL_DEFINE (or UNROLL_RAW) where you can pass anything. What do you think? > > > + > > +#define call_int_hook(FUNC, IRC, ...) \ > > + ({ \ > > + __label__ out; \ > > + int RC = IRC; \ > > + do { \ > > + LSM_UNROLL(__CALL_STATIC_INT, RC, FUNC, __VA_ARGS__) \ > > + \ > > + } while (0); \ > > Then this becomes: > > ({ > int RC = IRC; > LSM_UNROLL(__CALL_STATIC_INT, RC, FUNC, __VA_ARGS__); > out: > RC; > }) > > > +#define security_for_each_hook(scall, NAME, expression) \ > > + for (scall = static_calls_table.NAME; \ > > + scall - static_calls_table.NAME < MAX_LSM_COUNT; scall++) { \ > > + if (!static_key_enabled(scall->enabled_key)) \ > > + continue; \ > > + (expression); \ > > + } > > Why isn't this using static_branch_enabled()? I would expect this to be: I am guessing you mean static_branch_likely, I tried using static_branch_likely here and it does not work, the key is now being visited from a loop counter and the static_branch_likely needs it at compile time. So one needs to resort to the underlying static_key implementation. Doing this causes: ./arch/x86/include/asm/jump_label.h:27:20: error: invalid operand for inline asm constraint 'i' ./arch/x86/include/asm/jump_label.h:27:20: error: invalid operand for inline asm constraint 'i' The operand needs to be an immediate operand and needs patching at runtime. I think it's okay we are already not doing any optimization here as we still have the indirect call. TL; DR static_branch_likely cannot depend on runtime computations > > #define security_for_each_hook(scall, NAME) \ > for (scall = static_calls_table.NAME; \ > scall - static_calls_table.NAME < MAX_LSM_COUNT; scall++) \ > if (static_branch_likely(scall->enabled_key)) I like this macro, it does make the code easier to read thanks. > > > > > /* Security operations */ > > > > @@ -859,7 +924,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; > > > > @@ -870,13 +935,13 @@ 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); > > + security_for_each_hook(scall, vm_enough_memory, ({ > > + rc = scall->hl->hook.vm_enough_memory(mm, pages); > > if (rc <= 0) { > > cap_sys_admin = 0; > > break; > > } > > - } > > + })); > > Then these replacements don't look weird. This would just be: > > security_for_each_hook(scall, vm_enough_memory) { > rc = scall->hl->hook.vm_enough_memory(mm, pages); > if (rc <= 0) { > cap_sys_admin = 0; > break; > } > } Agreed, Thanks! > > I'm excited to have this. The speed improvements are pretty nice. Yay! > > -- > Kees Cook
On 2/6/2023 5:04 AM, KP Singh wrote: > On Fri, Jan 20, 2023 at 5:36 AM Kees Cook <keescook@chromium.org> wrote: >> On Fri, Jan 20, 2023 at 01:08:17AM +0100, KP Singh wrote: >>> The indirect calls are not really needed as one knows the addresses of > [...] > >>> +/* >>> + * 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_ENABLED_KEY(NAME, NUM)); >> Hm, another place where we would benefit from having separated logic for >> "is it built?" and "is it enabled by default?" and we could use >> DEFINE_STATIC_KEY_MAYBE(). But, since we don't, I think we need to use >> DEFINE_STATIC_KEY_TRUE() here or else won't all the calls be >> out-of-line? (i.e. the default compiled state will be NOPs?) If we're >> trying to optimize for having LSMs, I think we should default to inline >> calls. (The machine code in the commit log seems to indicate that they >> are out of line -- it uses jumps.) >> > I should have added it in the commit description, actually we are > optimizing for "hot paths are less likely to have LSM hooks enabled" > (eg. socket_sendmsg). How did you come to that conclusion? Where is there a correlation between "hot path" and "less likely to be enabled"? > But I do see that there are LSMs that have these > enabled. Maybe we can put this behind a config option, possibly > depending on CONFIG_EXPERT? Help me, as the maintainer of one of those LSMs, understand why that would be a good idea.
On Mon, Feb 6, 2023 at 8:29 AM Casey Schaufler <casey@schaufler-ca.com> wrote: > > On 2/6/2023 5:04 AM, KP Singh wrote: > > On Fri, Jan 20, 2023 at 5:36 AM Kees Cook <keescook@chromium.org> wrote: > >> On Fri, Jan 20, 2023 at 01:08:17AM +0100, KP Singh wrote: > >>> The indirect calls are not really needed as one knows the addresses of > > [...] > > > >>> +/* > >>> + * 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_ENABLED_KEY(NAME, NUM)); > >> Hm, another place where we would benefit from having separated logic for > >> "is it built?" and "is it enabled by default?" and we could use > >> DEFINE_STATIC_KEY_MAYBE(). But, since we don't, I think we need to use > >> DEFINE_STATIC_KEY_TRUE() here or else won't all the calls be > >> out-of-line? (i.e. the default compiled state will be NOPs?) If we're > >> trying to optimize for having LSMs, I think we should default to inline > >> calls. (The machine code in the commit log seems to indicate that they > >> are out of line -- it uses jumps.) > >> > > I should have added it in the commit description, actually we are > > optimizing for "hot paths are less likely to have LSM hooks enabled" > > (eg. socket_sendmsg). > > How did you come to that conclusion? Where is there a correlation between > "hot path" and "less likely to be enabled"? I could echo KP's reasoning here. AFAICT, the correlation is that LSMs on hot path will give more performance overhead. In our use cases (Meta), we are very careful with "small" performance hits. 0.25% is significant overhead; 1% overhead will not fly without very good reasons (Do we have to do this? Are there any other alternatives?). If it is possible to achieve similar security on a different hook, we will not enable the hook on the hot path. For example, we may not enable socket_sendmsg, but try to disallow opening such sockets instead. > > > But I do see that there are LSMs that have these > > enabled. Maybe we can put this behind a config option, possibly > > depending on CONFIG_EXPERT? > > Help me, as the maintainer of one of those LSMs, understand why that would > be a good idea. IIUC, this is also from performance concerns. We would like to manage the complexity at compile time for performance benefits. Thanks, Song
On Mon, Feb 6, 2023 at 6:49 PM Song Liu <song@kernel.org> wrote: > > On Mon, Feb 6, 2023 at 8:29 AM Casey Schaufler <casey@schaufler-ca.com> wrote: > > > > On 2/6/2023 5:04 AM, KP Singh wrote: > > > On Fri, Jan 20, 2023 at 5:36 AM Kees Cook <keescook@chromium.org> wrote: > > >> On Fri, Jan 20, 2023 at 01:08:17AM +0100, KP Singh wrote: > > >>> The indirect calls are not really needed as one knows the addresses of > > > [...] > > > > > >>> +/* > > >>> + * 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_ENABLED_KEY(NAME, NUM)); > > >> Hm, another place where we would benefit from having separated logic for > > >> "is it built?" and "is it enabled by default?" and we could use > > >> DEFINE_STATIC_KEY_MAYBE(). But, since we don't, I think we need to use > > >> DEFINE_STATIC_KEY_TRUE() here or else won't all the calls be > > >> out-of-line? (i.e. the default compiled state will be NOPs?) If we're > > >> trying to optimize for having LSMs, I think we should default to inline > > >> calls. (The machine code in the commit log seems to indicate that they > > >> are out of line -- it uses jumps.) > > >> > > > I should have added it in the commit description, actually we are > > > optimizing for "hot paths are less likely to have LSM hooks enabled" > > > (eg. socket_sendmsg). > > > > How did you come to that conclusion? Where is there a correlation between > > "hot path" and "less likely to be enabled"? > > I could echo KP's reasoning here. AFAICT, the correlation is that LSMs on > hot path will give more performance overhead. In our use cases (Meta), > we are very careful with "small" performance hits. 0.25% is significant +1 to everything Song said, I am not saying that one direction is better than the other and for distros that have LSMs (like SELinux and AppArmor enabled) it's okay to have this default to static_branch_likely. On systems that will have just the BPF LSM enabled, it's the opposite that is true, i.e. one would never add a hook on a hotpath as the overheads are unacceptable, and when one does add a hook, they are willing to add the extra overhead (this is already much less compared to the indirect calls). I am okay with the default being static_branch_likely if that's what the other LSM maintainers prefer. > overhead; 1% overhead will not fly without very good reasons (Do we > have to do this? Are there any other alternatives?). If it is possible to > achieve similar security on a different hook, we will not enable the hook on > the hot path. For example, we may not enable socket_sendmsg, but try > to disallow opening such sockets instead. > > > > > > But I do see that there are LSMs that have these > > > enabled. Maybe we can put this behind a config option, possibly > > > depending on CONFIG_EXPERT? > > > > Help me, as the maintainer of one of those LSMs, understand why that would > > be a good idea. > > IIUC, this is also from performance concerns. We would like to manage > the complexity at compile time for performance benefits. > > Thanks, > Song
On 2/6/2023 9:48 AM, Song Liu wrote: > On Mon, Feb 6, 2023 at 8:29 AM Casey Schaufler <casey@schaufler-ca.com> wrote: >> On 2/6/2023 5:04 AM, KP Singh wrote: >>> On Fri, Jan 20, 2023 at 5:36 AM Kees Cook <keescook@chromium.org> wrote: >>>> On Fri, Jan 20, 2023 at 01:08:17AM +0100, KP Singh wrote: >>>>> The indirect calls are not really needed as one knows the addresses of >>> [...] >>> >>>>> +/* >>>>> + * 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_ENABLED_KEY(NAME, NUM)); >>>> Hm, another place where we would benefit from having separated logic for >>>> "is it built?" and "is it enabled by default?" and we could use >>>> DEFINE_STATIC_KEY_MAYBE(). But, since we don't, I think we need to use >>>> DEFINE_STATIC_KEY_TRUE() here or else won't all the calls be >>>> out-of-line? (i.e. the default compiled state will be NOPs?) If we're >>>> trying to optimize for having LSMs, I think we should default to inline >>>> calls. (The machine code in the commit log seems to indicate that they >>>> are out of line -- it uses jumps.) >>>> >>> I should have added it in the commit description, actually we are >>> optimizing for "hot paths are less likely to have LSM hooks enabled" >>> (eg. socket_sendmsg). >> How did you come to that conclusion? Where is there a correlation between >> "hot path" and "less likely to be enabled"? > I could echo KP's reasoning here. AFAICT, the correlation is that LSMs on > hot path will give more performance overhead. In our use cases (Meta), > we are very careful with "small" performance hits. 0.25% is significant > overhead; 1% overhead will not fly without very good reasons (Do we > have to do this? Are there any other alternatives?). If it is possible to > achieve similar security on a different hook, we will not enable the hook on > the hot path. For example, we may not enable socket_sendmsg, but try > to disallow opening such sockets instead. I'm not asking about BPF. I'm asking about the impact on other LSMs. If you're talking strictly about BPF you need to say that. I'm all for performance improvement. But as I've said before, it should be for all the security modules, not just BPF. > >>> But I do see that there are LSMs that have these >>> enabled. Maybe we can put this behind a config option, possibly >>> depending on CONFIG_EXPERT? >> Help me, as the maintainer of one of those LSMs, understand why that would >> be a good idea. > IIUC, this is also from performance concerns. We would like to manage > the complexity at compile time for performance benefits. What complexity? What config option? I know that I'm slow, but it looks as if you're suggesting making the LSM infrastructure incredibly fragile and difficult to understand. > > Thanks, > Song
On Mon, Feb 6, 2023 at 7:29 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > > On 2/6/2023 9:48 AM, Song Liu wrote: > > On Mon, Feb 6, 2023 at 8:29 AM Casey Schaufler <casey@schaufler-ca.com> wrote: > >> On 2/6/2023 5:04 AM, KP Singh wrote: > >>> On Fri, Jan 20, 2023 at 5:36 AM Kees Cook <keescook@chromium.org> wrote: > >>>> On Fri, Jan 20, 2023 at 01:08:17AM +0100, KP Singh wrote: > >>>>> The indirect calls are not really needed as one knows the addresses of > >>> [...] > >>> > >>>>> +/* > >>>>> + * 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_ENABLED_KEY(NAME, NUM)); > >>>> Hm, another place where we would benefit from having separated logic for > >>>> "is it built?" and "is it enabled by default?" and we could use > >>>> DEFINE_STATIC_KEY_MAYBE(). But, since we don't, I think we need to use > >>>> DEFINE_STATIC_KEY_TRUE() here or else won't all the calls be > >>>> out-of-line? (i.e. the default compiled state will be NOPs?) If we're > >>>> trying to optimize for having LSMs, I think we should default to inline > >>>> calls. (The machine code in the commit log seems to indicate that they > >>>> are out of line -- it uses jumps.) > >>>> > >>> I should have added it in the commit description, actually we are > >>> optimizing for "hot paths are less likely to have LSM hooks enabled" > >>> (eg. socket_sendmsg). > >> How did you come to that conclusion? Where is there a correlation between > >> "hot path" and "less likely to be enabled"? > > I could echo KP's reasoning here. AFAICT, the correlation is that LSMs on > > hot path will give more performance overhead. In our use cases (Meta), > > we are very careful with "small" performance hits. 0.25% is significant > > overhead; 1% overhead will not fly without very good reasons (Do we > > have to do this? Are there any other alternatives?). If it is possible to > > achieve similar security on a different hook, we will not enable the hook on > > the hot path. For example, we may not enable socket_sendmsg, but try > > to disallow opening such sockets instead. > > I'm not asking about BPF. I'm asking about the impact on other LSMs. > If you're talking strictly about BPF you need to say that. I'm all for > performance improvement. But as I've said before, it should be for all > the security modules, not just BPF. It's a trade off that will work differently for different LSMs and distros (based on the LSM they chose) and this the config option. I even suggested this be behind CONFIG_EXPERT (which is basically says this: "This option allows certain base kernel options and settings to be disabled or tweaked. This is for specialized environments which can tolerate a "non-standard" kernel. Only use this if you really know what you are doing." > > > > >>> But I do see that there are LSMs that have these > >>> enabled. Maybe we can put this behind a config option, possibly > >>> depending on CONFIG_EXPERT? > >> Help me, as the maintainer of one of those LSMs, understand why that would > >> be a good idea. > > IIUC, this is also from performance concerns. We would like to manage > > the complexity at compile time for performance benefits. > > What complexity? What config option? I know that I'm slow, but it looks > as if you're suggesting making the LSM infrastructure incredibly fragile > and difficult to understand. I am sorry but the LSM is a core piece of the kernel that currently has significant unnecessary overheads (look at the numbers that I posted) and this not making it fragile, it's making it performant, such optimisations are everywhere in the kernel and the LSM infrastructure has somehow been neglected and is just catching up. These are resources being wasted which could be saved. > > > > > Thanks, > > Song
On Mon, Feb 06, 2023 at 07:41:04PM +0100, KP Singh wrote: > On Mon, Feb 6, 2023 at 7:29 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > > > > On 2/6/2023 9:48 AM, Song Liu wrote: > > > On Mon, Feb 6, 2023 at 8:29 AM Casey Schaufler <casey@schaufler-ca.com> wrote: > > >> On 2/6/2023 5:04 AM, KP Singh wrote: > > >>> On Fri, Jan 20, 2023 at 5:36 AM Kees Cook <keescook@chromium.org> wrote: > > >>>> On Fri, Jan 20, 2023 at 01:08:17AM +0100, KP Singh wrote: > > >>>>> The indirect calls are not really needed as one knows the addresses of > > >>> [...] > > >>> > > >>>>> +/* > > >>>>> + * 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_ENABLED_KEY(NAME, NUM)); > > >>>> Hm, another place where we would benefit from having separated logic for > > >>>> "is it built?" and "is it enabled by default?" and we could use > > >>>> DEFINE_STATIC_KEY_MAYBE(). But, since we don't, I think we need to use > > >>>> DEFINE_STATIC_KEY_TRUE() here or else won't all the calls be > > >>>> out-of-line? (i.e. the default compiled state will be NOPs?) If we're > > >>>> trying to optimize for having LSMs, I think we should default to inline > > >>>> calls. (The machine code in the commit log seems to indicate that they > > >>>> are out of line -- it uses jumps.) > > >>>> > > >>> I should have added it in the commit description, actually we are > > >>> optimizing for "hot paths are less likely to have LSM hooks enabled" > > >>> (eg. socket_sendmsg). > > >> How did you come to that conclusion? Where is there a correlation between > > >> "hot path" and "less likely to be enabled"? > > > I could echo KP's reasoning here. AFAICT, the correlation is that LSMs on > > > hot path will give more performance overhead. In our use cases (Meta), > > > we are very careful with "small" performance hits. 0.25% is significant > > > overhead; 1% overhead will not fly without very good reasons (Do we > > > have to do this? Are there any other alternatives?). If it is possible to > > > achieve similar security on a different hook, we will not enable the hook on > > > the hot path. For example, we may not enable socket_sendmsg, but try > > > to disallow opening such sockets instead. > > > > I'm not asking about BPF. I'm asking about the impact on other LSMs. > > If you're talking strictly about BPF you need to say that. I'm all for > > performance improvement. But as I've said before, it should be for all > > the security modules, not just BPF. > > It's a trade off that will work differently for different LSMs and > distros (based on the LSM they chose) and this the config option. I > even suggested this be behind CONFIG_EXPERT (which is basically says > this: > > "This option allows certain base kernel options and settings > to be disabled or tweaked. This is for specialized > environments which can tolerate a "non-standard" kernel. > Only use this if you really know what you are doing." Using the DEFINE_STATIC_KEY_MAYBE() and static_branch_maybe() macros tied to a new CONFIG seems like it can give us a reasonable knob for in-line vs out-of-line calls. > > >>> But I do see that there are LSMs that have these > > >>> enabled. Maybe we can put this behind a config option, possibly > > >>> depending on CONFIG_EXPERT? > > >> Help me, as the maintainer of one of those LSMs, understand why that would > > >> be a good idea. > > > IIUC, this is also from performance concerns. We would like to manage > > > the complexity at compile time for performance benefits. > > > > What complexity? What config option? I know that I'm slow, but it looks > > as if you're suggesting making the LSM infrastructure incredibly fragile > > and difficult to understand. > > I am sorry but the LSM is a core piece of the kernel that currently > has significant unnecessary overheads (look at the numbers that I > posted) and this not making it fragile, it's making it performant, > such optimisations are everywhere in the kernel and the LSM > infrastructure has somehow been neglected and is just catching up. > These are resources being wasted which could be saved. Let's just move forward to v2, which I think will look much cleaner. I think we can get to both maintainable code and run-time performance with this series.
On Mon, Feb 6, 2023 at 10:29 AM Casey Schaufler <casey@schaufler-ca.com> wrote: > [...] > >>> I should have added it in the commit description, actually we are > >>> optimizing for "hot paths are less likely to have LSM hooks enabled" > >>> (eg. socket_sendmsg). > >> How did you come to that conclusion? Where is there a correlation between > >> "hot path" and "less likely to be enabled"? > > I could echo KP's reasoning here. AFAICT, the correlation is that LSMs on > > hot path will give more performance overhead. In our use cases (Meta), > > we are very careful with "small" performance hits. 0.25% is significant > > overhead; 1% overhead will not fly without very good reasons (Do we > > have to do this? Are there any other alternatives?). If it is possible to > > achieve similar security on a different hook, we will not enable the hook on > > the hot path. For example, we may not enable socket_sendmsg, but try > > to disallow opening such sockets instead. > > I'm not asking about BPF. I'm asking about the impact on other LSMs. > If you're talking strictly about BPF you need to say that. I'm all for > performance improvement. But as I've said before, it should be for all > the security modules, not just BPF. I don't think anything here is BPF specific. Performance-security tradeoff should be the same for all LSMs. A hook on the hot path is more expensive than a hook on a cooler path. This is the same for all LSMs, no? Thanks, Song
On 2/6/2023 11:05 AM, Song Liu wrote: > On Mon, Feb 6, 2023 at 10:29 AM Casey Schaufler <casey@schaufler-ca.com> wrote: > [...] >>>>> I should have added it in the commit description, actually we are >>>>> optimizing for "hot paths are less likely to have LSM hooks enabled" >>>>> (eg. socket_sendmsg). >>>> How did you come to that conclusion? Where is there a correlation between >>>> "hot path" and "less likely to be enabled"? >>> I could echo KP's reasoning here. AFAICT, the correlation is that LSMs on >>> hot path will give more performance overhead. In our use cases (Meta), >>> we are very careful with "small" performance hits. 0.25% is significant >>> overhead; 1% overhead will not fly without very good reasons (Do we >>> have to do this? Are there any other alternatives?). If it is possible to >>> achieve similar security on a different hook, we will not enable the hook on >>> the hot path. For example, we may not enable socket_sendmsg, but try >>> to disallow opening such sockets instead. >> I'm not asking about BPF. I'm asking about the impact on other LSMs. >> If you're talking strictly about BPF you need to say that. I'm all for >> performance improvement. But as I've said before, it should be for all >> the security modules, not just BPF. > I don't think anything here is BPF specific. Performance-security tradeoff > should be the same for all LSMs. A hook on the hot path is more expensive > than a hook on a cooler path. This is the same for all LSMs, no? Yes. Alas, for some security schemes it isn't possible to avoid checks in hot paths. The assumption that "hot paths are less likely to have LSM hooks enabled" is not generally valid. The statement is question seems to imply that it's OK to not optimize hot paths. Maybe I read it wrong. I will hold off further comment until we see the next version. > > Thanks, > Song
On Mon, Feb 6, 2023 at 7:51 PM Kees Cook <keescook@chromium.org> wrote: > > On Mon, Feb 06, 2023 at 07:41:04PM +0100, KP Singh wrote: > > On Mon, Feb 6, 2023 at 7:29 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > > > > > > On 2/6/2023 9:48 AM, Song Liu wrote: > > > > On Mon, Feb 6, 2023 at 8:29 AM Casey Schaufler <casey@schaufler-ca.com> wrote: > > > >> On 2/6/2023 5:04 AM, KP Singh wrote: > > > >>> On Fri, Jan 20, 2023 at 5:36 AM Kees Cook <keescook@chromium.org> wrote: > > > >>>> On Fri, Jan 20, 2023 at 01:08:17AM +0100, KP Singh wrote: > > > >>>>> The indirect calls are not really needed as one knows the addresses of > > > >>> [...] > > > >>> > > > >>>>> +/* > > > >>>>> + * 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_ENABLED_KEY(NAME, NUM)); > > > >>>> Hm, another place where we would benefit from having separated logic for > > > >>>> "is it built?" and "is it enabled by default?" and we could use > > > >>>> DEFINE_STATIC_KEY_MAYBE(). But, since we don't, I think we need to use > > > >>>> DEFINE_STATIC_KEY_TRUE() here or else won't all the calls be > > > >>>> out-of-line? (i.e. the default compiled state will be NOPs?) If we're > > > >>>> trying to optimize for having LSMs, I think we should default to inline > > > >>>> calls. (The machine code in the commit log seems to indicate that they > > > >>>> are out of line -- it uses jumps.) > > > >>>> > > > >>> I should have added it in the commit description, actually we are > > > >>> optimizing for "hot paths are less likely to have LSM hooks enabled" > > > >>> (eg. socket_sendmsg). > > > >> How did you come to that conclusion? Where is there a correlation between > > > >> "hot path" and "less likely to be enabled"? > > > > I could echo KP's reasoning here. AFAICT, the correlation is that LSMs on > > > > hot path will give more performance overhead. In our use cases (Meta), > > > > we are very careful with "small" performance hits. 0.25% is significant > > > > overhead; 1% overhead will not fly without very good reasons (Do we > > > > have to do this? Are there any other alternatives?). If it is possible to > > > > achieve similar security on a different hook, we will not enable the hook on > > > > the hot path. For example, we may not enable socket_sendmsg, but try > > > > to disallow opening such sockets instead. > > > > > > I'm not asking about BPF. I'm asking about the impact on other LSMs. > > > If you're talking strictly about BPF you need to say that. I'm all for > > > performance improvement. But as I've said before, it should be for all > > > the security modules, not just BPF. > > > > It's a trade off that will work differently for different LSMs and > > distros (based on the LSM they chose) and this the config option. I > > even suggested this be behind CONFIG_EXPERT (which is basically says > > this: > > > > "This option allows certain base kernel options and settings > > to be disabled or tweaked. This is for specialized > > environments which can tolerate a "non-standard" kernel. > > Only use this if you really know what you are doing." > > Using the DEFINE_STATIC_KEY_MAYBE() and static_branch_maybe() macros > tied to a new CONFIG seems like it can give us a reasonable knob for > in-line vs out-of-line calls. Coming back to this after a while as I finally got time to work on this. (work/personal downtime). I am changing it to DEFINE_STATIC_KEY_TRUE in this patch and DEFINE_STATIC_KEY_MAYBE in a subsequent one and guarded by a config called CONFIG_SECURITY_HOOK_LIKELY. I am letting it default to yes, but distros can change it depending on their choice of LSM and performance characteristics. > > > > >>> But I do see that there are LSMs that have these > > > >>> enabled. Maybe we can put this behind a config option, possibly > > > >>> depending on CONFIG_EXPERT? > > > >> Help me, as the maintainer of one of those LSMs, understand why that would > > > >> be a good idea. > > > > IIUC, this is also from performance concerns. We would like to manage > > > > the complexity at compile time for performance benefits. > > > > > > What complexity? What config option? I know that I'm slow, but it looks > > > as if you're suggesting making the LSM infrastructure incredibly fragile > > > and difficult to understand. > > > > I am sorry but the LSM is a core piece of the kernel that currently > > has significant unnecessary overheads (look at the numbers that I > > posted) and this not making it fragile, it's making it performant, > > such optimisations are everywhere in the kernel and the LSM > > infrastructure has somehow been neglected and is just catching up. > > These are resources being wasted which could be saved. > > Let's just move forward to v2, which I think will look much cleaner. I > think we can get to both maintainable code and run-time performance with > this series. > > -- > Kees Cook
On Wed, Jun 7, 2023 at 10:48 PM KP Singh <kpsingh@kernel.org> wrote: > On Mon, Feb 6, 2023 at 7:51 PM Kees Cook <keescook@chromium.org> wrote: > > On Mon, Feb 06, 2023 at 07:41:04PM +0100, KP Singh wrote: > > > On Mon, Feb 6, 2023 at 7:29 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > > > > > > > > On 2/6/2023 9:48 AM, Song Liu wrote: > > > > > On Mon, Feb 6, 2023 at 8:29 AM Casey Schaufler <casey@schaufler-ca.com> wrote: > > > > >> On 2/6/2023 5:04 AM, KP Singh wrote: > > > > >>> On Fri, Jan 20, 2023 at 5:36 AM Kees Cook <keescook@chromium.org> wrote: > > > > >>>> On Fri, Jan 20, 2023 at 01:08:17AM +0100, KP Singh wrote: > > > > >>>>> The indirect calls are not really needed as one knows the addresses of > > > > >>> [...] > > > > >>> > > > > >>>>> +/* > > > > >>>>> + * 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_ENABLED_KEY(NAME, NUM)); > > > > >>>> Hm, another place where we would benefit from having separated logic for > > > > >>>> "is it built?" and "is it enabled by default?" and we could use > > > > >>>> DEFINE_STATIC_KEY_MAYBE(). But, since we don't, I think we need to use > > > > >>>> DEFINE_STATIC_KEY_TRUE() here or else won't all the calls be > > > > >>>> out-of-line? (i.e. the default compiled state will be NOPs?) If we're > > > > >>>> trying to optimize for having LSMs, I think we should default to inline > > > > >>>> calls. (The machine code in the commit log seems to indicate that they > > > > >>>> are out of line -- it uses jumps.) > > > > >>>> > > > > >>> I should have added it in the commit description, actually we are > > > > >>> optimizing for "hot paths are less likely to have LSM hooks enabled" > > > > >>> (eg. socket_sendmsg). > > > > >> How did you come to that conclusion? Where is there a correlation between > > > > >> "hot path" and "less likely to be enabled"? > > > > > I could echo KP's reasoning here. AFAICT, the correlation is that LSMs on > > > > > hot path will give more performance overhead. In our use cases (Meta), > > > > > we are very careful with "small" performance hits. 0.25% is significant > > > > > overhead; 1% overhead will not fly without very good reasons (Do we > > > > > have to do this? Are there any other alternatives?). If it is possible to > > > > > achieve similar security on a different hook, we will not enable the hook on > > > > > the hot path. For example, we may not enable socket_sendmsg, but try > > > > > to disallow opening such sockets instead. > > > > > > > > I'm not asking about BPF. I'm asking about the impact on other LSMs. > > > > If you're talking strictly about BPF you need to say that. I'm all for > > > > performance improvement. But as I've said before, it should be for all > > > > the security modules, not just BPF. > > > > > > It's a trade off that will work differently for different LSMs and > > > distros (based on the LSM they chose) and this the config option. I > > > even suggested this be behind CONFIG_EXPERT (which is basically says > > > this: > > > > > > "This option allows certain base kernel options and settings > > > to be disabled or tweaked. This is for specialized > > > environments which can tolerate a "non-standard" kernel. > > > Only use this if you really know what you are doing." > > > > Using the DEFINE_STATIC_KEY_MAYBE() and static_branch_maybe() macros > > tied to a new CONFIG seems like it can give us a reasonable knob for > > in-line vs out-of-line calls. > > Coming back to this after a while as I finally got time to work on > this. (work/personal downtime). > > I am changing it to DEFINE_STATIC_KEY_TRUE in this patch and > DEFINE_STATIC_KEY_MAYBE in a subsequent one and guarded by a config > called CONFIG_SECURITY_HOOK_LIKELY. I am letting it default to yes, > but distros can change it depending on their choice of LSM and > performance characteristics. I'm still more curious about the correctness/isolation aspect that I mused about back in Jan/Feb on your original posting.
On Tue, Jun 13, 2023 at 11:43 PM Paul Moore <paul@paul-moore.com> wrote: > > On Wed, Jun 7, 2023 at 10:48 PM KP Singh <kpsingh@kernel.org> wrote: > > On Mon, Feb 6, 2023 at 7:51 PM Kees Cook <keescook@chromium.org> wrote: > > > On Mon, Feb 06, 2023 at 07:41:04PM +0100, KP Singh wrote: > > > > On Mon, Feb 6, 2023 at 7:29 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > > > > > > > > > > On 2/6/2023 9:48 AM, Song Liu wrote: > > > > > > On Mon, Feb 6, 2023 at 8:29 AM Casey Schaufler <casey@schaufler-ca.com> wrote: > > > > > >> On 2/6/2023 5:04 AM, KP Singh wrote: > > > > > >>> On Fri, Jan 20, 2023 at 5:36 AM Kees Cook <keescook@chromium.org> wrote: > > > > > >>>> On Fri, Jan 20, 2023 at 01:08:17AM +0100, KP Singh wrote: > > > > > >>>>> The indirect calls are not really needed as one knows the addresses of > > > > > >>> [...] > > > > > >>> > > > > > >>>>> +/* > > > > > >>>>> + * 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_ENABLED_KEY(NAME, NUM)); > > > > > >>>> Hm, another place where we would benefit from having separated logic for > > > > > >>>> "is it built?" and "is it enabled by default?" and we could use > > > > > >>>> DEFINE_STATIC_KEY_MAYBE(). But, since we don't, I think we need to use > > > > > >>>> DEFINE_STATIC_KEY_TRUE() here or else won't all the calls be > > > > > >>>> out-of-line? (i.e. the default compiled state will be NOPs?) If we're > > > > > >>>> trying to optimize for having LSMs, I think we should default to inline > > > > > >>>> calls. (The machine code in the commit log seems to indicate that they > > > > > >>>> are out of line -- it uses jumps.) > > > > > >>>> > > > > > >>> I should have added it in the commit description, actually we are > > > > > >>> optimizing for "hot paths are less likely to have LSM hooks enabled" > > > > > >>> (eg. socket_sendmsg). > > > > > >> How did you come to that conclusion? Where is there a correlation between > > > > > >> "hot path" and "less likely to be enabled"? > > > > > > I could echo KP's reasoning here. AFAICT, the correlation is that LSMs on > > > > > > hot path will give more performance overhead. In our use cases (Meta), > > > > > > we are very careful with "small" performance hits. 0.25% is significant > > > > > > overhead; 1% overhead will not fly without very good reasons (Do we > > > > > > have to do this? Are there any other alternatives?). If it is possible to > > > > > > achieve similar security on a different hook, we will not enable the hook on > > > > > > the hot path. For example, we may not enable socket_sendmsg, but try > > > > > > to disallow opening such sockets instead. > > > > > > > > > > I'm not asking about BPF. I'm asking about the impact on other LSMs. > > > > > If you're talking strictly about BPF you need to say that. I'm all for > > > > > performance improvement. But as I've said before, it should be for all > > > > > the security modules, not just BPF. > > > > > > > > It's a trade off that will work differently for different LSMs and > > > > distros (based on the LSM they chose) and this the config option. I > > > > even suggested this be behind CONFIG_EXPERT (which is basically says > > > > this: > > > > > > > > "This option allows certain base kernel options and settings > > > > to be disabled or tweaked. This is for specialized > > > > environments which can tolerate a "non-standard" kernel. > > > > Only use this if you really know what you are doing." > > > > > > Using the DEFINE_STATIC_KEY_MAYBE() and static_branch_maybe() macros > > > tied to a new CONFIG seems like it can give us a reasonable knob for > > > in-line vs out-of-line calls. > > > > Coming back to this after a while as I finally got time to work on > > this. (work/personal downtime). > > > > I am changing it to DEFINE_STATIC_KEY_TRUE in this patch and > > DEFINE_STATIC_KEY_MAYBE in a subsequent one and guarded by a config > > called CONFIG_SECURITY_HOOK_LIKELY. I am letting it default to yes, > > but distros can change it depending on their choice of LSM and > > performance characteristics. > > I'm still more curious about the correctness/isolation aspect that I > mused about back in Jan/Feb on your original posting. Thanks, I put some clarifications there. I will post a v2 soon (TM). Although beware I have upcoming downtime due to a surgery next week. > > -- > paul-moore.com
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 0a5ba81f7367..c82d15a4ef50 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -28,6 +28,26 @@ #include <linux/security.h> #include <linux/init.h> #include <linux/rculist.h> +#include <linux/static_call.h> +#include <linux/unroll.h> +#include <linux/jump_label.h> + +/* Include the generated MAX_LSM_COUNT */ +#include <generated/lsm_count.h> + +#define SECURITY_HOOK_ENABLED_KEY(HOOK, IDX) security_enabled_key_##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_UNROLL(M, ...) UNROLL(MAX_LSM_COUNT, M, __VA_ARGS__) /** * union security_list_options - Linux Security Module hook function list @@ -1657,21 +1677,48 @@ union security_list_options { #define LSM_HOOK(RET, DEFAULT, NAME, ...) RET (*NAME)(__VA_ARGS__); #include "lsm_hook_defs.h" #undef LSM_HOOK + void *lsm_callback; }; -struct security_hook_heads { - #define LSM_HOOK(RET, DEFAULT, NAME, ...) struct hlist_head NAME; - #include "lsm_hook_defs.h" +/* + * @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. + * @enabled_key: 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; + struct static_key *enabled_key; +}; + +/* + * 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 } __randomize_layout; /* * 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 char *lsm; } __randomize_layout; @@ -1701,10 +1748,12 @@ struct lsm_blob_sizes { * 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, CALLBACK) \ + { \ + .scalls = static_calls_table.NAME, \ + .hook = { .NAME = CALLBACK } \ + } -extern struct security_hook_heads security_hook_heads; extern char *lsm_names; extern void security_add_hooks(struct security_hook_list *hooks, int count, @@ -1756,10 +1805,21 @@ extern struct lsm_info __start_early_lsm_info[], __end_early_lsm_info[]; static inline void security_delete_hooks(struct security_hook_list *hooks, int count) { - int i; + struct lsm_static_call *scalls; + int i, j; + + for (i = 0; i < count; i++) { + scalls = hooks[i].scalls; + for (j = 0; j < MAX_LSM_COUNT; j++) { + if (scalls[j].hl != &hooks[i]) + continue; - for (i = 0; i < count; i++) - hlist_del_rcu(&hooks[i].list); + static_key_disable(scalls[j].enabled_key); + __static_call_update(scalls[j].key, + scalls[j].trampoline, NULL); + scalls[j].hl = NULL; + } + } } #endif /* CONFIG_SECURITY_SELINUX_DISABLE */ @@ -1771,5 +1831,6 @@ static inline void security_delete_hooks(struct security_hook_list *hooks, #endif /* CONFIG_SECURITY_WRITABLE_HOOKS */ 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 d1571900a8c7..e54d5ba187d1 100644 --- a/security/security.c +++ b/security/security.c @@ -29,6 +29,8 @@ #include <linux/string.h> #include <linux/msg.h> #include <net/flow.h> +#include <linux/static_call.h> +#include <linux/jump_label.h> #define MAX_LSM_EVM_XATTR 2 @@ -74,7 +76,6 @@ const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = { [LOCKDOWN_CONFIDENTIALITY_MAX] = "confidentiality", }; -struct security_hook_heads security_hook_heads __lsm_ro_after_init; static BLOCKING_NOTIFIER_HEAD(blocking_lsm_notifier_chain); static struct kmem_cache *lsm_file_cache; @@ -93,6 +94,43 @@ static __initconst const char * const builtin_lsm_order = CONFIG_LSM; static __initdata struct lsm_info **ordered_lsms; static __initdata struct lsm_info *exclusive; +/* + * 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_ENABLED_KEY(NAME, NUM)); + +#define LSM_HOOK(RET, DEFAULT, NAME, ...) \ + LSM_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 __lsm_ro_after_init = { +#define INIT_LSM_STATIC_CALL(NUM, NAME) \ + (struct lsm_static_call) { \ + .key = &STATIC_CALL_KEY(LSM_STATIC_CALL(NAME, NUM)), \ + .trampoline = &STATIC_CALL_TRAMP(LSM_STATIC_CALL(NAME, NUM)),\ + .enabled_key = &SECURITY_HOOK_ENABLED_KEY(NAME, NUM).key,\ + }, +#define LSM_HOOK(RET, DEFAULT, NAME, ...) \ + .NAME = { \ + LSM_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 { \ @@ -153,7 +191,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. */ @@ -318,6 +356,24 @@ 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_callback); + scall->hl = hl; + static_key_enable(scall->enabled_key); + return; + } + scall++; + } + panic("%s - No static call remaining to add LSM hook.\n", __func__); +} + static void __init lsm_early_cred(struct cred *cred); static void __init lsm_early_task(struct task_struct *task); @@ -395,11 +451,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; @@ -515,7 +566,7 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count, for (i = 0; i < count; i++) { hooks[i].lsm = lsm; - hlist_add_tail_rcu(&hooks[i].list, hooks[i].head); + lsm_static_call_init(&hooks[i]); } /* @@ -753,28 +804,42 @@ static int lsm_superblock_alloc(struct super_block *sb) * call_int_hook: * This is a hook that returns a value. */ +#define __CALL_STATIC_VOID(NUM, HOOK, ...) \ + if (static_branch_unlikely(&SECURITY_HOOK_ENABLED_KEY(HOOK, NUM))) { \ + static_call(LSM_STATIC_CALL(HOOK, NUM))(__VA_ARGS__); \ + } -#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(FUNC, ...) \ + do { \ + LSM_UNROLL(__CALL_STATIC_VOID, FUNC, __VA_ARGS__) \ } while (0) -#define call_int_hook(FUNC, IRC, ...) ({ \ - int RC = IRC; \ - do { \ - struct security_hook_list *P; \ - \ - hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \ - RC = P->hook.FUNC(__VA_ARGS__); \ - if (RC != 0) \ - break; \ - } \ - } while (0); \ - RC; \ -}) +#define __CALL_STATIC_INT(NUM, R, HOOK, ...) \ + if (static_branch_unlikely(&SECURITY_HOOK_ENABLED_KEY(HOOK, NUM))) { \ + R = static_call(LSM_STATIC_CALL(HOOK, NUM))(__VA_ARGS__); \ + if (R != 0) \ + goto out; \ + } + +#define call_int_hook(FUNC, IRC, ...) \ + ({ \ + __label__ out; \ + int RC = IRC; \ + do { \ + LSM_UNROLL(__CALL_STATIC_INT, RC, FUNC, __VA_ARGS__) \ + \ + } while (0); \ +out: \ + RC; \ + }) + +#define security_for_each_hook(scall, NAME, expression) \ + for (scall = static_calls_table.NAME; \ + scall - static_calls_table.NAME < MAX_LSM_COUNT; scall++) { \ + if (!static_key_enabled(scall->enabled_key)) \ + continue; \ + (expression); \ + } /* Security operations */ @@ -859,7 +924,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; @@ -870,13 +935,13 @@ 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); + security_for_each_hook(scall, vm_enough_memory, ({ + rc = scall->hl->hook.vm_enough_memory(mm, pages); if (rc <= 0) { cap_sys_admin = 0; break; } - } + })); return __vm_enough_memory(mm, pages, cap_sys_admin); } @@ -918,18 +983,17 @@ 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); + security_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) return trc; - } + })); return rc; } @@ -1092,18 +1156,19 @@ int security_dentry_init_security(struct dentry *dentry, int mode, const char **xattr_name, void **ctx, u32 *ctxlen) { - struct security_hook_list *hp; + struct lsm_static_call *scall; int rc; /* * Only one module will provide a security context. */ - hlist_for_each_entry(hp, &security_hook_heads.dentry_init_security, list) { - rc = hp->hook.dentry_init_security(dentry, mode, name, + security_for_each_hook(scall, dentry_init_security, ({ + rc = scall->hl->hook.dentry_init_security(dentry, mode, name, xattr_name, ctx, ctxlen); if (rc != LSM_RET_DEFAULT(dentry_init_security)) return rc; - } + })); + return LSM_RET_DEFAULT(dentry_init_security); } EXPORT_SYMBOL(security_dentry_init_security); @@ -1502,7 +1567,7 @@ int security_inode_getsecurity(struct user_namespace *mnt_userns, struct inode *inode, const char *name, void **buffer, bool alloc) { - struct security_hook_list *hp; + struct lsm_static_call *scall; int rc; if (unlikely(IS_PRIVATE(inode))) @@ -1510,17 +1575,17 @@ int security_inode_getsecurity(struct user_namespace *mnt_userns, /* * Only one module will provide an attribute with a given name. */ - hlist_for_each_entry(hp, &security_hook_heads.inode_getsecurity, list) { - rc = hp->hook.inode_getsecurity(mnt_userns, inode, name, buffer, alloc); + security_for_each_hook(scall, inode_getsecurity, ({ + rc = scall->hl->hook.inode_getsecurity(mnt_userns, inode, name, buffer, alloc); if (rc != LSM_RET_DEFAULT(inode_getsecurity)) return rc; - } + })); return LSM_RET_DEFAULT(inode_getsecurity); } int security_inode_setsecurity(struct inode *inode, const char *name, const void *value, size_t size, int flags) { - struct security_hook_list *hp; + struct lsm_static_call *scall; int rc; if (unlikely(IS_PRIVATE(inode))) @@ -1528,12 +1593,11 @@ int security_inode_setsecurity(struct inode *inode, const char *name, const void /* * Only one module will provide an attribute with a given name. */ - hlist_for_each_entry(hp, &security_hook_heads.inode_setsecurity, list) { - rc = hp->hook.inode_setsecurity(inode, name, value, size, - flags); + security_for_each_hook(scall, inode_setsecurity, ({ + rc = scall->hl->hook.inode_setsecurity(inode, name, value, size, flags); if (rc != LSM_RET_DEFAULT(inode_setsecurity)) return rc; - } + })); return LSM_RET_DEFAULT(inode_setsecurity); } @@ -1558,7 +1622,7 @@ EXPORT_SYMBOL(security_inode_copy_up); int security_inode_copy_up_xattr(const char *name) { - struct security_hook_list *hp; + struct lsm_static_call *scall; int rc; /* @@ -1566,12 +1630,11 @@ int security_inode_copy_up_xattr(const char *name) * xattr), -EOPNOTSUPP if it does not know anything about the xattr or * any other error code incase of an error. */ - hlist_for_each_entry(hp, - &security_hook_heads.inode_copy_up_xattr, list) { - rc = hp->hook.inode_copy_up_xattr(name); + security_for_each_hook(scall, inode_copy_up_xattr, ({ + rc = scall->hl->hook.inode_copy_up_xattr(name); if (rc != LSM_RET_DEFAULT(inode_copy_up_xattr)) return rc; - } + })); return LSM_RET_DEFAULT(inode_copy_up_xattr); } @@ -1968,16 +2031,16 @@ 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); + security_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) break; } - } + })); return rc; } @@ -2142,26 +2205,26 @@ EXPORT_SYMBOL(security_d_instantiate); int security_getprocattr(struct task_struct *p, const char *lsm, 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 (lsm != NULL && strcmp(lsm, hp->lsm)) + security_for_each_hook(scall, getprocattr, ({ + if (lsm != NULL && strcmp(lsm, scall->hl->lsm)) continue; - return hp->hook.getprocattr(p, name, value); - } + return scall->hl->hook.getprocattr(p, name, value); + })); return LSM_RET_DEFAULT(getprocattr); } int security_setprocattr(const char *lsm, 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 (lsm != NULL && strcmp(lsm, hp->lsm)) + security_for_each_hook(scall, setprocattr, ({ + if (lsm != NULL && strcmp(lsm, scall->hl->lsm)) continue; - return hp->hook.setprocattr(name, value, size); - } + return scall->hl->hook.setprocattr(name, value, size); + })); return LSM_RET_DEFAULT(setprocattr); } @@ -2178,18 +2241,18 @@ EXPORT_SYMBOL(security_ismaclabel); int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen) { - struct security_hook_list *hp; + struct lsm_static_call *scall; int rc; /* * Currently, only one LSM can implement secid_to_secctx (i.e this * LSM hook is not "stackable"). */ - hlist_for_each_entry(hp, &security_hook_heads.secid_to_secctx, list) { - rc = hp->hook.secid_to_secctx(secid, secdata, seclen); + security_for_each_hook(scall, secid_to_secctx, ({ + rc = scall->hl->hook.secid_to_secctx(secid, secdata, seclen); if (rc != LSM_RET_DEFAULT(secid_to_secctx)) return rc; - } + })); return LSM_RET_DEFAULT(secid_to_secctx); } @@ -2582,7 +2645,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); /* @@ -2594,11 +2657,10 @@ 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); + security_for_each_hook(scall, xfrm_state_pol_flow_match, ({ + rc = scall->hl->hook.xfrm_state_pol_flow_match(x, xp, flic); break; - } + })); return rc; }