Re: [PATCH v2 11/19] accel: ethosu: Account for feature map element size

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Incomplete bounds validation for multi-tile feature maps in `feat_matrix_size` allows the NPU to access memory outside the GEM buffer.
- [High] The `feat_matrix_length` function does not account for the 16-channel hardware brick alignment for NHCWB16 formats, underestimating the buffer size.
--

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

accel: ethosu: Account for feature map element size

The feature map bounds calculation currently accounts for only one byte
of the final element. This can leave the tail of B16, B32, and B64
elements outside the validated buffer, so the commit uses the feature map
element size when calculating the final byte.

> diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
> index c54496fa08f4c..bca68fab05275 100644
> --- a/drivers/accel/ethosu/ethosu_gem.c
> +++ b/drivers/accel/ethosu/ethosu_gem.c
> @@ -415,7 +415,7 @@ static u64 feat_matrix_length(struct ethosu_device *edev,
>  		return U64_MAX;
>  	}
>  
> -	if (check_add_overflow(addr, 1ULL, &offset))
> +	if (check_add_overflow(addr, (u64)element_size, &offset))

[Severity: High]
This isn't a bug introduced by this patch, but while the patch adds a missing
single element_size to the calculation, is it sufficient for NHCWB16 formats?

Looking slightly above in feat_matrix_length():

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

		if (check_mul_overflow(c / 16, (u64)fm->stride_c, &offset) ||
		    check_add_overflow(addr, offset, &addr) ||
		    check_mul_overflow(16 * x + (c & 0xf), element_size, &offset) ||
		    check_add_overflow(addr, offset, &addr))
			return U64_MAX;
		break;

The Ethos-U NPU inherently processes NHCWB16 data in 16-channel bricks. If
the channel depth modulo 16 is less than 15, the hardware will still read
or write the full 16-element brick.

Because the validation calculation stops precisely after the specified channel
and doesn't appear to round up the offset to the 16-element brick boundary,
could it underestimate the required buffer size by up to 15 * element_size
bytes, leading to out-of-bounds hardware memory accesses?

>  		return U64_MAX;
>  
>  	info->region_size[fm->region] = max(info->region_size[fm->region], offset);


[Severity: Critical]
This is a pre-existing issue, but incomplete bounds validation for multi-tile
feature maps in feat_matrix_size() might allow the NPU to access memory
outside the GEM buffer.

In drivers/accel/ethosu/ethosu_gem.c:feat_matrix_size():

	if (ethosu_is_u65(edev) || storage == 0) {
		for (int xi = 0; xi < 2; xi++) {
			for (int yi = 0; yi < 2; yi++) {
				ret = feat_matrix_check_location(edev, info, st, fm, type,
								 xi ? x : 0,
								 yi ? y : 0, c, ofm,
								 max_len);
				if (ret)
					return ret;
			}
		}
		return 0;
	}

The loop evaluates the minimum and global maximum coordinates but skips the
internal boundary coordinates (like fm->width0) of intermediate tiles.

Similarly, for vertically split tiles (storage == 1):

	if (storage == 1) {
		ret = feat_matrix_check_location(edev, info, st, fm, type, x, 0, c,
						 ofm, max_len);

The maximum Y coordinate within the first tile (fm->height[0]) is completely
omitted from bounds validation.

If a multi-tile feature map is submitted, and tile 0's base address is
configured near the end of the GEM allocation, with a massive internal
width0 and stride_x for tile 0 (while properly bounding subsequent tiles
so the global maximum falls into tile 1), could this artificially bypass
validation for the rest of tile 0 and allow unconstrained memory accesses?

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