Re: [RFC PATCH v1 5/8] misc/arm-cla: Accelerator context save and restore

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Heap buffer overflow due to unit mismatch in internal accelerator state (IAS) memory allocation and bounds checking.
- [Medium] Incomplete context save leaves `regs->accel_valid` as true, leading to corrupted hardware state on subsequent restore.
--

--- Patch [5]: [RFC PATCH v1 5/8] misc/arm-cla: Accelerator context save and restore ---
commit 96459db186d7c9fdaf43a3521dada1e5846d6b74
Author: Jean-Philippe Brucker <jpb@kernel.org>

misc/arm-cla: Accelerator context save and restore

This commit adds helpers to save and restore CLA context for later domain
switching. The saved state includes DATA registers, LRESP, per-accelerator
SRSTATE, and internal accelerator state exposed through the IAS register
window.

> diff --git a/drivers/misc/arm-cla/cla-regs.c b/drivers/misc/arm-cla/cla-regs.c
> new file mode 100644
> index 0000000000000..7507796cd551f
> --- /dev/null
> +++ b/drivers/misc/arm-cla/cla-regs.c
> @@ -0,0 +1,263 @@
[ ... ]
> +static int cla_regs_save_accel(struct cla_dev *dev, unsigned int accid,
> +			       struct cla_regs *regs, off_t *regstate_off)
> +{
[ ... ]
> +	regstate_size = FIELD_GET(CLA_SRSTATE_0_REGSTATE, srstate[0]);
> +	if (WARN_ON(*regstate_off + regstate_size * 8 > dev->iassizes))
> +		return -ENOSPC;

[Severity: High]
Does this bounds check correctly handle the units for *regstate_off?

*regstate_off is incremented by regstate_size (which is a register count),
so it is also a register count.

If dev->iassizes is in bytes, this check improperly adds a register count
(*regstate_off) to a byte count (regstate_size * 8) and compares it against
a byte count.

Because regs->regstate + *regstate_off scales the offset by 8 during the
memory access, this check can pass even when the access goes out of bounds.

For example, if dev->iassizes = 160 bytes, *regstate_off = 20, and
regstate_size = 10, the check evaluates to 20 + 80 <= 160 (passes). However,
the accessed memory is at byte offset (20 + 10) * 8 = 240, overflowing the
buffer.

> +
> +	if (regstate_size) {
> +		ret = cla_op_regread(dev, accid, CLA_REG_IASn, regstate_size,
> +				     regs->regstate + *regstate_off);
[ ... ]
> +static int cla_regs_restore_accel(struct cla_dev *dev, unsigned int accid,
> +				  struct cla_regs *regs, off_t *regstate_off)
> +{
[ ... ]
> +	regstate_size = FIELD_GET(CLA_SRSTATE_0_REGSTATE, srstate[0]);
> +	if (WARN_ON(*regstate_off + regstate_size * 8 > dev->iassizes))
> +		return -ENOSPC;

[Severity: High]
This appears to have the exact same unit mismatch issue as
cla_regs_save_accel(), which could lead to out-of-bounds memory reads when
restoring accelerator state.

> +
> +	if (regstate_size) {
> +		ret = cla_op_regwrite(dev, accid, CLA_REG_IASn, regstate_size,
> +				      regs->regstate + *regstate_off);
[ ... ]
> +int cla_regs_switch_out(struct cla_dev *dev, struct cla_regs *regs,
> +			bool save_regs)
> +{
[ ... ]
> +	if (save_regs) {
> +		for (i = 0; i < CLA_NUM_DATA_REGS; i++)
> +			regs->data[i] = cla_reg_read(dev, CLA_REG_DATA(i));
> +
> +		cla_for_each_accid(dev, accid) {
> +			ret = cla_regs_save_accel(dev, accid, regs, &regstate_off);
> +			if (ret)
> +				return ret;

[Severity: Medium]
If cla_regs_save_accel() fails here, the function returns without clearing
regs->accel_valid.

Since regs is allocated once per domain and reused, accel_valid might
already be true from a previous successful save. Returning an error here
leaves the structure partially overwritten (a mix of old and new accelerator
states) but still marked as valid.

If the caller handles this switch-out failure non-fatally and retries the
context later, will cla_regs_switch_in() restore this corrupted state to
the hardware?

> +		}
> +
> +		regs->accel_valid = true;
> +	}
[ ... ]
> +struct cla_regs **cla_regs_alloc_domain(struct cla_domain *domain)
> +{
[ ... ]
> +	for (i = 0; i < domain->nr_devs; i++) {
> +		struct cla_regs *regs;
> +		struct cla_dev *dev = domain->devs[i];
> +
> +		/*
> +		 * The regs structures are only ever accessed from the CLA
> +		 * device's CPU, so try to allocate them on the right NUMA node
> +		 */
> +		size = sizeof(*regs) + dev->iassizes;

[Severity: High]
If dev->iassizes represents a register count rather than a byte count,
should it be scaled by sizeof(u64) here?

If it is unscaled, the allocation for the flexible array regs->regstate
will be 8 times too small, leading to heap buffer overflows during save and
restore operations.

> +		regs = kvzalloc_node(size, GFP_KERNEL_ACCOUNT,
> +				     cpu_to_node(dev->cpu));

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717104759.123203-1-ryan.roberts@arm.com?part=5