Re: [PATCH v2 3/7] LSM: Two hooks for manipulating struct lsm_prop

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The wrapper security_secctx_to_lsmprop() fails to zero-initialize the output struct lsm_prop, risking uninitialized memory usage.
- [Medium] Inconsistent default return value for unsupported security_secctx_to_lsmprop operation (-EINVAL vs -EOPNOTSUPP).
- [Low] The `update_lsmprop` hook leaks framework routing details by requiring `lsmid` as a parameter, forcing LSM implementations to duplicate dispatch filtering instead of handling it centrally in the LSM framework.
--

commit 945240d1e544dee2c2c0632c7b84efeac1548fde
Author: Casey Schaufler <casey@schaufler-ca.com>

LSM: Two hooks for manipulating struct lsm_prop

This commit introduces two new hooks, update_lsmprop and secctx_to_lsmprop,
which allow updating the properties of a specified LSM and setting the 
lsm_prop field based on a passed security context.

> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index 65c9609ec2077..679c40a8e1275 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
[ ... ]
> @@ -305,7 +305,11 @@ LSM_HOOK(int, 0, ismaclabel, const char *name)
>  LSM_HOOK(int, -EOPNOTSUPP, secid_to_secctx, u32 secid, struct lsm_context *cp)
>  LSM_HOOK(int, -EOPNOTSUPP, lsmprop_to_secctx, struct lsm_prop *prop,
>  	 struct lsm_context *cp)
> +LSM_HOOK(void, LSM_RET_VOID, update_lsmprop, struct lsm_prop *dest,
> +	 struct lsm_prop *src, int lsmid)

[Severity: Low]
Does exposing lsmid directly to the update_lsmprop hook break the
encapsulation of the LSM routing mechanism? In the LSM framework, targeted
hooks usually handle lsmid filtering centrally in security.c using an
explicit loop (e.g., lsmprop_to_secctx). By broadcasting with call_void_hook
and passing lsmid, individual LSMs are forced to implement duplicate
filtering rather than having it handled by the framework.

>  LSM_HOOK(int, 0, secctx_to_secid, const char *secdata, u32 seclen, u32 *secid)
> +LSM_HOOK(int, -EINVAL, secctx_to_lsmprop, const char *secdata, u32 seclen,
> +	 struct lsm_prop *prop)

[Severity: Medium]
Is -EINVAL the correct default return value here? The inline stub for
CONFIG_SECURITY=n in security.h returns -EOPNOTSUPP. Returning -EINVAL when
no LSM handles the hook might incorrectly imply a parsing error rather than
a lack of support, potentially altering user-space visible error codes
(e.g., returning -EINVAL instead of -ENOENT or -EOPNOTSUPP from
xtables/nftables).

>  LSM_HOOK(void, LSM_RET_VOID, release_secctx, struct lsm_context *cp)
>  LSM_HOOK(void, LSM_RET_VOID, inode_invalidate_secctx, struct inode *inode)
>  LSM_HOOK(int, 0, inode_notifysecctx, struct inode *inode, void *ctx, u32 ctxlen)
> diff --git a/security/security.c b/security/security.c
> index 71aea8fdf0142..1dec0037370ba 100644
> --- a/security/security.c
> +++ b/security/security.c
[ ... ]
> @@ -3982,6 +3989,31 @@ int security_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid)
>  }
>  EXPORT_SYMBOL(security_secctx_to_secid);
>  
> +/**
> + * security_secctx_to_lsmprop() - Convert a secctx to a lsmprop
> + * @secdata: secctx
> + * @seclen: length of secctx
> + * @prop: prop
> + * @lsmid: which LSM the context is appropriate to.
> + *
> + * Convert security context to an lsmprop.
> + *
> + * Return: Returns 0 on success, error on failure.
> + */
> +int security_secctx_to_lsmprop(const char *secdata, u32 seclen,
> +			       struct lsm_prop *prop, int lsmid)
> +{
> +	struct lsm_static_call *scall;
> +
> +	lsm_for_each_hook(scall, secctx_to_lsmprop) {

[Severity: High]
Should this function zero-initialize the output struct lsm_prop (perhaps
via lsmprop_init) before calling the hooks?

Callers typically allocate struct lsm_prop on the stack without
initialization, and the active LSM hook only populates its own specific
field. This leaves the rest of the struct as uninitialized stack garbage.

If callers pass this partially initialized structure to secxa_from_lsmprop(),
the memcmp() and kmemdup() operations on the entire struct will evaluate the
stack garbage. This could leak kernel stack memory into the secxa_xa xarray
heap object and cause identical contexts to mismatch, potentially leading to
unbounded memory allocation and a denial of service.

> +		if (lsmid != LSM_ID_UNDEF && lsmid != scall->hl->lsmid->id)
> +			continue;
> +		return scall->hl->hook.secctx_to_lsmprop(secdata, seclen, prop);
> +	}
> +	return LSM_RET_DEFAULT(secctx_to_lsmprop);
> +}
> +EXPORT_SYMBOL(security_secctx_to_lsmprop);
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902220150.18586-1-casey@schaufler-ca.com?part=3