Re: [PATCH v13 4/5] security: Update non standard hooks to use static calls

[Paul Moore <paul@paul-moore.com>]

On Jun 29, 2024 KP Singh <kpsingh@kernel.org> wrote:
> 
> There are some LSM hooks which do not use the common pattern followed
> by other LSM hooks and thus cannot use call_{int, void}_hook macros and
> instead use lsm_for_each_hook macro which still results in indirect
> call.
> 
> There is one additional generalizable pattern where a hook matching an
> lsmid is called and the indirect calls for these are addressed with the
> newly added call_hook_with_lsmid macro which internally uses an
> implementation similar to call_int_hook but has an additional check that
> matches the lsmid.
> 
> For the generic case the lsm_for_each_hook macro is updated to accept
> logic before and after the invocation of the LSM hook (static call) in
> the unrolled loop.
> 
> Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: KP Singh <kpsingh@kernel.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> ---
>  security/security.c | 248 +++++++++++++++++++++++++-------------------
>  1 file changed, 144 insertions(+), 104 deletions(-)
> 
> diff --git a/security/security.c b/security/security.c
> index e0ec185cf125..4f0f35857217 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -948,10 +948,48 @@ out:									\
>  	RC;								\
>  })
>  
> -#define lsm_for_each_hook(scall, NAME)					\
> -	for (scall = static_calls_table.NAME;				\
> -	     scall - static_calls_table.NAME < MAX_LSM_COUNT; scall++)  \
> -		if (static_key_enabled(&scall->active->key))
> +/*
> + * Can be used in the context passed to lsm_for_each_hook to get the lsmid of the
> + * current hook
> + */
> +#define current_lsmid() _hook_lsmid

See my comments below about security_getselfattr(), I think we can drop
the current_lsmid() macro.  If we really must keep it, we need to rename
it to something else as it clashes too much with the other current_XXX()
macros/functions which are useful outside of our wacky macros.

> +#define __CALL_HOOK(NUM, HOOK, RC, BLOCK_BEFORE, BLOCK_AFTER, ...)	     \
> +do {									     \
> +	int __maybe_unused _hook_lsmid;					     \
> +									     \
> +	if (static_branch_unlikely(&SECURITY_HOOK_ACTIVE_KEY(HOOK, NUM))) {  \
> +		_hook_lsmid = static_calls_table.HOOK[NUM].hl->lsmid->id;    \
> +		BLOCK_BEFORE						     \
> +		RC = static_call(LSM_STATIC_CALL(HOOK, NUM))(__VA_ARGS__);   \
> +		BLOCK_AFTER						     \
> +	}								     \
> +} while (0);
> +
> +#define lsm_for_each_hook(HOOK, RC, BLOCK_AFTER, ...)	\
> +	LSM_LOOP_UNROLL(__CALL_HOOK, HOOK, RC, ;, BLOCK_AFTER, __VA_ARGS__)
> +
> +#define call_hook_with_lsmid(HOOK, LSMID, ...)				\
> +({									\
> +	__label__ out;							\
> +	int RC = LSM_RET_DEFAULT(HOOK);					\
> +									\
> +	LSM_LOOP_UNROLL(__CALL_HOOK, HOOK, RC,				\
> +	/* BLOCK BEFORE INVOCATION */					\
> +	{								\
> +		if (current_lsmid() != LSMID)				\
> +			continue;					\
> +	},								\
> +	/* END BLOCK BEFORE INVOCATION */				\
> +	/* BLOCK AFTER INVOCATION */					\
> +	{								\
> +		goto out;						\
> +	},								\
> +	/* END BLOCK AFTER INVOCATION */				\
> +	__VA_ARGS__);							\
> +out:									\
> +	RC;								\
> +})
>  
>  /* Security operations */

...

