Re: [PATCH v7 15/28] media: rockchip: rga: align stride to 4 bytes

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The `vir_stride` bitfield in `union rga_src_vir_info` is 10 bits, causing silent truncation for valid V4L2 widths.
- [High] In `rga_open()`, the return value of `rga_setup_ctrls(ctx)` is completely ignored. If control setup fails, subsequent code uses a destroyed control handler and locks a destroyed mutex.
- [High] An off-by-one error in the bounds check of `fill_descriptors` combined with an under-allocation of the descriptor array causes a heap out-of-bounds write of 4 bytes when handling multi-planar formats.
--

commit c43e898ca5762d2604bb3d4b71e3a6c220a67ef8
Author: Sven Püschel <s.pueschel@pengutronix.de>

media: rockchip: rga: align stride to 4 bytes

Add an alignment setting to rga_hw to set the desired stride alignment.
As the RGA2 register for the stride counts in word units, the code
already divides the bytesperline value by 4 when writing it into the
register. Therefore fix the alignment to a multiple of 4 to avoid
potential off by one errors due from the division.

> diff --git a/drivers/media/platform/rockchip/rga/rga-hw.c b/drivers/media/platform/rockchip/rga/rga-hw.c
> index 9881c14f908d5..dac3cb6aa17d3 100644
> --- a/drivers/media/platform/rockchip/rga/rga-hw.c
> +++ b/drivers/media/platform/rockchip/rga/rga-hw.c
> @@ -580,6 +580,7 @@ const struct rga_hw rga2_hw = {
>  	.max_width = MAX_WIDTH,
>  	.min_height = MIN_HEIGHT,
>  	.max_height = MAX_HEIGHT,
> +	.stride_alignment = 4,

[Severity: High]
This is a pre-existing issue, but the vir_stride bitfield in
union rga_src_vir_info is 10 bits, causing silent truncation for valid V4L2
widths.

The hardware requires the source stride in 4-byte words, which the driver
calculates as ctx->in.stride >> 2:

drivers/media/platform/rockchip/rga/rga-hw.c:rga_hw_start() {
    ...
	/*
	 * SRC_VIR_INFO
	 * note that the step of vir_stride / vir_width is 4 byte words
	 */
	src_vir_info.data.vir_stride = ctx->in.stride >> 2;
    ...
}

For a 1920x1080 image in a 32-bit format (like ARGB32), the stride is 7680
bytes, which equals 1920 words. However, vir_stride is defined as a 10-bit
bitfield in union rga_src_vir_info:

drivers/media/platform/rockchip/rga/rga-hw.h:
union rga_src_vir_info {
	unsigned int val;
	struct {
		/* [0:15] */
		unsigned int vir_width:15;
		unsigned int reserved:1;
		/* [16:25] */
		unsigned int vir_stride:10;
		/* [26:31] */
		unsigned int reserved1:6;
	} data;
};

A 10-bit field can only hold values up to 1023. When 1920 is assigned to it,
it silently overflows and truncates to 896 (1920 & 1023).

This truncated stride is then programmed into the hardware via the
RGA_SRC_VIR_INFO register. Will this cause the hardware to read from
incorrect memory offsets for all lines after the first, resulting in severe
data corruption for common high resolutions like 1080p?

>  
>  	.start = rga_hw_start,
>  	.handle_irq = rga_handle_irq,
> diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c
> index bf6bbcbfc869b..d080cb672740b 100644
> --- a/drivers/media/platform/rockchip/rga/rga.c
> +++ b/drivers/media/platform/rockchip/rga/rga.c
> @@ -234,10 +234,10 @@ static int rga_open(struct file *file)
>  	ctx->in = def_frame;
>  	ctx->out = def_frame;
>  
> -	v4l2_fill_pixfmt_mp(&ctx->in.pix,
> -			    ctx->in.fmt->fourcc, def_width, def_height);
> -	v4l2_fill_pixfmt_mp(&ctx->out.pix,
> -			    ctx->out.fmt->fourcc, def_width, def_height);
> +	v4l2_fill_pixfmt_mp_aligned(&ctx->in.pix, ctx->in.fmt->fourcc,
> +				    def_width, def_height, rga->hw->stride_alignment);
> +	v4l2_fill_pixfmt_mp_aligned(&ctx->out.pix, ctx->out.fmt->fourcc,
> +				    def_width, def_height, rga->hw->stride_alignment);

[Severity: High]
This isn't a bug introduced by this patch, but in rga_open(), the return
value of rga_setup_ctrls(ctx) is completely ignored.

If rga_setup_ctrls() encounters an error (e.g., -ENOMEM during
v4l2_ctrl_handler_init), it handles its own failure by calling
v4l2_ctrl_handler_free(&ctx->ctrl_handler) before returning the error code.
This free operation invokes mutex_destroy(&ctx->ctrl_handler._lock).

However, rga_open() completely ignores the returned error code and proceeds:

drivers/media/platform/rockchip/rga/rga.c:rga_open() {
    ...
	rga_setup_ctrls(ctx);

	/* Write the default values to the ctx struct */
	v4l2_ctrl_handler_setup(&ctx->ctrl_handler);

	ctx->fh.ctrl_handler = &ctx->ctrl_handler;
	mutex_unlock(&rga->mutex);
    ...
}

It immediately calls v4l2_ctrl_handler_setup(&ctx->ctrl_handler), which
inadvertently calls mutex_lock() on the destroyed mutex. Furthermore, it
assigns ctx->fh.ctrl_handler = &ctx->ctrl_handler, exposing the broken,
destroyed handler to userspace ioctls like VIDIOC_S_CTRL.

Could this result in a reliable use-after-destroy vulnerability if a user can
induce out-of-memory conditions during device open?

[ ... ]

> @@ -393,7 +393,8 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f)
>  		fmt = &hw->formats[0];
>  
>  	v4l2_apply_frmsize_constraints(&pix_fmt->width, &pix_fmt->height, &frmsize);
> -	v4l2_fill_pixfmt_mp(pix_fmt, fmt->fourcc, pix_fmt->width, pix_fmt->height);
> +	v4l2_fill_pixfmt_mp_aligned(pix_fmt, fmt->fourcc,
> +				    pix_fmt->width, pix_fmt->height, hw->stride_alignment);

