Re: [PATCH 5/7] Smack: hooks for secctx_to_lsmprop and update_lsmprop

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] `smack_secctx_to_lsmprop` sets `prop->smack.skp` to NULL for unknown contexts but returns success, leading to a later NULL pointer dereference in `smack_lsmprop_to_secctx`.

Pre-existing issues:
- [High] The `smack_secctx_to_lsmprop` function ignores the `seclen` parameter and passes a potentially non-NUL-terminated buffer to `smk_find_entry()`, resulting in an out-of-bounds read.
--

commit 7c2ef4130b3b7eeea3057be8cb9283a398ff6cf7
Author: Casey Schaufler <casey@schaufler-ca.com>

Smack: hooks for secctx_to_lsmprop and update_lsmprop

This commit implements the secctx_to_lsmprop and update_lsmprop hooks for
the Smack LSM module. These hooks provide conversions and updates between
security contexts and lsm_prop structures.

> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index ff115068c5c06..fcfadd5d99941 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
[ ... ]
> @@ -4909,6 +4909,37 @@ static int smack_lsmprop_to_secctx(struct lsm_prop *prop,
>  	return smack_to_secctx(prop->smack.skp, cp);
>  }
>  
> +/**
> + * smack_secctx_to_lsmprop - add the smack label to an lsmprop
> + * @secdata: smack label
> + * @seclen: how long label is
> + * @prop: where to put the result
> + *
> + * Exists for audit and networking code.
> + */
> +static int smack_secctx_to_lsmprop(const char *secdata, u32 seclen,
> +				   struct lsm_prop *prop)
> +{
> +	prop->smack.skp = smk_find_entry(secdata);

[Severity: High]
This isn't a bug introduced by this patch, but does this ignore the seclen
parameter and pass a potentially non-NUL-terminated buffer to
smk_find_entry()?

If secdata comes from a netlink NLA_BINARY attribute and isn't inherently
NUL-terminated, passing it directly to smk_find_entry() will trigger an
out-of-bounds read, because smk_find_entry() in security/smack/smack_access.c
unconditionally calls strlen() on the string. This seems to duplicate a
pre-existing flaw from smack_secctx_to_secid, but re-establishes it in this
new API contract.

> +
> +	return 0;

[Severity: High]
Can smk_find_entry() return NULL for an unknown context, and if so, is it
safe to return 0 (success) here?

If an unknown context is passed in, prop->smack.skp becomes NULL. Because
this returns 0, the caller (such as SECMARK via iptables) might assume the
property is fully populated. Later, if the system tries to convert this
property back to a security context string via smack_lsmprop_to_secctx(), it
passes the NULL pointer to smack_to_secctx():

security/smack/smack_lsm.c:smack_lsmprop_to_secctx() {
    return smack_to_secctx(prop->smack.skp, cp);
}

security/smack/smack_lsm.c:smack_to_secctx() {
    int len = strlen(skp->smk_known);
    ...
}

Will this cause a NULL pointer dereference and kernel panic when skp is NULL?

> +}
[ ... ]

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