Re: [PATCH 7/7] net, lsm: Change skb secmarks to x-array indexes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [Critical] Missing NULL checks on the results of `xa_load()` in `secxa_set_secmark()` causes unconditional kernel panics.
- [Critical] In `smack_from_skb()`, failure from `secxa_get_lsmprop()` leaves a stack pointer uninitialized, leading to a wild pointer dereference.
- [High] Uninitialized stack memory causes XArray deduplication failures and leaks kernel stack data.
- [High] Negative error codes from `secxa_from_lsmprop()` are truncated to unsigned 32-bit integers, bypassing error checks.
- [High] The XArray is allowed to allocate index 0, breaking the `skb->secmark == 0` universal sentinel for unlabeled packets.
- [High] A newly allocated `struct lsm_prop` is inserted into the global XArray before its contents are initialized, leading to a race condition that corrupts packet labels.
- [High] AppArmor unconditionally dereferences `prop->apparmor.label`, leading to a crash if the secmark was created by another LSM.
- [Medium] The XArray allocation limit `xa_limit_32b` exceeds the maximum positive value representable by the `int` return type.
- [Medium] Missing synchronization around the read-allocate-insert sequence in `secxa_from_lsmprop()` allows concurrent threads to insert duplicate entries.
- [Medium] SELinux hooks evaluate packets as unlabeled when `secxa_get_lsmprop()` fails, potentially bypassing security policies.
--

--- Patch [7]: [PATCH 7/7] net, lsm: Change skb secmarks to x-array indexes ---
commit 4d10524e4c6ddab86481df7f6394707d0a6f8278
Author: Casey Schaufler <casey@schaufler-ca.com>

net, lsm: Change skb secmarks to x-array indexes

This commit shifts the skb secmark infrastructure to maintain an xarray of
lsm_prop structures. It replaces the LSM-specific secid with an xarray
index representing the security information.

