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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Using `memcmp()` to compare `struct lsm_prop` fails to match identical properties due to uninitialized padding bytes.
- [High] Time-Of-Check to Time-Of-Use (TOCTOU) race condition in `secxa_from_lsmprop` allows duplicate XArray entries and memory leaks.
--

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

LSM: Implement x array functions for secmarks

This commit implements the functions required to map lsm_prop structures to
xarray indexes for use in secmarks. It introduces secxa_get_lsmprop() and
secxa_from_lsmprop() to manage the mapping.

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

[ ... ]

> +/**
> + * secxa_from_lsmprop - get the secxa associated with a lsm_prop
> + * @prop: lsm_prop pointer
> + * @secxa: result
> + *
> + * Find the secxa associated with @prop. If there is none, create it.
> + *
> + * Returns 0, or an error if the mapping cannot be created
> + */
> +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) {
> +		if (!memcmp(prop, lp, sizeof(*prop))) {

[Severity: Critical]
Will this memcmp() check fail to match identical properties due to uninitialized
padding bytes? 

If struct lsm_prop is ever allocated on the stack without zeroing the entire
structure, the padding bytes or inactive fields could contain uninitialized
stack garbage. 

This might cause logically identical properties to fail the memcmp() comparison,
resulting in continuous duplicate allocations and rapid exhaustion of the 31-bit
ID space.

> +			*secxa = il;
> +			return 0;
> +		}
> +	}
> +
> +	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);

[Severity: High]
Is there a time-of-check to time-of-use race condition when inserting new
entries into the xarray?

The xa_for_each() loop above performs a lockless search for an existing entry.
If multiple threads execute secxa_from_lsmprop() concurrently and simultaneously
fail to find a match, they will all proceed to allocate memory and call
xa_alloc_bh().

Since xa_alloc_bh() simply acquires the lock to assign a new ID and does not
re-verify if an identical lsm_prop was just inserted by another thread, could
this result in duplicate entries and memory leaks over time?

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

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