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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Out-of-bounds read in `smack_secctx_to_lsmprop` due to ignored `seclen` parameter.
- [High] NULL pointer dereference when an unknown security label is provided to `smack_secctx_to_lsmprop`.
- [High] RCU locking violation when calling `smk_find_entry()` from `smack_secctx_to_lsmprop`.
--

--- Patch [5]: [PATCH 5/7] Smack: hooks for secctx_to_lsmprop and update_lsmprop ---
commit dbfa723dc5394e90f883ff953a774973bbb1759e
Author: Casey Schaufler <[email protected]>

Smack: hooks for secctx_to_lsmprop and update_lsmprop

Implement these hooks.

> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 3f9ae05039a28..cd9f5765979ff 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
[ ... ]
> @@ -4914,6 +4914,40 @@ 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]
Does this code ignore the seclen parameter? If secdata is provided by
networking code or audit without null termination, passing it directly to
smk_find_entry() will cause an out-of-bounds read:

security/smack/smack_access.c:smk_find_entry() {
	...
	hash = full_name_hash(NULL, string, strlen(string));
	...
}

Should this code respect seclen or ensure secdata is null-terminated before
the search?


[Severity: High]
Is it safe to call smk_find_entry() here without holding the RCU read lock?

Looking at smk_find_entry() in security/smack/smack_access.c, it traverses the
hash list using RCU:

	hlist_for_each_entry_rcu(skp, head, smk_hashed) {
		...
	}

Without an rcu_read_lock() wrapping this call, could this trigger a lockdep
warning or read malformed pointers during a concurrent insertion?

> +
> +	return 0;

[Severity: High]
If smk_find_entry() fails to find a matching label, it returns NULL. By
returning 0 (success) here, we leave prop->smack.skp as NULL.

When this property is later passed to smack_lsmprop_to_secctx(), it will
delegate to smack_to_secctx():

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

Will this cause a deterministic NULL pointer dereference? Should this
function return an error like -EINVAL when the label is not found?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5