Re: [PATCH v7 22/28] media: rockchip: rga: remove size from rga_frame

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] An off-by-one error in `fill_descriptors()` combined with memory under-allocation in `rga_buf_init()` causes an exploitable heap buffer overflow.
- [High] Plane DMA base addresses ignore the scatterlist intra-page offset, leading to incorrect hardware memory access for USERPTR and DMABUF buffers.
- [High] Missing early exits after `WARN_ON()` checks in `rga_isr()` lead to NULL pointer dereferences.
--

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

media: rockchip: rga: remove size from rga_frame

This commit removes the size member from the rga_frame structure, as it is
only used for the MMU page table mapping. Instead, the driver now calculates
the size in place when needed, which also avoids the calculation entirely
when an external IOMMU is used.

> diff --git a/drivers/media/platform/rockchip/rga/rga-buf.c b/drivers/media/platform/rockchip/rga/rga-buf.c
> index cd6904d5fe5a6..092c2bdf6b672 100644
> --- a/drivers/media/platform/rockchip/rga/rga-buf.c
> +++ b/drivers/media/platform/rockchip/rga/rga-buf.c
> @@ -79,6 +79,8 @@ static int rga_buf_init(struct vb2_buffer *vb)
>  	struct rockchip_rga *rga = ctx->rga;
>  	struct rga_frame *f = rga_get_frame(ctx, vb->vb2_queue->type);
>  	size_t n_desc = 0;
> +	u32 size = 0;
> +	u8 i;
>  
>  	if (IS_ERR(f))
>  		return PTR_ERR(f);
> @@ -86,7 +88,9 @@ static int rga_buf_init(struct vb2_buffer *vb)
>  	if (!rga_has_internal_iommu(rga))
>  		return 0;
>  
> -	n_desc = DIV_ROUND_UP(f->size, PAGE_SIZE);
> +	for (i = 0; i < f->pix.num_planes; i++)
> +		size += f->pix.plane_fmt[i].sizeimage;
> +	n_desc = DIV_ROUND_UP(size, PAGE_SIZE);

[Severity: Critical]
This is a pre-existing issue, but can this memory allocation logic trigger a
heap buffer overflow when combined with fill_descriptors()?

Because multi-planar buffers have independent page alignments per plane,
the actual number of pages mapped in the SG tables can exceed the
calculation here: sum(DIV_ROUND_UP(size[i])) >= DIV_ROUND_UP(sum(size[i])).

Looking at fill_descriptors(), it checks if (n_desc > max_desc) return
-EINVAL. Since it uses > instead of >=, it permits exactly one out-of-bounds
write and then returns success.

In rga_buf_prepare(), if the first plane overflows by 1 page due to
under-allocation, fill_descriptors() silently corrupts the heap and returns
success. Then curr_desc increments past rbuf->n_desc.

For the next plane, max_desc is calculated as rbuf->n_desc - curr_desc,
which underflows to SIZE_MAX. Subsequent planes are then passed to
fill_descriptors() with max_desc = SIZE_MAX, completely disabling bounds
checks and allowing arbitrary-length out-of-bounds writes on the kernel
heap.

Could we update the allocation to calculate the number of descriptors per
plane, and fix the off-by-one error in fill_descriptors()?

>  
>  	rbuf->n_desc = n_desc;
>  	rbuf->dma_desc = dma_alloc_coherent(rga->dev,

While reviewing this, I also noticed a couple of other pre-existing issues
in the surrounding code:

[Severity: High]
This is a pre-existing issue, but does this calculation ignore the
scatterlist intra-page offset?

drivers/media/platform/rockchip/rga/rga-buf.c:rga_buf_prepare() {
[ ... ]
		if (rga_has_internal_iommu(ctx->rga)) {
			/* Create local MMU table for RGA */
			n_desc = fill_descriptors(&rbuf->dma_desc[curr_desc],
						  rbuf->n_desc - curr_desc,
						  vb2_dma_sg_plane_desc(vb, i));
			if (n_desc < 0) {
				v4l2_err(&ctx->rga->v4l2_dev,
					 "Failed to map video buffer to RGA\n");
				return n_desc;
			}
			dma_addrs[i] = curr_desc << PAGE_SHIFT;
			curr_desc += n_desc;
[ ... ]
}

When creating the local MMU table, the virtual base address for each plane
is calculated strictly as curr_desc << PAGE_SHIFT.

Buffers imported via USERPTR or DMABUF are not guaranteed to be page-aligned
and may have a non-zero intra-page offset (sgt->sgl->offset). By not adding
this offset to the DMA base address, does this instruct the hardware to start
reading/writing at the beginning of the mapped physical page rather than at
the actual start of the data?


[Severity: High]
This is a pre-existing issue, but could these missing early exits after
WARN_ON() checks lead to NULL pointer dereferences?

drivers/media/platform/rockchip/rga/rga.c:rga_isr() {
[ ... ]
		WARN_ON(!ctx);

		rga->curr = NULL;

		src = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
		dst = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);

		WARN_ON(!src);
		WARN_ON(!dst);

		v4l2_m2m_buf_copy_metadata(src, dst);
[ ... ]
}

If a spurious interrupt or a shared interrupt mismatch causes ctx to be NULL,
the WARN_ON() fires, but the code immediately proceeds to call
v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx), which forcefully dereferences the
NULL pointer.

Similarly, it checks WARN_ON(!src) and WARN_ON(!dst) but still
unconditionally attempts to access them via
v4l2_m2m_buf_copy_metadata(src, dst) immediately afterward.

Should we return or skip the rest of the handler if any of these are NULL?

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