Re: [PATCH v2 10/19] accel: ethosu: Validate all feature map tiles

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Incomplete bounds validation for multi-tile feature maps allows NPU out-of-bounds memory access (stride overflow).
--

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

accel: ethosu: Validate all feature map tiles

The commit modifies the command-stream validator to check the final
coordinate of every tile touched by an operation, rather than just the
final feature-map coordinate. It also updates the treatment of U65 feature
maps to be checked as 2x2 tiled.

> diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
> index c913c95e48ae0..c54496fa08f4c 100644
> --- a/drivers/accel/ethosu/ethosu_gem.c
> +++ b/drivers/accel/ethosu/ethosu_gem.c
[ ... ]
> @@ -423,6 +423,74 @@ static u64 feat_matrix_length(struct ethosu_device *edev,
[ ... ]
> +static int feat_matrix_size(struct ethosu_device *edev,
> +			    struct ethosu_validated_cmdstream_info *info,
> +			    struct cmd_state *st, struct feat_matrix *fm,
> +			    enum feat_matrix_type type,
> +			    u32 x, u32 y, u32 c, bool ofm, u64 *max_len)
> +{
> +	u32 storage = ethosu_is_u65(edev) ? 0 : fm->precision >> 14;
> +	int ret;
> +
> +	*max_len = 0;
> +
> +	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);

[Severity: Critical]
This isn't a bug introduced by this patch, but it appears the validation
logic might still be incomplete for intermediate tiles. In feat_matrix_size(),
when checking storage == 0, the loop evaluates to 0 (the relative minimum
coordinate) for intermediate tiles rather than checking the tile's maximum
extent. 

Because feat_matrix_length() calculates offset by multiplying strides by the
passed coordinates, passing 0 skips validating the maximum memory offset
for tile 0 (which should be evaluated at fm->width0 and fm->height[0]).
Could an ioctl command stream from unprivileged userspace, carrying a valid
base address but artificially large strides, bypass bounds checking here and
cause the NPU to access out-of-bounds memory?

> +				if (ret)
> +					return ret;
> +			}
> +		}
> +		return 0;
> +	}
> +
> +	if (storage == 1) {
> +		ret = feat_matrix_check_location(edev, info, st, fm, type, x, 0, c,
> +						 ofm, max_len);
> +		if (ret)
> +			return ret;
> +		if (fm->height[0] < fm->height[1] && fm->height[1] <= y) {
> +			ret = feat_matrix_check_location(edev, info, st, fm, type, x,
> +							 fm->height[1], c, ofm,
> +							 max_len);

[Severity: Critical]
This is also a pre-existing issue, but similar to the loop above, does this
storage == 1 logic skip validating the maximum memory bound of tile 0?

The code explicitly checks the boundaries at y = 0 and y = fm->height[1], but
it seems to skip verifying the maximum extent of tile 0 at y = fm->height[0].
Would this allow unprivileged userspace to trigger out-of-bounds accesses
during intermediate tile processing if the vertical stride is maliciously
large?

> +			if (ret)
> +				return ret;
> +		}
[ ... ]

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