Re: [PATCH] accel: ethosu: Don't read the U65 rounding mode as a storage mode

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing default error case in the feature matrix format switch allows out-of-bounds DMA.
--

--- Patch [1]: [PATCH] accel: ethosu: Don't read the U65 rounding mode as a storage mode ---
commit 1513ddc6b4e0eb1db33e857fa9c9f18ec8c80157
Author: Tomeu Vizoso <[email protected]>

accel: ethosu: Don't read the U65 rounding mode as a storage mode

This commit modifies the ethosu driver to stop reading bits 15:14 of
the precision field as a storage mode on U65 NPUs. On U65, this field
holds the rounding mode instead, so the storage mode is now forced to 0
for these devices to prevent command stream parsing failures.

> diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
> index d50fed64d4d93..fa37a190e9fff 100644
> --- a/drivers/accel/ethosu/ethosu_gem.c
> +++ b/drivers/accel/ethosu/ethosu_gem.c
> @@ -204,7 +204,7 @@ static u64 feat_matrix_length(struct ethosu_device *edev,
>  			      struct feat_matrix *fm,
>  			      u32 x, u32 y, u32 c, bool ofm)
>  {
> -	u32 element_size, storage = fm->precision >> 14;
> +	u32 element_size, storage = ethosu_is_u65(edev) ? 0 : fm->precision >> 14;

[Severity: High]
This is a pre-existing issue, but by forcing storage to 0 on U65, does this
inadvertently allow uninitialized precision values to bypass storage
validation?

If a command stream omits the NPU_SET_IFM_PRECISION command, fm->precision is
initialized to 0xffff. Because storage is now forced to 0, this bypasses the
earlier switch (storage) check which would have rejected it as 3.

Later in feat_matrix_length(), there is a switch for the format:

	switch ((fm->precision >> 6) & 0x3) { // format
	case 0: //nhwc:
		element_size = BIT((fm->precision >> (ofm ? 1 : 2)) & 0x3);
		addr += x * fm->stride_x + c * element_size;
		break;
	case 1: //nhcwb16:
		element_size = BIT((fm->precision >> (ofm ? 1 : 2)) & 0x3);

		addr += (c / 16) * fm->stride_c + (16 * x + (c & 0xf)) * element_size;
		break;
	}

Because there is no default error case, formats 2 and 3 fall through without
adding the width and depth (x and c) components to the required length.

Does this result in an under-calculated buffer length that bypasses the
driver's GEM size validation and could allow out-of-bounds DMA accesses?

>  	int tile = 0;
>  	u64 addr;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1