Re: [PATCH v4 27/31] accel: ethosu: Validate OFM transpose

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] accel: ethosu: missing NHCWB16 format rejection when OFM transpose is enabled
--

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

accel: ethosu: Validate OFM transpose

This commit adds validation and dimension permutation for Ethos-U85 output
feature map (OFM) transposition in the command stream validator.

> diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
> index 04c0de8336cd1..fe6bf1a2f8cb2 100644
> --- a/drivers/accel/ethosu/ethosu_gem.c
> +++ b/drivers/accel/ethosu/ethosu_gem.c

[ ... ]

> @@ -1000,8 +1054,19 @@ static int ethosu_gem_cmdstream_copy_and_validate(struct drm_device *ddev,
>  		case NPU_SET_OFM_PRECISION:
>  			if (((param >> 6) & 0x3) > 1)
>  				return -EINVAL;
> -			if (!ethosu_is_u65(edev) && (param & GENMASK(13, 11)))
> -				return -EINVAL;
> +			if (!ethosu_is_u65(edev)) {
> +				u32 transpose = FIELD_GET(NPU_OFM_TRANSPOSE_MASK, param);
> +
> +				switch (transpose) {
> +				case 4:
> +				case 5:
> +					return -EINVAL;
> +				}
> +				if (transpose && ((param >> 1) & 0x3) == 3)
> +					return -EINVAL;
> +				if ((param >> 14) == 2 && transpose)
> +					return -EINVAL;
> +			}
>  			st.ofm.precision = param;
>  			break;

[Severity: Critical]
Should ethosu_gem_cmdstream_copy_and_validate() also reject non-zero
transpose when the activation format in NPU_SET_OFM_PRECISION is NHCWB16
(((param >> 6) & 0x3) == 1)?

On Ethos-U85, OFM transposition is only valid with the linear NHWC format.
If a command stream specifies both NHCWB16 (((param >> 6) & 0x3) == 1) and
a non-zero transpose in NPU_SET_OFM_PRECISION:

1. feat_matrix_size() -> feat_matrix_permute() permutes x, y, and c into
   the transposed coordinates.

2. feat_matrix_length() -> feat_matrix_validate() checks:

	format = (fm->precision >> 6) & 0x3;
	stride_cmd = feat_matrix_base_cmd(type) + (format ? 6 : 4);
	if (!cmd_state_reg_is_set(st, stride_cmd))
		return -EINVAL;

   which only requires NPU_SET_OFM_STRIDE_C to be set when format is 1,
   leaving NPU_SET_OFM_STRIDE_X unvalidated.

3. feat_matrix_length() then computes the region size using the NHCWB16
   brick address calculation on the already-permuted x and c coordinates:

	case 1: //nhcwb16:
		element_size = BIT((fm->precision >> (ofm ? 1 : 2)) & 0x3);

		/* NHCWB16 accesses the complete final channel brick. */
		if (check_mul_overflow(c / 16, (u64)fm->stride_c, &offset) ||
		    check_add_overflow(addr, offset, &addr) ||
		    check_mul_overflow(16 * x + 15, element_size, &offset) ||
		    check_add_overflow(addr, offset, &addr))
			return U64_MAX;
		break;

Can this cause feat_matrix_length() to underestimate
info->region_size[fm->region] (for example, by permuting a large width into
c while fm->stride_c is 0) and allow out-of-bounds DMA writes past the end
of the GEM buffer?

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