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

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Incomplete U65 storage encoding override causes inconsistent tile address calculation and broken validation.

Pre-existing issues:
- [Critical] Insufficient bounds checking of intermediate feature map tiles due to evaluating start coordinates instead of maximum accessed coordinates.
--

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

accel: ethosu: Validate all feature map tiles

This commit updates the command-stream validator to check the final 
coordinate of every tile touched by an operation instead of just the 
final feature-map coordinate. It also treats U65 feature maps as 2x2 
tiled.

> diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
> index 2aafbfe95a8c2..a042e650f626a 100644
> --- a/drivers/accel/ethosu/ethosu_gem.c
> +++ b/drivers/accel/ethosu/ethosu_gem.c
[ ... ]
> @@ -259,6 +259,72 @@ static u64 feat_matrix_length(struct ethosu_device *edev,
>  	return addr;
>  }
>  
> +static int feat_matrix_check_location(struct ethosu_device *edev,
[ ... ]
> +static int feat_matrix_size(struct ethosu_device *edev,
> +			    struct ethosu_validated_cmdstream_info *info,
> +			    struct feat_matrix *fm,
> +			    u32 x, u32 y, u32 c, bool ofm, u64 *max_len)
> +{
> +	u32 storage = ethosu_is_u65(edev) ? 0 : fm->precision >> 14;

[Severity: Critical]
Does this U65 storage encoding override create an inconsistency with how
feat_matrix_length() calculates the storage?

In feat_matrix_size(), storage is forced to 0 for U65. However, looking at
feat_matrix_length(), it still decodes storage using the precision bits:

static u64 feat_matrix_length(...)
{
        u32 element_size, storage = fm->precision >> 14;
        ...
}

Could a manipulated command stream use this discrepancy to pass validation on
one tile (using storage = 0) while the hardware accesses a different,
unvalidated tile base address (using storage = 1)?

> +	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, fm,
> +								 xi ? x : 0,
> +								 yi ? y : 0, c, ofm,
> +								 max_len);

[Severity: Critical]
This is a pre-existing issue, but does this loop evaluate the absolute
coordinate 0 instead of the maximum valid coordinate for the intermediate
tile?

By checking (0, y) and (x, y), it appears we skip the maximum local
coordinate accessed in the intermediate tile (such as fm->width0). Could
this allow a command stream with an inflated stride to pass validation
here but cause the hardware to perform an out-of-bounds DMA access?

> +				if (ret)
> +					return ret;
> +			}
> +		}
> +		return 0;
> +	}
> +
> +	if (storage == 1) {
> +		ret = feat_matrix_check_location(edev, info, fm, x, 0, c,
> +						 ofm, max_len);

[Severity: Critical]
This is a pre-existing issue, but similar to the 2x2 case above, is tile 0
checked at y=0 instead of its maximum accessed extent (fm->height[0])?

This might leave its size bounds not fully validated.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827-ethosu-fixes-v1-0-346f9ea8791c@kernel.org?part=10