Re: [PATCH 6/7] Apparmor: hooks for secctx_to_lsmprop and update_lsmprop

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

New issues:
- [High] apparmor_secctx_to_lsmprop leaks a reference count on the parsed aa_label, exposing a fundamental design conflict with lsm_prop.

Pre-existing issues:
- [High] Preexisting memory leak in apparmor_secctx_to_secid when extracting the secid from a parsed aa_label.
--

--- Patch [6]: [PATCH 6/7] Apparmor: hooks for secctx_to_lsmprop and update_lsmprop ---
commit 81357db42de4a31531a31239f8d75a01c866a80e
Author: Casey Schaufler <[email protected]>

Apparmor: hooks for secctx_to_lsmprop and update_lsmprop

This commit implements the secctx_to_lsmprop and update_lsmprop LSM
hooks for AppArmor. It adds the capability to convert a security
context string into a struct lsm_prop containing an AppArmor label.

> diff --git a/security/apparmor/secid.c b/security/apparmor/secid.c
> index 28caf66b90335..51f3e4ab053e5 100644
> --- a/security/apparmor/secid.c
> +++ b/security/apparmor/secid.c
> @@ -106,6 +106,29 @@ int apparmor_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid)
>  	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but does apparmor_secctx_to_secid leak the
label reference?

When aa_label_strn_parse is called, it allocates or refcounts an aa_label.
The secid is extracted, but it appears aa_put_label is never called
before returning, causing a memory leak that could be triggered by an
unprivileged user with CAP_NET_ADMIN in a user or network namespace.

> +int apparmor_secctx_to_lsmprop(const char *secdata, u32 seclen,
> +			       struct lsm_prop *prop)
> +{
> +	struct aa_label *label;
> +
> +	label = aa_label_strn_parse(&root_ns->unconfined->label, secdata,
> +				    seclen, GFP_KERNEL, false, false);
> +	if (IS_ERR(label))
> +		return PTR_ERR(label);
> +	prop->apparmor.label = label;
> +
> +	return 0;
> +}

[Severity: High]
Does this code leak the refcount on the parsed aa_label?

Similar to the above issue, aa_label_strn_parse returns a refcounted label,
but struct lsm_prop does not seem to have a mechanism to release it.

Furthermore, since struct lsm_prop acts as an unmanaged container (as seen
in apparmor_update_lsmprop performing a raw pointer copy), if we were
to drop the reference here, wouldn't prop->apparmor.label become a dangling
pointer, leading to a use-after-free?

> +int apparmor_update_lsmprop(struct lsm_prop *dest, struct lsm_prop *src,
> +			    int lsmid)
> +{
> +	if (lsmid == LSM_ID_APPARMOR || lsmid == LSM_ID_UNDEF)
> +		dest->apparmor.label = src->apparmor.label;
> +
> +	return 0;
> +}

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