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 7 potential issue(s) to consider:
- [High] Uninitialized stack memory in `struct lsm_prop` causes duplicate xarray insertions and potential kernel stack leaks.
- [High] Unconditional NULL pointer dereference in `secxa_set_secmark` on invalid secmarks.
- [High] UAPI Breakage / Type Confusion in Netfilter conntrack secmark dumps.
- [High] Unconditional NULL pointer dereference in AppArmor secmark evaluation.
- [High] Uninitialized pointer dereference in `smack_from_skb` due to ignored error code.
- [Medium] Check-then-act (TOCTOU) race condition in `secxa_from_lsmprop`.
- [Low] Syntax error due to stray `CBS` tokens in `secxa_from_lsmprop`.
--

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

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

This commit changes skb->secmark to store an xarray index representing an
lsm_prop structure instead of a direct LSM secid. It updates various netfilter
modules and LSM hooks to translate between this index and the underlying
security properties.

> Maintain a xarray of lsm_prop structures which represent the
> LSM security information passed via skb->secmark. Pass the xarray
> index of the appropriate lsm_prop (the secxa) instead of an LSM
> specific secid. Allow multiple LSMs to specify their components
> in xarray entries, or create new entries as necessary.
>
> Change uses of security_secctx_to_secid() to security_secctx_to_lsmprop()
> in the netfilter and iptables code. Change security_secmark_relabel_packet()
> to accept an lsm_prop pointer rather than a secid. Change secxa_set_secmark()
> to update and create new entries as necessary.

[Severity: High]
Are there missing updates for connection tracking? The commit updates
netfilter and iptables code, but it appears conntrack dumps via netlink
(ctnetlink_dump_secctx) and procfs (seq_print_acct) still pass ct->secmark
directly to security_secid_to_secctx(). Since the secmark is now an xarray
index rather than an LSM secid, won't this result in invalid memory lookups
and broken userspace dumps?

[ ... ]

> diff --git a/net/netfilter/nft_meta.c b/net/netfilter/nft_meta.c
> index bd0f7a0931f44..664191dfa4b29 100644
> --- a/net/netfilter/nft_meta.c
> +++ b/net/netfilter/nft_meta.c
> @@ -927,21 +927,24 @@ static const struct nla_policy nft_secmark_policy[NFTA_SECMARK_MAX + 1] = {
>  
>  static int nft_secmark_compute_secid(struct nft_secmark *priv)
>  {
> -	u32 tmp_secid = 0;
> +	struct lsm_prop tmp_prop;
> +	u32 secxa = 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]
Can this leak uninitialized stack memory into the xarray cache? The tmp_prop
structure is declared on the stack without zeroing. When
security_secctx_to_lsmprop() populates it, it may only write to the specific
active LSM's fields, leaving the padding and other fields uninitialized.
Later, when secxa_from_lsmprop() performs a memcmp on this struct, the
uninitialized stack garbage will prevent matches on identical labels and cause
boundless duplicate entries to be allocated.

[ ... ]

> diff --git a/net/netfilter/xt_SECMARK.c b/net/netfilter/xt_SECMARK.c
> index ea67aa92ddc2f..05b023a7c5760 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_secmark_compute_secid(), does this code also pass
an uninitialized struct lsm_prop to security_secctx_to_lsmprop()? This looks
like it could cause the same memcmp failures and duplicate xarray allocations.

[ ... ]

> diff --git a/security/apparmor/net.c b/security/apparmor/net.c
> index cf590dd08540c..e26e15c2d9473 100644
> --- a/security/apparmor/net.c
> +++ b/security/apparmor/net.c
> @@ -365,12 +366,17 @@ static int aa_secmark_perm(struct aa_profile *profile, u32 request, u32 secid,
>  			   struct apparmor_audit_data *ad)
>  {
[ ... ]
> +	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]);
> @@ -378,7 +384,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]
Will this unconditionally dereference a NULL pointer if the packet was labeled
by a different LSM? If another LSM (like SELinux) populated the lsm_prop
structure but AppArmor did not, prop->apparmor.label would be NULL, causing
a panic when accessing its secid field.

[ ... ]

> diff --git a/security/lsm_secxa.c b/security/lsm_secxa.c
> index 361ce6770edc4..7ad6a6711f881 100644
> --- a/security/lsm_secxa.c
> +++ b/security/lsm_secxa.c
> @@ -99,7 +99,25 @@ EXPORT_SYMBOL(secxa_from_lsmprop);
>   */
>  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;
> +	u32 tsecxa;
> +	int rc;
> +
> +	if (!skb->secmark) {
>  		skb->secmark = secxa;
> +		return;
> +	}
> +
> +	olp = xa_load(&secxa_xa, skb->secmark);
> +	nlp = xa_load(&secxa_xa, secxa);
> +
> +	prop = *olp;

[Severity: High]
Can xa_load() return NULL here and cause an unconditional NULL pointer
dereference? If network packets arrive with an invalid or untrusted secmark,
the xarray lookup could fail. Does this code need to check if olp and nlp
are valid before dereferencing them?

[ ... ]

> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index fcfadd5d99941..79140e6829a47 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -4189,10 +4190,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: High]
Does this code ignore the return value of secxa_get_lsmprop() and evaluate
an uninitialized pointer? If the lookup fails, prop remains uninitialized
and checking if (prop) will evaluate stack garbage, potentially leading to
a wild pointer dereference when accessing prop->smack.skp. Should this check
if (!rc) instead?

[ ... ]

Additionally, in the new secxa_from_lsmprop() function introduced in this
series:

[Severity: Low]
Is there a syntax error here that will prevent the kernel from compiling?

security/lsm_secxa.c:secxa_from_lsmprop() {
    ...
    xa_for_each(&secxa_xa, il, lp) {
        if (!memcmp(prop, lp, sizeof(*prop))) {
            *secxa = il; return 0; CBS }
                                   ^^^
The CBS token appears to be a stray undeclared identifier at the end of the
return statement.

[Severity: Medium]
Is there a check-then-act (TOCTOU) race condition during concurrent xarray
insertions?

security/lsm_secxa.c:secxa_from_lsmprop() {
    ...
    xa_for_each(&secxa_xa, il, lp) {
        if (!memcmp(prop, lp, sizeof(*prop))) {
            *secxa = il; return 0;
        }
    }
    ...
    rc = xa_alloc_bh(&secxa_xa, &index, lp, xa_limit_31b, GFP_ATOMIC);
    ...
}

The lockless xa_for_each search and the subsequent xa_alloc_bh insertion
are not atomic. If two threads check for the same property simultaneously,
could they both fail the initial check and blindly insert duplicates into
the xarray cache?

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