> diff --git a/net/netfilter/nft_meta.c b/net/netfilter/nft_meta.c
> index bd9a9377919cf..0ce662ec3de06 100644
> --- a/net/netfilter/nft_meta.c
> +++ b/net/netfilter/nft_meta.c
> @@ -926,17 +926,20 @@ static const struct nla_policy nft_secmark_policy[NFTA_SECMARK_MAX + 1] = {
>  
>  static int nft_secmark_compute_secid(struct nft_secmark *priv)
>  {
> +	struct lsm_prop tmp_prop;
>  	u32 tmp_secid = 0;
>  	int err;
>  
> -	err = security_secctx_to_secid(priv->ctx, strlen(priv->ctx), &tmp_secid);
> +	err = security_secctx_to_lsmprop(priv->ctx, strlen(priv->ctx),
> +					 &tmp_prop, LSM_ID_UNDEF);

[Severity: High]
Does this leave the tmp_prop structure partially uninitialized on the stack?

Since security_secctx_to_lsmprop() only initializes fields for the active
LSM, the rest of the struct might contain stack garbage. When passed to
secxa_from_lsmprop(), memcmp() is used on the entire struct, which can fail
to match due to garbage bytes, breaking XArray deduplication and leaking
kernel stack data.

Is it necessary to initialize this with an empty struct assignment?

>  	if (err)
>  		return err;
>  
> -	if (!tmp_secid)
> -		return -ENOENT;
> +	tmp_secid = secxa_from_lsmprop(&tmp_prop);
> +	if (tmp_secid < 0)
> +		return tmp_secid;

[Severity: High]
Can the tmp_secid < 0 check ever be true here?

Since tmp_secid is declared as a u32, a negative error code from
secxa_from_lsmprop() (like -ENOMEM) will be truncated to a large unsigned
integer, bypassing the error check. This could store an invalid secmark that
causes guaranteed lookup failures and crashes later.

> diff --git a/net/netfilter/xt_SECMARK.c b/net/netfilter/xt_SECMARK.c
> index ea67aa92ddc2f..5a7b83c674305 100644
> --- a/net/netfilter/xt_SECMARK.c
> +++ b/net/netfilter/xt_SECMARK.c
> @@ -43,13 +43,15 @@ secmark_tg(struct sk_buff *skb, const struct xt_secmark_target_info_v1 *info)
>  
>  static int checkentry_lsm(struct xt_secmark_target_info_v1 *info)
>  {
> +	struct lsm_prop prop;
>  	int err;
>  
>  	info->secctx[SECMARK_SECCTX_MAX - 1] = '\0';
>  	info->secid = 0;
>  
> -	err = security_secctx_to_secid(info->secctx, strlen(info->secctx),
> -				       &info->secid);
> +	err = security_secctx_to_lsmprop(info->secctx, strlen(info->secctx),
> +					 &prop, LSM_ID_UNDEF);
> +

[Severity: High]
Similar to the issue in nft_meta.c, does this leave the prop variable
partially uninitialized on the stack?

[ ... ]

> -	err = security_secmark_relabel_packet(info->secid);
> +	err = security_secmark_relabel_packet(&prop);
>  	if (err) {
>  		pr_info_ratelimited("unable to obtain relabeling permission\n");
>  		return err;
>  	}
>  
> +	info->secid = secxa_from_lsmprop(&prop);

[Severity: High]
Does this directly assign a potential negative error code to info->secid
without any error checking?

> diff --git a/security/apparmor/net.c b/security/apparmor/net.c
> index 44c04102062f3..1d33734d15594 100644
> --- a/security/apparmor/net.c
> +++ b/security/apparmor/net.c
> @@ -362,12 +363,17 @@ static int aa_secmark_perm(struct aa_profile *profile, u32 request, u32 secid,
>  			   struct apparmor_audit_data *ad)
>  {
>  	int i, ret;
> +	struct lsm_prop *prop;
>  	struct aa_perms perms = { };
>  	struct aa_ruleset *rules = profile->label.rules[0];
>  
>  	if (rules->secmark_count == 0)
>  		return 0;
>  
> +	ret = secxa_get_lsmprop(&prop, secid);
> +	if (ret)
> +		return ret;
> +
>  	for (i = 0; i < rules->secmark_count; i++) {
>  		if (!rules->secmark[i].secid) {
>  			ret = apparmor_secmark_init(&rules->secmark[i]);
> @@ -375,7 +381,7 @@ static int aa_secmark_perm(struct aa_profile *profile, u32 request, u32 secid,
>  				return ret;
>  		}
>  
> -		if (rules->secmark[i].secid == secid ||
> +		if (rules->secmark[i].secid == prop->apparmor.label->secid ||

[Severity: High]
Does this unconditionally dereference prop->apparmor.label?

If the packet's secmark was generated by a different LSM (like SELinux
setting iptables rules), the apparmor.label field might be NULL. Could this
cause a NULL pointer dereference crash?

> diff --git a/security/lsm_secxa.c b/security/lsm_secxa.c
> index 5b67d8218fd25..2015dab01bdf4 100644
> --- a/security/lsm_secxa.c
> +++ b/security/lsm_secxa.c
> @@ -71,8 +71,6 @@ int secxa_from_lsmprop(struct lsm_prop *prop)
>  	int rc;
>  
>  	xa_for_each(&secxa_xa, il, lp) {
> -		if (!memcmp(prop, lp, sizeof(*prop)))
> -			pr_info("%s found at index %lu\n", __func__, il);
>  		if (!memcmp(prop, lp, sizeof(*prop)))
>  			return il;
>  	}
> @@ -101,7 +99,21 @@ EXPORT_SYMBOL(secxa_from_lsmprop);

[Severity: Medium]
Is this read-allocate-insert sequence safe from concurrent updates?

Since there is no lock protecting this check-then-act block, multiple
threads processing packets with identical security contexts could
simultaneously fail the search phase and insert duplicate entries.

> +	rc = xa_alloc(&secxa_xa, &index, lp, xa_limit_32b, GFP_ATOMIC);

[Severity: High]
Could xa_alloc assign index 0 here?

The networking core across subsystems uses skb->secmark == 0 as a universal
sentinel for unlabeled packets. If index 0 is allocated to a valid context,
will those labeled packets be incorrectly evaluated as unlabeled?

[Severity: Medium]
Because xa_limit_32b allows indices up to U32_MAX, could an index larger
than INT_MAX be returned here as a negative integer, causing callers to
treat it as an error code since the function returns an int?

> +	if (rc) {
> +		kfree(lp);
> +		return -EINVAL;
> +	}
> +	*lp = *prop;
> +
> +	return index;
> +}

[Severity: High]
Does this introduce a race condition by assigning *lp = *prop after
xa_alloc() has made the pointer globally visible in the XArray?

Another thread doing a lookup might see the fully zeroed memory and use it
to label its packets before the memory is fully initialized here, leading to
incorrect policy application.

>  void secxa_set_secmark(struct sk_buff *skb, u32 secxa)
>  {
> -	if (!skb->secmark)
> +	struct lsm_prop *olp;
> +	struct lsm_prop *nlp;
> +	struct lsm_prop prop;
> +
> +	if (!skb->secmark) {
>  		skb->secmark = secxa;
> +		return;
> +	}
> +
> +	olp = xa_load(&secxa_xa, skb->secmark);
> +	nlp = xa_load(&secxa_xa, secxa);
> +
> +	prop = *olp;
> +	security_update_lsmprop(&prop, nlp, LSM_ID_UNDEF);

[Severity: Critical]
Does this safely handle xa_load() returning NULL?

If the secmark index does not exist, olp or nlp will be NULL, and the
subsequent prop = *olp assignment or security_update_lsmprop() call would
cause a panic.

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 66f32dfadf9b3..1739dd2eb0779 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -5996,10 +6014,21 @@ static unsigned int selinux_ip_forward(void *priv, struct sk_buff *skb,
>  		}
>  	}
>  
> -	if (secmark_active)
> -		if (avc_has_perm(peer_sid, skb->secmark,
> +	if (secmark_active) {
> +		struct lsm_prop *prop;
> +		u32 secmark = 0;
> +		int err;
> +
> +		if (skb->secmark) {
> +			err = secxa_get_lsmprop(&prop, skb->secmark);
> +			if (!err)
> +				secmark = prop->selinux.secid;
> +		}
> +
> +		if (avc_has_perm(peer_sid, secmark,
>  				 SECCLASS_PACKET, PACKET__FORWARD_IN, &ad))
>  			return NF_DROP;
> +	}

[Severity: Medium]
If secxa_get_lsmprop() fails, secmark retains its default value of 0.
Will this cause the packet to be evaluated as unlabeled instead of being
dropped? This looks like it might act as a fallback that could bypass
restrictive policies. Similar logic applies to selinux_ip_postroute().

> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index cd9f5765979ff..2eddd47556c79 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -4194,10 +4195,17 @@ static int smk_skb_to_addr_ipv6(struct sk_buff *skb, struct sockaddr_in6 *sip)
>  #ifdef CONFIG_NETWORK_SECMARK
>  static struct smack_known *smack_from_skb(struct sk_buff *skb)
>  {
> +	struct lsm_prop *prop;
> +	int rc;
> +
>  	if (skb == NULL || skb->secmark == 0)
>  		return NULL;
>  
> -	return smack_from_secid(skb->secmark);
> +	rc = secxa_get_lsmprop(&prop, skb->secmark);
> +	if (prop)
> +		return prop->smack.skp;

[Severity: Critical]
If secxa_get_lsmprop() returns an error, could prop remain uninitialized on
the stack?

The if (prop) check evaluates this uninitialized stack memory, which might
evaluate to true and lead to a wild pointer dereference when accessing
prop->smack.skp.

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