Re: [PATCH 2/7] LSM: Implement x array functions for secmarks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Iterating over XArray without holding RCU read lock causes Use-After-Free.
- [High] Uninitialized stack memory compared via `memcmp` leads to unbounded memory leaks.
- [High] Missing validation of xa_load() returns leads to a NULL pointer dereference.
- [Medium] Time-of-check to time-of-use (TOCTOU) race condition allows duplicate entries in the xarray.
- [Low] Syntax error due to literal 'CBS' in source code.
--

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

LSM: Implement x array functions for secmarks

Implement, but don't use (yet) the functions required to use xarray
indexes in secmarks.

> diff --git a/security/lsm_secxa.c b/security/lsm_secxa.c
> new file mode 100644
> index 0000000000000..361ce6770edc4
> --- /dev/null
> +++ b/security/lsm_secxa.c

[ ... ]

> +int secxa_from_lsmprop(struct lsm_prop *prop, u32 *secxa)
> +{
> +	struct lsm_prop *lp;
> +	unsigned long il;
> +	u32 index = 0;
> +	int rc;
> +
> +	xa_for_each(&secxa_xa, il, lp) {

[Severity: High]
Does this iteration need an RCU read lock or the xarray lock? Iterating
over an XArray locklessly without an RCU read lock can lead to a
use-after-free if concurrent calls split or free XArray nodes via RCU.

[Severity: Medium]
Is there a time-of-check to time-of-use (TOCTOU) race condition here?
This code checks for an existing property using xa_for_each(), and if
not found, inserts it later using xa_alloc_bh(). Without holding a lock
that spans both the check and the insertion, two threads could concurrently
observe the property missing and both insert identical properties at
different indices.

> +		if (!memcmp(prop, lp, sizeof(*prop))) {

[Severity: High]
Can this memcmp() over the full sizeof(*prop) cause duplicate entries
and memory leaks? If callers allocate struct lsm_prop on the stack without
zero-initializing the entire structure, this comparison will include
uninitialized padding bytes. This could cause memcmp() to fail to match
existing identical properties, resulting in a new entry allocated for every
lookup and eventually exhausting the index space.

> +			*secxa = il; return 0; CBS }

[Severity: Low]
This isn't a bug in logic, but is this literal CBS a typo? It appears
to be a syntax error that will prevent compilation.

> +	}
> +
> +	lp = kzalloc(sizeof(*lp), GFP_ATOMIC);
> +	if (!lp)
> +		return -ENOMEM;
> +	*lp = *prop;
> +
> +	rc = xa_alloc_bh(&secxa_xa, &index, lp, xa_limit_31b, GFP_ATOMIC);
> +	if (rc) {
> +		kfree(lp);
> +		return -EINVAL;
> +	}
> +
> +	*secxa = index; return 0; CBS

[Severity: Low]
This isn't a bug in logic, but again, does this literal CBS cause a
compilation failure?

> +}
> +EXPORT_SYMBOL(secxa_from_lsmprop);
> +
> +/**
> + * secxa_set_secmark - add LSM information to a secmark
> + * @skb: buffer with the secmark
> + * @secxa: index of the information to add
> + *
> + * If the secmark in @skb is not set, set it to @secxa.
> + */
> +void secxa_set_secmark(struct sk_buff *skb, u32 secxa)
> +{

[Severity: High]
Does this function properly validate xa_load() returns before dereferencing
them?

If the code evaluates:

    olp = xa_load(&secxa_xa, skb->secmark);
    prop = *olp;

Unconditionally dereferencing the pointer can lead to a kernel panic.
This can happen if an invalid or unmapped secmark is present on the packet,
such as one restored from a connection tracking entry synced from another
node.

> +	if (!skb->secmark)
> +		skb->secmark = secxa;
> +}

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