> @@ -1581,15 +1629,19 @@ int security_sb_set_mnt_opts(struct super_block *sb,
>  			     unsigned long kern_flags,
>  			     unsigned long *set_kern_flags)
>  {
> -	struct lsm_static_call *scall;
>  	int rc = mnt_opts ? -EOPNOTSUPP : LSM_RET_DEFAULT(sb_set_mnt_opts);
>  
> -	lsm_for_each_hook(scall, sb_set_mnt_opts) {
> -		rc = scall->hl->hook.sb_set_mnt_opts(sb, mnt_opts, kern_flags,
> -					      set_kern_flags);
> -		if (rc != LSM_RET_DEFAULT(sb_set_mnt_opts))
> -			break;
> -	}
> +	lsm_for_each_hook(
> +		sb_set_mnt_opts, rc,
> +		/* BLOCK AFTER INVOCATION */
> +		{
> +			if (rc != LSM_RET_DEFAULT(sb_set_mnt_opts))
> +				goto out;
> +		},
> +		/* END BLOCK AFTER INVOCATION */
> +		sb, mnt_opts, kern_flags, set_kern_flags);
> +
> +out:
>  	return rc;
>  }
>  EXPORT_SYMBOL(security_sb_set_mnt_opts);

I know I was the one who asked to implement the static_calls for *all*
of the LSM functions - thank you for doing that - but I think we can
all agree that some of the resulting code is pretty awful.  I'm probably
missing something important, but would an apporach similar to the pseudo
code below work?

  #define call_int_hook_special(HOOK, RC, LABEL, ...) \
    LSM_LOOP_UNROLL(HOOK##_SPECIAL, RC, HOOK, LABEL, __VA_ARGS__)

  int security_sb_set_mnt_opts(...)
  {
      int rc = LSM_RET_DEFAULT(sb_set_mnt_opts);

  #define sb_set_mnt_opts_SPECIAL \
      do { \
        if (rc != LSM_RET_DEFAULT(sb_set_mnt_opts)) \
          goto out; \
      } while (0)

      rc = call_int_hook_special(sb_set_mnt_opts, rc, out, ...);

  out:
    return rc;
  }

> @@ -4040,7 +4099,6 @@ EXPORT_SYMBOL(security_d_instantiate);
>  int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
>  			 u32 __user *size, u32 flags)
>  {
> -	struct lsm_static_call *scall;
>  	struct lsm_ctx lctx = { .id = LSM_ID_UNDEF, };
>  	u8 __user *base = (u8 __user *)uctx;
>  	u32 entrysize;
> @@ -4078,31 +4136,42 @@ int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
>  	 * In the usual case gather all the data from the LSMs.
>  	 * In the single case only get the data from the LSM specified.
>  	 */
> -	lsm_for_each_hook(scall, getselfattr) {
> -		if (single && lctx.id != scall->hl->lsmid->id)
> -			continue;
> -		entrysize = left;
> -		if (base)
> -			uctx = (struct lsm_ctx __user *)(base + total);
> -		rc = scall->hl->hook.getselfattr(attr, uctx, &entrysize, flags);
> -		if (rc == -EOPNOTSUPP) {
> -			rc = 0;
> -			continue;
> -		}
> -		if (rc == -E2BIG) {
> -			rc = 0;
> -			left = 0;
> -			toobig = true;
> -		} else if (rc < 0)
> -			return rc;
> -		else
> -			left -= entrysize;
> +	LSM_LOOP_UNROLL(
> +		__CALL_HOOK, getselfattr, rc,
> +		/* BLOCK BEFORE INVOCATION */
> +		{
> +			if (single && lctx.id != current_lsmid())
> +				continue;
> +			entrysize = left;
> +			if (base)
> +				uctx = (struct lsm_ctx __user *)(base + total);
> +		},
> +		/* END BLOCK BEFORE INVOCATION */
> +		/* BLOCK AFTER INVOCATION */
> +		{
> +			if (rc == -EOPNOTSUPP) {
> +				rc = 0;
> +			} else {
> +				if (rc == -E2BIG) {
> +					rc = 0;
> +					left = 0;
> +					toobig = true;
> +				} else if (rc < 0)
> +					return rc;
> +				else
> +					left -= entrysize;
> +
> +				total += entrysize;
> +				count += rc;
> +				if (single)
> +					goto out;
> +			}
> +		},
> +		/* END BLOCK AFTER INVOCATION */
> +		attr, uctx, &entrysize, flags);
> +
> +out:
>  
> -		total += entrysize;
> -		count += rc;
> -		if (single)
> -			break;
> -	}
>  	if (put_user(total, size))
>  		return -EFAULT;
>  	if (toobig)

I think we may need to admit defeat with security_getselfattr() and
leave it as-is, the above is just too ugly to live.  I'd suggest
adding a comment explaining that it wasn't converted due to complexity
and the resulting awfulness.

--
paul-moore.com