Re: [PATCH v4 28/31] accel: ethosu: Validate resize operations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] accel: ethosu: off-by-one in OFM block size for resize_axis_size()
- [High] accel: ethosu: off-by-one IFM coordinate bound in resize_axis_size()
--

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

accel: ethosu: Validate resize operations

This commit adds command stream validation for Ethos-U85 NPU_OP_RESIZE
operations by tracking the resize scale, offset, step, and OFM block size
registers and validating the input feature map bounds.

> diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
> index fe6bf1a2f8cb2..0b227664afc7b 100644
> --- a/drivers/accel/ethosu/ethosu_gem.c
> +++ b/drivers/accel/ethosu/ethosu_gem.c

[ ... ]

> @@ -663,6 +675,98 @@ calc_acc_input_size(struct drm_device *ddev,
>  	return ret;
>  }
>  
> +static int resize_axis_size(struct cmd_state *st, int axis, u16 ofm_size,
> +			    u16 ofm_blk_size, u32 *size)
> +{
> +	struct resize_axis *resize = &st->resize[axis];
> +	u64 one_step, blk_step, coord;
> +
> +	if (resize->offset < -(s16)resize->scale_n ||
> +	    resize->offset >= resize->scale_n ||
> +	    resize->one_step_mod >= resize->scale_n ||
> +	    resize->blk_step_mod >= resize->scale_n)
> +		return -EINVAL;
> +
> +	one_step = resize->one_step_int * resize->scale_n +
> +		resize->one_step_mod;
> +	blk_step = resize->blk_step_int * resize->scale_n +
> +		resize->blk_step_mod;
> +	if (check_mul_overflow((u64)ofm_blk_size, one_step, &coord) ||
> +	    blk_step != coord)
> +		return -EINVAL;
> +
> +	if (check_mul_overflow((u64)ofm_size, one_step, &coord) ||
> +	    check_add_overflow(coord, (u64)resize->scale_n - 1, &coord))
> +		return -EINVAL;
> +
> +	coord = div_u64(coord, resize->scale_n);
> +	if (coord >= U32_MAX)
> +		return -EINVAL;
> +
> +	*size = coord + 1;
> +	return 0;
> +}
> +
> +
> +static int calc_sizes_resize(struct drm_device *ddev,
> +			     struct ethosu_validated_cmdstream_info *info,
> +			     struct cmd_state *st)
> +{
> +	struct ethosu_device *edev = to_ethosu_device(ddev);
> +	u32 ifm_width, ifm_height;
> +	u64 len;
> +	int ret;
> +
> +	if (!cmd_state_reg_is_set(st, NPU_SET_KERNEL_WIDTH_M1) ||
> +	    !cmd_state_reg_is_set(st, NPU_SET_KERNEL_HEIGHT_M1) ||
> +	    !cmd_state_reg_is_set(st, NPU_SET_OFM_BLK_WIDTH_M1) ||
> +	    !cmd_state_reg_is_set(st, NPU_SET_OFM_BLK_HEIGHT_M1) ||
> +	    !cmd_state_reg_is_set(st, NPU_SET_RESIZE_X_SCALE_N_M1) ||
> +	    !cmd_state_reg_is_set(st, NPU_SET_RESIZE_Y_SCALE_N_M1) ||
> +	    !cmd_state_reg_is_set(st, NPU_SET_RESIZE_X_OFFSET) ||
> +	    !cmd_state_reg_is_set(st, NPU_SET_RESIZE_Y_OFFSET) ||
> +	    !cmd_state_reg_is_set(st, NPU_SET_RESIZE_X) ||
> +	    !cmd_state_reg_is_set(st, NPU_SET_RESIZE_Y))
> +		return -EINVAL;
> +
> +	ret = resize_axis_size(st, 0, st->ofm.width, st->ofm_blk_width,
> +			       &ifm_width);
> +	if (ret)
> +		return ret;
> +	ret = resize_axis_size(st, 1, st->ofm.height[2], st->ofm_blk_height,
> +			       &ifm_height);
> +	if (ret)
> +		return ret;
> +
> +	if (check_add_overflow(ifm_width, (u32)st->ifm.width, &ifm_width) ||
> +	    check_add_overflow(ifm_height, (u32)st->ifm.height[2], &ifm_height))
> +		return -EINVAL;
> +
> +	ret = feat_matrix_size(edev, info, st, &st->ifm, FEAT_MATRIX_IFM,
> +			       ifm_width, ifm_height, st->ifm.depth, false, &len);

[Severity: High]
Does this overestimate ifm_width and ifm_height when passing them to
feat_matrix_size()?

In resize_axis_size(), ofm_size is already the 0-based maximum OFM coordinate
(st->ofm.width or st->ofm.height[2]), so coord after dividing by
resize->scale_n is a 0-based IFM coordinate. Setting *size = coord + 1
converts it to a 1-based size, and calc_sizes_resize() then adds st->ifm.width
(NPU_SET_KERNEL_WIDTH_M1) and st->ifm.height[2] (NPU_SET_KERNEL_HEIGHT_M1) on
top of that. In addition, resize_axis_size() adds resize->scale_n - 1 rather
than accounting for resize->offset, which can increase coord by another 1 when
resize->offset < resize->scale_n - 1.

However, feat_matrix_size() -> feat_matrix_check_location() ->
feat_matrix_length() expects 0-based maximum coordinates (x, y, c), matching
how st->ofm.width, st->ofm.height[2], and st->ifm.depth are passed:

drivers/accel/ethosu/ethosu_gem.c:feat_matrix_length() {
	...
	switch (storage) {
	case 0:
		if (x >= fm->width0 + 1) {
			x -= fm->width0 + 1;
			tile += 1;
		}
		if (y >= fm->height[tile] + 1) {
			y -= fm->height[tile] + 1;
			tile += 2;
		}
		break;
	...
}

When ifm_width or ifm_height exceeds the actual 0-based maximum coordinate by
1 or 2 on a single-tile IFM (where fm->width0 and fm->height[0] are set to the
IFM dimensions minus 1 and only NPU_SET_IFM_BASE0 is programmed), wouldn't
feat_matrix_length() advance to tiles 1..3 and fail validation with -EINVAL,
or require a larger IFM buffer than the tensor size?

[ ... ]

> @@ -1069,6 +1178,12 @@ static int ethosu_gem_cmdstream_copy_and_validate(struct drm_device *ddev,
>  			}
>  			st.ofm.precision = param;
>  			break;
> +		case NPU_SET_OFM_BLK_WIDTH_M1:
> +			st.ofm_blk_width = param & 0x7f;
> +			break;
> +		case NPU_SET_OFM_BLK_HEIGHT_M1:
> +			st.ofm_blk_height = param & 0x7f;
> +			break;

[Severity: High]
Should st.ofm_blk_width and st.ofm_blk_height store (param & 0x7f) + 1 (or
should resize_axis_size() add 1 to ofm_blk_size)?

NPU_SET_OFM_BLK_WIDTH_M1 and NPU_SET_OFM_BLK_HEIGHT_M1 encode the block
dimensions minus 1, similar to NPU_SET_RESIZE_X_SCALE_N_M1 and
NPU_SET_RESIZE_Y_SCALE_N_M1 where param + 1 is stored.

In ethosu_gem_cmdstream_copy_and_validate() -> calc_sizes_resize() ->
resize_axis_size(), ofm_blk_size is multiplied directly by one_step:

	if (check_mul_overflow((u64)ofm_blk_size, one_step, &coord) ||
	    blk_step != coord)
		return -EINVAL;

With the unadjusted _M1 value, resize_axis_size() checks that blk_step equals
(block_size - 1) * one_step instead of block_size * one_step, which rejects
valid NPU_OP_RESIZE command streams with -EINVAL whenever one_step is
non-zero.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-ethosu-fixes-v4-0-4aa25e22578e@kernel.org?part=28