Re: [PATCH v2 5/5] security: Add CONFIG_SECURITY_HOOK_LIKELY

[Kees Cook <keescook@chromium.org>]

On Fri, Jun 16, 2023 at 02:04:41AM +0200, KP Singh wrote:
> [...]
> @@ -110,6 +110,9 @@ static __initdata struct lsm_info *exclusive;
>  #undef LSM_HOOK
>  #undef DEFINE_LSM_STATIC_CALL
>  
> +#define security_hook_active(n, h) \
> +	static_branch_maybe(CONFIG_SECURITY_HOOK_LIKELY, &SECURITY_HOOK_ACTIVE_KEY(h, n))
> +
>  /*
>   * Initialise a table of static calls for each LSM hook.
>   * DEFINE_STATIC_CALL_NULL invocation above generates a key (STATIC_CALL_KEY)
> @@ -816,7 +819,7 @@ static int lsm_superblock_alloc(struct super_block *sb)
>   */
>  #define __CALL_STATIC_VOID(NUM, HOOK, ...)				     \
>  do {									     \
> -	if (static_branch_unlikely(&SECURITY_HOOK_ACTIVE_KEY(HOOK, NUM))) {    \
> +	if (security_hook_active(NUM, HOOK)) {    			     \
>  		static_call(LSM_STATIC_CALL(HOOK, NUM))(__VA_ARGS__);	     \
>  	}								     \
>  } while (0);
> @@ -828,7 +831,7 @@ do {									     \
>  
>  #define __CALL_STATIC_INT(NUM, R, HOOK, LABEL, ...)			     \
>  do {									     \
> -	if (static_branch_unlikely(&SECURITY_HOOK_ACTIVE_KEY(HOOK, NUM))) {  \
> +	if (security_hook_active(NUM, HOOK)) {    \
>  		R = static_call(LSM_STATIC_CALL(HOOK, NUM))(__VA_ARGS__);    \
>  		if (R != 0)						     \
>  			goto LABEL;					     \

I actually think I'd prefer there be no macro wrapping
static_branch_maybe(), just for reading it more easily. i.e. people
reading this code are going to expect the static_branch/static_call code
patterns, and seeing "security_hook_active" only slows them down in
understanding it. I don't think it's _that_ ugly to have it all typed
out. e.g.:

	if (static_branch_maybe(CONFIG_SECURITY_HOOK_LIKELY,		     \
				&SECURITY_HOOK_ACTIVE_KEY(HOOK, NUM)) {	     \
  		R = static_call(LSM_STATIC_CALL(HOOK, NUM))(__VA_ARGS__);    \
  		if (R != 0)						     \
  			goto LABEL;					     \



-- 
Kees Cook