Re: [PATCH v2 18/19] accel: ethosu: Validate OFM transpose

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] feat_matrix_size completely fails to evaluate intermediate tile boundaries, allowing arbitrary out-of-bounds DMA access
- [High] feat_matrix_length assumes monotonic address growth for nhcwb16 format, underestimating buffer bounds by up to 60 bytes
--

commit 5cfa238ea5bc9f37e1c0be661f6a282b416aafb4
Author: Rob Herring (Arm) <robh@kernel.org>
Subject: accel: ethosu: Validate OFM transpose

U85 OFM dimensions are specified before transposition, while
tile bases and strides address the transposed feature map. Permute
the output endpoint before validating its tile and stride accesses.

> diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
> index 559fbf55f12d5..8114447891b25 100644
> --- a/drivers/accel/ethosu/ethosu_gem.c
> +++ b/drivers/accel/ethosu/ethosu_gem.c
> @@ -397,6 +397,52 @@ static int feat_matrix_validate(struct ethosu_device *edev,
>  
>  	return 0;
>  }
> +
> +static int feat_matrix_permute(struct ethosu_device *edev,
> +			       struct feat_matrix *fm, u32 *x, u32 *y,
> +			       u32 *c, bool ofm)
> +{

[ ... ]

> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
>  static u64 feat_matrix_length(struct ethosu_device *edev,
>  			      struct ethosu_validated_cmdstream_info *info,
>  			      struct cmd_state *st, struct feat_matrix *fm,

[Severity: High]
This isn't a bug introduced by this patch, but feat_matrix_length() assumes
monotonic address growth for the nhcwb16 format, underestimating buffer
bounds by up to 60 bytes.

For the nhcwb16 format, the address offset calculation:

    check_mul_overflow(c / 16, (u64)fm->stride_c, &offset) ||
    ...
    check_mul_overflow(16 * x + (c & 0xf), element_size, &offset)

is not monotonic with respect to c if stride_c is smaller than
15 * element_size (which userspace fully controls).

Because the validator only checks the offset at the maximum depth,
malicious userspace can set stride_c to 0 and c_max to 16. It then
evaluates only at c=16, which yields 16 * x * element_size.

However, the hardware will also access c=15, resulting in a larger offset
of 16 * x * element_size + 15 * element_size. This allows out-of-bounds DMA
access into adjacent kernel memory.

Can this logic be updated to evaluate the maximum offset accurately by
accounting for intermediate values of c?

> @@ -502,6 +548,9 @@ static int feat_matrix_size(struct ethosu_device *edev,
>  	int ret;
>  
>  	*max_len = 0;
> +	ret = feat_matrix_permute(edev, fm, &x, &y, &c, ofm);
> +	if (ret)
> +		return ret;
>  
>  	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 is a pre-existing issue, but feat_matrix_size() completely fails to
evaluate intermediate tile boundaries, allowing arbitrary out-of-bounds DMA
access.

For tiled matrices (storage == 0 or storage == 1), each tile has an
independent base address. The maximum memory offset within tile 0 occurs
at x = min(x, fm->width0) and y = min(y, fm->height[0]).

The validator never evaluates these intermediate maxima:

For storage == 0, it only evaluates x at 0 and max_x in the loop:

    for (int xi = 0; xi < 2; xi++) {
        for (int yi = 0; yi < 2; yi++) {
            ret = feat_matrix_check_location(..., xi ? x : 0, yi ? y : 0, ...);

This misses the local maximum x offset inside tile 0 (fm->width0).

For storage == 1, it evaluates y at 0 and fm->height[1]:

    if (fm->height[0] < fm->height[1] && fm->height[1] <= y) {
        ret = feat_matrix_check_location(..., x, fm->height[1], ...);

This completely skips the end of tile 0 (fm->height[0]), leaving the max
offset for tile 0 unvalidated.

Userspace can bypass validation by setting base[0] near the end of the
buffer while base[0] + offset(x, fm->height[0]) massively overflows it.

Could this be corrected to evaluate the local maxima for tile boundaries?

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