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

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Newly allocated lsm_prop is published to the xarray before its contents are initialized, exposing zeroed memory to concurrent readers.
- [High] The check-then-allocate sequence (`xa_for_each` then `xa_alloc`) lacks synchronization, allowing concurrent duplicate allocations for the same property.
- [High] `secxa_from_lsmprop` returns an `int` but allows allocating indices up to `U32_MAX`, turning valid high indices into negative error codes.
- [Medium] `xa_alloc` is used without disabling softirqs, risking deadlocks if called from process context and preempted by a network softirq.
- [Low] Missing braces around an if-statement cause a redundant `memcmp` operation.
--

--- Patch [2]: [PATCH 2/7] LSM: Implement x array functions for secmarks ---
commit 71cb09431142a3a514763b3603252091420dfe8f
Author: Casey Schaufler <[email protected]>
LSM: Implement x array functions for secmarks

This commit implements the infrastructure for mapping LSM properties to secmarks
using an xarray, allowing future code to store and retrieve these properties via
an allocated index.

> diff --git a/security/lsm_secxa.c b/security/lsm_secxa.c
> new file mode 100644
> index 0000000000000..5b67d8218fd25
> --- /dev/null
> +++ b/security/lsm_secxa.c
> @@ -0,0 +1,107 @@

[ ... ]

> +int secxa_from_lsmprop(struct lsm_prop *prop)
> +{
> +	struct lsm_prop *lp;
> +	unsigned long il;
> +	unsigned int index = 0;
> +	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;
> +	}

[Severity: Low]
This isn't a bug, but did you intend to evaluate memcmp() twice? The first
if statement lacks braces, so the second memcmp() is evaluated on every
loop iteration. Can these be combined into a single check with braces?

[Severity: High]
Could concurrent calls to secxa_from_lsmprop() with the same property allocate
duplicate entries? Since the xa_for_each() loop locklessly checks for
existence before allocating and inserting with xa_alloc(), could two threads
both fail to find the property and insert it twice, wasting xarray indices
and leaking memory?

> +
> +	lp = kzalloc(sizeof(*lp), GFP_ATOMIC);
> +	if (!lp)
> +		return -ENOMEM;
> +
> +	rc = xa_alloc(&secxa_xa, &index, lp, xa_limit_32b, GFP_ATOMIC);

[Severity: Medium]
Is this susceptible to deadlocks if interrupted by network softirqs? The
xarray uses the standard spinlock (via XA_FLAGS_ALLOC1), but secmarks
typically bridge process and softirq contexts. Should this use
XA_FLAGS_LOCK_BH and xa_alloc_bh() to prevent a network softirq from
deadlocking on the same CPU?

[Severity: High]
Will this return negative error codes for valid high indices? The function
returns an int, but xa_limit_32b allows allocating indices up to U32_MAX.
If the index exceeds 0x7FFFFFFF, will callers incorrectly treat the returned
index as an error code? Should this use xa_limit_31b instead?

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

[Severity: High]
Could this publish a zero-initialized lsm_prop to concurrent readers? Since
xa_alloc() makes the entry visible in the xarray before the contents are
copied by *lp = *prop, is it possible for a concurrent secxa_get_lsmprop()
to retrieve an empty property?

> +
> +	return index;
> +}

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