Re: [PATCH v2 06/19] accel: ethosu: Track command stream register setup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] dma_params_valid() incorrectly returns U64_MAX in a boolean function, which evaluates to true and bypasses missing DMA parameter validation.
--

commit 6f2462930489d04d8fd80b48b9dfe3672578205e
Author: Rob Herring (Arm) <robh@kernel.org>

accel: ethosu: Track command stream register setup

The current method of tracking command stream state initializing state
tracking to illegal values and assuming unaccessed registers are 0 is
proving inadequate with additional validation. Instead, track all the
registers in a bitmap as the register address space is fairly small.

> diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
> index 408b93350dd79..8a44a5d89bce6 100644
> --- a/drivers/accel/ethosu/ethosu_gem.c
> +++ b/drivers/accel/ethosu/ethosu_gem.c
[ ... ]
> @@ -158,13 +185,54 @@ static u64 cmd_to_addr(u32 *cmd)
[ ... ]
> +static bool dma_params_valid(struct ethosu_device *edev, struct cmd_state *st,
> +			     const struct dma_state *dma_st,
> +			     const struct dma *dma,
> +			     u16 region_cmd, u16 addr_cmd)
> +{
> +	s8 mode = dma->mode;
> +
> +	if (!cmd_state_reg_is_set(st, region_cmd) ||
> +	    !cmd_state_reg_is_set(st, addr_cmd) ||
> +	    !cmd_state_reg_is_set(st, NPU_SET_DMA0_LEN) || mode < 0 || mode > 2)
> +		return false;
> +
> +	if (mode >= 1 &&
> +	    !cmd_state_reg_is_set(st, dma_use_src_stride(edev, dma_st, dma) ?
> +				  NPU_SET_DMA0_SRC_STRIDE0 :
> +				  NPU_SET_DMA0_DST_STRIDE0))
> +		return U64_MAX;

[Severity: High]
Since dma_params_valid() returns a bool, does returning U64_MAX here get
converted to true?

If an attacker intentionally omits the stride command in a malicious stream,
could this evaluate as true and inadvertently bypass the validation check,
potentially leading to out-of-bounds DMA accesses using retained hardware
stride values?

> +	if (mode == 2 &&
> +	    !cmd_state_reg_is_set(st, dma_use_src_stride(edev, dma_st, dma) ?
> +				  NPU_SET_DMA0_SRC_STRIDE1 :
> +				  NPU_SET_DMA0_DST_STRIDE1))
> +		return U64_MAX;

[Severity: High]
Does returning U64_MAX here also inadvertently evaluate to true?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-ethosu-fixes-v2-0-3767738756a4@kernel.org?part=6