Re: [PATCH v4 26/31] accel: ethosu: Split U65 and U85 DMA length validation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] accel: ethosu: missing size0/size1 scaling for 1D side in mixed-mode DMA
--

commit 186c2e0ed128e4d62938b05418f963aa19e82266
Author: Rob Herring (Arm) <robh@kernel.org>

accel: ethosu: Split U65 and U85 DMA length validation

This commit splits DMA length validation in the Ethos-U driver into separate
functions for U65 and U85 to account for their different stride and skip
models. It also updates the command stream validator to populate shared U65
strides and sign-extended U85 strides for source and destination DMA.

> diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
> index 117046e5a61f8..04c0de8336cd1 100644
> --- a/drivers/accel/ethosu/ethosu_gem.c
> +++ b/drivers/accel/ethosu/ethosu_gem.c
> @@ -245,40 +245,97 @@ static bool dma_params_valid(struct ethosu_device *edev, struct cmd_state *st,
>  	return true;
>  }
>  
> -static u64 dma_length(struct ethosu_device *edev,
> -		      struct ethosu_validated_cmdstream_info *info,
> -		      struct cmd_state *st, struct dma_state *dma_st,
> -		      struct dma *dma, u16 region_cmd, u16 addr_cmd)
> +static u64 dma_length_finish(struct ethosu_validated_cmdstream_info *info,
> +			     const struct dma *dma, u64 len)
> +{
> +	if (dma->region >= 0) {
> +		u64 end;
> +
> +		if (check_add_overflow(len, dma->offset, &end))
> +			return U64_MAX;
> +		info->region_size[dma->region] =
> +			max(info->region_size[dma->region], end);
> +	}
> +
> +	return len;
> +}
> +
> +static u64 dma_length_u65(struct ethosu_validated_cmdstream_info *info,
> +			  struct dma_state *dma_st,
> +			  struct dma *dma)
>  {
>  	s8 mode = dma->mode;
>  	u64 len = dma->len;
> +	u64 stride;
>  
> -	if (!dma_params_valid(edev, st, dma_st, dma, region_cmd, addr_cmd))
> -		return U64_MAX;
> +	if (mode >= 1) {
> +		if (check_mul_overflow(len, (u64)dma_st->size0, &len) ||
> +		    check_mul_overflow((u64)dma->stride[0],
> +				       (u64)dma_st->size0 - 1, &stride) ||
> +		    check_add_overflow(len, stride, &len))
> +			return U64_MAX;
> +	}
> +	if (mode == 2) {
> +		if (check_mul_overflow(len, (u64)dma_st->size1, &len) ||
> +		    check_mul_overflow((u64)dma->stride[1],
> +				       (u64)dma_st->size1 - 1, &stride) ||
> +		    check_add_overflow(len, stride, &len))
> +			return U64_MAX;
> +	}
> +
> +	return dma_length_finish(info, dma, len);
> +}
> +
> +static u64 dma_length_u85(struct ethosu_validated_cmdstream_info *info,
> +			  struct dma_state *dma_st,
> +			  struct dma *dma)
> +{
> +	s8 mode = dma->mode;
> +	s64 min = 0;
> +	u64 max = dma->len;
> +	s64 stride;
>  
>  	if (mode >= 1) {
> -		if (dma->stride[0] < 0 && (u64)(-dma->stride[0]) > len)
> +		if (check_mul_overflow(dma->stride[0],
> +				       (s64)dma_st->size0 - 1, &stride))
>  			return U64_MAX;
> -		len += dma->stride[0];
> -		if (check_mul_overflow(len, (u64)dma_st->size0, &len))
> +		if (stride < 0) {
> +			if (check_add_overflow(min, stride, &min))
> +				return U64_MAX;
> +		} else if (check_add_overflow(max, (u64)stride, &max)) {
>  			return U64_MAX;
> +		}
>  	}
>  	if (mode == 2) {
> -		if (dma->stride[1] < 0 && (u64)(-dma->stride[1]) > len)
> +		if (check_mul_overflow(dma->stride[1],
> +				       (s64)dma_st->size1 - 1, &stride))
>  			return U64_MAX;
> -		len += dma->stride[1];
> -		if (check_mul_overflow(len, (u64)dma_st->size1, &len))
> +		if (stride < 0) {
> +			if (check_add_overflow(min, stride, &min))
> +				return U64_MAX;
> +		} else if (check_add_overflow(max, (u64)stride, &max)) {
>  			return U64_MAX;
> +		}
>  	}
> -	if (dma->region >= 0) {
> -		u64 end;
>  
> -		if (check_add_overflow(len, dma->offset, &end))
> -			return U64_MAX;
> -		info->region_size[dma->region] = max(info->region_size[dma->region], end);
> -	}
> +	if (min < 0 && -(u64)min > dma->offset)
> +		return U64_MAX;
>  
> -	return len;
> +	return dma_length_finish(info, dma, max);
> +}

[Severity: High]
When ethosu_gem_cmdstream_copy_and_validate() handles NPU_OP_DMA_START in
drivers/accel/ethosu/ethosu_gem.c, it validates the source and destination
endpoints independently via dma_length():

		case NPU_OP_DMA_START:
			srclen = dma_length(edev, info, &st, &st.dma, &st.dma.src,
					    NPU_SET_DMA0_SRC_REGION, NPU_SET_DMA0_SRC);
			dstlen = dma_length(edev, info, &st, &st.dma, &st.dma.dst,
					    NPU_SET_DMA0_DST_REGION, NPU_SET_DMA0_DST);

What happens when a DMA operation uses mixed modes between source and
destination, such as copying between a 1D contiguous buffer (mode == 0) and
a 2D or 3D strided buffer (mode == 1 or 2)?

On U65 in particular, NPU_SET_DMA0_SRC_STRIDE0 and NPU_SET_DMA0_SRC_STRIDE1
are shared between source and destination, so copying between a contiguous
buffer and a strided buffer requires one side to use mode == 0 while the
other uses mode == 1 or 2.

Both dma_length_u65() and dma_length_u85() only inspect dma->mode of the
endpoint currently being evaluated. When dma->mode == 0 while the other
endpoint has mode >= 1, the 1D endpoint's length is left as dma->len (a
single 1D transfer) rather than being scaled by dma_st->size0 (and
dma_st->size1 for 3D) to cover all transfers performed by the DMA engine.

Could this under-compute info->region_size[dma->region] in
dma_length_finish() for the 1D endpoint and allow out-of-bounds DMA reads or
writes past the end of the backing GEM buffer object?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-ethosu-fixes-v4-0-4aa25e22578e@kernel.org?part=26