[Severity: High]
This is a pre-existing issue, but an off-by-one error in the bounds check of
fill_descriptors() combined with an under-allocation of the descriptor array
causes a heap out-of-bounds write of 4 bytes when handling multi-planar
formats.

In rga_buf_init(), the DMA descriptor array is allocated based on the total
size of the frame:

drivers/media/platform/rockchip/rga/rga-buf.c:rga_buf_init() {
    ...
	n_desc = DIV_ROUND_UP(f->size, PAGE_SIZE);

	rbuf->n_desc = n_desc;
	rbuf->dma_desc = dma_alloc_coherent(rga->dev,
					    rbuf->n_desc * sizeof(*rbuf->dma_desc),
					    &rbuf->dma_desc_pa, GFP_KERNEL);
    ...
}

However, videobuf2 allocates each plane independently and pads each plane's
allocation to a page boundary. This means the sum of pages in the scatterlist
tables across all planes can exceed the total allocated descriptors (e.g., if
two planes are 4098 bytes each, they require 2 pages each (4 total), but
f->size is 8196 bytes, so n_desc is only 3).

When rga_buf_prepare() iterates through the planes and calls
fill_descriptors(), it passes the remaining capacity as max_desc. Due to an
off-by-one error in fill_descriptors() (if (n_desc > max_desc) instead of >=),
the loop permits writing exactly one element past max_desc before returning
-EINVAL:

drivers/media/platform/rockchip/rga/rga-buf.c:fill_descriptors() {
    ...
	for_each_sgtable_dma_page(sgt, &iter, 0) {
		if (n_desc > max_desc)
			return -EINVAL;
		addr = sg_page_iter_dma_address(&iter);
		tmp->addr = lower_32_bits(addr);
		tmp++;
		n_desc++;
	}
    ...
}

If the remaining capacity is 1 but the scatterlist table provides 2 pages,
the loop writes to tmp[0] (valid) and tmp[1] (out-of-bounds). Can this result
in a reliable 4-byte heap buffer overflow?

>  	pix_fmt->field = V4L2_FIELD_NONE;
>  
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521-spu-rga3-v7-0-3f33e8c7145f@pengutronix.de?part=15