Re: [RFC PATCH v1 2/8] misc/arm-cla: Add launch operation helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] A 32-bit integer overflow when calculating `reg_group + 8` causes out-of-bounds memory access on the `regs` buffer and an infinite loop for high register indices.
- [High] The safety bounds check `regidx + nregs > max_regidx` is vulnerable to a 64-bit integer overflow, allowing massive `nregs` values to bypass the limit check.
--

--- Patch [2]: [RFC PATCH v1 2/8] misc/arm-cla: Add launch operation helpers ---
commit dc6365dd6ad591fc5563520a7266898c01430558
Author: Jean-Philippe Brucker <jpb@kernel.org>

misc/arm-cla: Add launch operation helpers

CLA commands are issued by programming the LAUNCH register and polling LRESP.
This patch adds a common launch helper to perform this sequence and translates
response codes into Linux errors, along with register read and write support.

> diff --git a/drivers/misc/arm-cla/cla-ops.c b/drivers/misc/arm-cla/cla-ops.c
> new file mode 100644
> index 0000000000000..d594344a2eb45
> --- /dev/null
> +++ b/drivers/misc/arm-cla/cla-ops.c
[ ... ]
> +static int cla_op_access_reg(struct cla_dev *dev, u8 op,
> +			     enum cla_launch_data_mode data_mode,
> +			     unsigned int accid, unsigned int regidx,
> +			     size_t nregs, u64 *regs)
> +{
> +	int ret = -EINVAL;
> +	unsigned long max_regidx;
> +	struct cla_launch launch = {
> +		.op = op,
> +		.accid = accid,
> +		.data_mode = data_mode,
> +	};
> +
> +	switch (op) {
> +	case CLA_LAUNCH_OP_REGREAD:
> +	case CLA_LAUNCH_OP_REGWRITE:
> +		max_regidx = 0x100000000;
> +		break;
> +	default:
> +		WARN_ON(1);
> +		return -EINVAL;
> +	}
> +
> +	if (WARN_ON(regidx + nregs > max_regidx))

[Severity: High]
Does this check adequately protect against integer overflow? If nregs is very
large (for example, (size_t)-1), the addition of regidx and nregs can overflow
size_t. This would result in a small positive value that bypasses the
max_regidx limit.

Could this be rewritten to avoid wrap-around, perhaps by checking if nregs
is greater than max_regidx - regidx?

> +		return -EINVAL;
> +
> +	/* 1 to 8 registers accessed at a time, within the same 8-reg group */
> +	while (nregs > 0) {
> +		unsigned int reg_group = ALIGN_DOWN(regidx, 8);
> +		unsigned int max_reg = min(regidx + nregs, reg_group + 8);

[Severity: High]
Can reg_group + 8 overflow? Since regidx and reg_group are 32-bit unsigned
integers, if a caller passes a register index near the 32-bit maximum
(for example, 0xFFFFFFF8) and a small nregs value, reg_group + 8 will wrap
around to 0.

If max_reg evaluates to 0, ndata becomes 0 - regidx, which can yield a value
larger than the original nregs. Will this cause out-of-bounds memory accesses
on the regs buffer when passed to cla_op_launch(), and an infinite loop due to
underflowing nregs?

> +		unsigned int ndata = max_reg - regidx;
> +
> +		launch.ndata_m1 = ndata - 1;
> +		launch.regidx = regidx;
> +		launch.data = regs;
> +
> +		ret = cla_op_launch(dev, &launch);
> +		if (ret)
> +			break;
> +
> +		regidx += ndata;
> +		regs += ndata;
> +		nregs -= ndata;
> +	}
> +
> +	return ret;
> +}

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