Re: [PATCH RESEND bpf-next 3/4] security: Replace indirect LSM hook calls with static calls

[Kees Cook <keescook@chromium.org>]

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.

-- 
Kees Cook