Re: [PATCH 2/4] media: rockchip: Add JPEG decoder driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [Critical] Missing upper bound checks on dynamically parsed JPEG dimensions cause an integer overflow in sizeimage calculation, leading to severe out-of-bounds memory corruption.
- [High] The driver struct `rkjpegd_dev` is tied to the device binding lifetime rather than the character device lifetime, causing Use-After-Free on unbind if file descriptors remain open.
- [High] Unsafe teardown ordering in `rkjpegd_remove` leads to Use-After-Free in the IRQ handler.
- [High] Decoding a YUV411 JPEG frame with an odd 16-pixel block width causes a hardware out-of-bounds write.
- [High] A race condition between the IRQ handler and the watchdog timeout causes a fatal bus fault.
- [High] The driver abruptly cuts power to the hardware during system suspend if a job is actively running, leading to bus hangs.
- [High] The hardware is instructed to read past the end of the mapped stream buffer, triggering IOMMU page faults.
- [Medium] Unlocked access to `ctx->dst_fmt` and `ctx->crop` in `rkjpegd_source_change` causes data races with userspace ioctls.
- [Medium] Incorrect wrapper macro `pm_sleep_ptr()` is used for the `dev_pm_ops` structure pointer.
--

--- Patch [2]: [PATCH 2/4] media: rockchip: Add JPEG decoder driver ---
commit bbf9cc98a537ee90d3d08994ca3f13423c297391
Author: Sascha Hauer <s.hauer@pengutronix.de>
Subject: media: rockchip: Add JPEG decoder driver

Add a driver for the JPEG hardware decoder Rockchip integrates into a number
of its SoCs. Downstream it is known as the VDPU720. Exposes one V4L2 M2M
device implementing the stateful decoder interface.

> diff --git a/drivers/media/platform/rockchip/rkjpegd/rkjpegd.c b/drivers/media/platform/rockchip/rkjpegd/rkjpegd.c
> index 0000000000000..cccf365ebc5d4
> --- /dev/null
> +++ b/drivers/media/platform/rockchip/rkjpegd/rkjpegd.c

[ ... ]

> +static void rkjpegd_fill_raw_fmt(struct v4l2_pix_format_mplane *pix_mp,
> +				 u32 width, u32 height)
> +{
> +	pix_mp->pixelformat = V4L2_PIX_FMT_NV12;
> +	pix_mp->width = width;
> +	pix_mp->height = height;
> +	pix_mp->field = V4L2_FIELD_NONE;
> +	pix_mp->num_planes = 1;
> +	pix_mp->plane_fmt[0].bytesperline = width;
> +	pix_mp->plane_fmt[0].sizeimage = width * height * 3 / 2;

[Severity: Critical]
Can this size calculation overflow the 32-bit integer?

If the parsed width and height are maliciously large, bypassing maximum
resolution clamping in rkjpegd_source_change(), the resulting undersized buffer
allocation can cause a massive out-of-bounds write later in
vdpu720_fill_chroma() where size - y_size underflows.

> +	memset(pix_mp->plane_fmt[0].reserved, 0,
> +	       sizeof(pix_mp->plane_fmt[0].reserved));
> +	memset(pix_mp->reserved, 0, sizeof(pix_mp->reserved));
> +}

[ ... ]

> +static int vdpu720_fill_regs(struct rkjpegd_ctx *ctx,
> +			     const struct v4l2_jpeg_header *hdr,
> +			     dma_addr_t tbl_dma,
> +			     dma_addr_t strm_dma, u32 strm_start_byte,
> +			     u32 strm_len_blks,
> +			     dma_addr_t out_dma)
> +{
> +	struct rkjpegd_dev *jpegd = ctx->dev;

[ ... ]

> +	/*
> +	 * The picture dimensions below are taken from the bitstream while the
> +	 * strides are taken from the negotiated capture format.  A frame that
> +	 * is larger than what was negotiated would make the decoder write
> +	 * beyond the capture buffer, so refuse it rather than program the
> +	 * hardware with the two sets of numbers mixed.
> +	 */
> +	if (jpeg_width > buf_width || jpeg_height_aligned > buf_height) {
> +		dev_err_ratelimited(jpegd->dev,

[Severity: High]
Is the raw header width check sufficient here?

YUV411 has a horizontal MCU width of 32 pixels. If an odd 16-pixel width is
requested (e.g., 48), the capture buffer stride is set to 48. Because the
check compares against the unaligned raw header width instead of the
MCU-aligned width, the hardware will process complete MCU blocks up to 64
pixels, overflowing the allocated row bounds.

> +				    "JPEG %ux%u does not fit the negotiated %ux%u\n",
> +				    jpeg_width, jpeg_height_aligned,
> +				    buf_width, buf_height);
> +		return -EINVAL;
> +	}

[ ... ]

> +static int rkjpegd_vdpu720_run(struct rkjpegd_ctx *ctx)
> +{
> +	struct rkjpegd_dev *jpegd = ctx->dev;

[ ... ]

> +	/*
> +	 * The stream register must be 16-byte aligned.  Round down to the
> +	 * nearest 16-byte boundary and record the sub-block start byte.
> +	 *
> +	 * Both are taken from the start of the plane rather than from the
> +	 * payload.  data_offset is set by userspace in VIDIOC_QBUF and
> +	 * videobuf2 only rejects it when it is not smaller than bytesused,
> +	 * so it carries arbitrary low bits.  Splitting an address that
> +	 * already includes it would leave STRM_BASE unaligned by those bits
> +	 * with no way to encode them, and the hardware would start reading
> +	 * from the wrong offset.
> +	 */
> +	strm_off        = data_offset + hdr->ecs_offset;
> +	hw_strm_off     = strm_off & ~0xfU;
> +	strm_start_byte = strm_off & 0xfU;
> +	strm_len_blks   = (ALIGN(payload - hw_strm_off, 16) - 1) >> 4;

[Severity: High]
Could this padding instruct the hardware to read past the mapped payload
bounds?

By blindly rounding the read length up to the next 16-byte boundary, a
tightly packed, page-aligned buffer (like an imported dmabuf) that ends
exactly on a page boundary might cause the hardware to over-read into
unmapped memory and trigger an IOMMU page fault.

> +
> +	ret = vdpu720_fill_regs(ctx, hdr, ctx->table_base.dma,
> +				src_dma + hw_strm_off, strm_start_byte,
> +				strm_len_blks, dst_dma);

[ ... ]

> +static irqreturn_t rkjpegd_vdpu720_irq(int irq, void *dev_id)
> +{
> +	struct rkjpegd_dev *jpegd = dev_id;
> +	enum vb2_buffer_state state;
> +	u32 status, clr_mask;
> +
> +	status = rkjpegd_read(jpegd, VDPU720_REG_INT);
> +
> +	/*
> +	 * Two-phase IRQ clear.  Write back a masked subset of status bits
> +	 * before checking IRQ_RAW, which some revisions require.
> +	 */
> +	clr_mask = (~(VDPU720_IRQ_CLR_COND & status)) &
> +		    (VDPU720_IRQ_CLR_KEEP & status);
> +	rkjpegd_write(jpegd, clr_mask, VDPU720_REG_INT);
> +
> +	if (!(status & VDPU720_IRQ_RAW))
> +		return IRQ_NONE;
> +
> +	/* Fully clear IRQ */
> +	rkjpegd_write(jpegd, 0, VDPU720_REG_INT);
> +
> +	state = (status & VDPU720_ERR_MASK) ?
> +		VB2_BUF_STATE_ERROR : VB2_BUF_STATE_DONE;
> +
> +	if (status & VDPU720_DEC_ERR) {
> +		u32 mcu_pos  = rkjpegd_read(jpegd, VDPU720_REG_DBG_MCU_POS);

[Severity: High]
Would it be safer to serialize the watchdog timeout with the IRQ handler?

The watchdog and IRQ handler run concurrently without serialization. If the
watchdog completes the job first and unconditionally disables the block's
clocks, the IRQ handler executing on another CPU reading this register from an
unclocked hardware block will cause a fatal bus fault.

> +		u32 err_info = rkjpegd_read(jpegd, VDPU720_REG_DBG_ERROR);
> +
> +		dev_warn_ratelimited(jpegd->dev,

[ ... ]

> +static void rkjpegd_source_change(struct rkjpegd_ctx *ctx,
> +				  struct rkjpegd_src_buf *src_buf)
> +{
> +	u32 width, height, buf_width, buf_height;
> +
> +	if (src_buf->parsed) {
> +		width = src_buf->header.frame.width;
> +		height = src_buf->header.frame.height;
> +	} else {
> +		/*
> +		 * A frame the parser refused carries no dimensions of its own,
> +		 * and the job will hand it back with an error.  Report the
> +		 * change anyway, from the coded format userspace configured.
> +		 * V4L2_FMT_FLAG_DYN_RESOLUTION tells an application to wait
> +		 * for this event before it sets the capture queue up, so one
> +		 * that never arrives leaves it waiting for a frame that cannot
> +		 * come instead of seeing the error and giving up.
> +		 */
> +		width = ctx->src_fmt.width;
> +		height = ctx->src_fmt.height;
> +	}
> +	buf_width = ALIGN(width, RKJPEGD_RAW_STEP);
> +	buf_height = ALIGN(height, RKJPEGD_RAW_STEP);
> +
> +	if (!ctx->initial_source_change &&
> +	    ctx->dst_fmt.width == buf_width &&
> +	    ctx->dst_fmt.height == buf_height &&
> +	    ctx->crop.width == width && ctx->crop.height == height)
> +		return;
> +
> +	dev_dbg(ctx->dev->dev, "source change to %ux%u\n", width, height);
> +
> +	rkjpegd_fill_raw_fmt(&ctx->dst_fmt, buf_width, buf_height);
> +	ctx->crop.left = 0;
> +	ctx->crop.top = 0;
> +	ctx->crop.width = width;
> +	ctx->crop.height = height;

[Severity: Medium]
Are these non-atomic structure assignments safe from concurrent userspace
ioctls?

This function updates these values from a workqueue context without holding
vdev_lock. A user concurrently calling ioctls that acquire the lock and read
these structures could see a torn, inconsistent structure state.

> +
> +	v4l2_event_queue_fh(&ctx->fh, &rkjpegd_src_change_event);
> +	ctx->source_change = true;

[ ... ]

> +static int rkjpegd_v4l2_init(struct rkjpegd_dev *jpegd)
> +{
> +	int ret;
> +
> +	ret = v4l2_device_register(jpegd->dev, &jpegd->v4l2_dev);

[ ... ]

> +	jpegd->vdev.lock = &jpegd->vdev_lock;
> +	jpegd->vdev.v4l2_dev = &jpegd->v4l2_dev;
> +	jpegd->vdev.fops = &rkjpegd_fops;
> +	jpegd->vdev.release = video_device_release_empty;

[Severity: High]
Does allocating the core driver structure with devres tie its lifetime
to the device bind rather than the file descriptor?

Combined with the empty video_device release callback, if a userspace process
holds the video node open during device unbind, devres will free the structure.
Closing the descriptor later will access freed memory.

> +	jpegd->vdev.vfl_dir = VFL_DIR_M2M;
> +	jpegd->vdev.device_caps = V4L2_CAP_STREAMING |
> +				  V4L2_CAP_VIDEO_M2M_MPLANE;
> +	jpegd->vdev.ioctl_ops = &rkjpegd_ioctl_ops;

[ ... ]

> +static void rkjpegd_remove(struct platform_device *pdev)
> +{
> +	struct rkjpegd_dev *jpegd = platform_get_drvdata(pdev);
> +
> +	cancel_delayed_work_sync(&jpegd->watchdog_work);
> +	rkjpegd_v4l2_cleanup(jpegd);

[Severity: High]
Is the teardown ordering here safe against active interrupts?

The M2M device is completely freed by rkjpegd_v4l2_cleanup() before the
hardware or IRQ is disabled. If an active hardware job finishes during or
after this unbind sequence, the IRQ handler will access the freed M2M context,
resulting in a use-after-free.

> +	reset_control_assert(jpegd->resets);
> +	pm_runtime_dont_use_autosuspend(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);
> +	clk_bulk_unprepare(RKJPEGD_NUM_CLOCKS, jpegd->clocks);
> +}
> +
> +/*
> + * The clocks are prepared once and only enabled and disabled around a job,
> + * which is what lets rkjpegd_job_finish() run from the interrupt handler.
> + * There is nothing else to do around a runtime transition: the power domain
> + * is handled by genpd on the device's behalf.
> + */
> +static const struct dev_pm_ops rkjpegd_pm_ops = {
> +	SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)

[Severity: High]
Does this PM operation safely handle active DMA transactions during system
suspend?

Setting pm_runtime_force_suspend directly cuts power abruptly without ensuring
active DMA transactions are idled or aborted. Powering off the VDPU while it
is issuing AXI transactions hangs the interconnect on Rockchip platforms.

> +};
> +
> +static const struct of_device_id of_rkjpegd_match[] = {
> +	{ .compatible = "rockchip,rk3568-jpegd" },
> +	{ .compatible = "rockchip,rk3588-jpegd" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, of_rkjpegd_match);
> +
> +static struct platform_driver rkjpegd_driver = {
> +	.probe = rkjpegd_probe,
> +	.remove = rkjpegd_remove,
> +	.driver = {
> +		.name = RKJPEGD_NAME,
> +		.of_match_table = of_rkjpegd_match,
> +		.pm = pm_sleep_ptr(&rkjpegd_pm_ops),

[Severity: Medium]
Should this use pm_ptr() instead of pm_sleep_ptr()?

Because the structure pointer must be wrapped with pm_ptr(), using
pm_sleep_ptr() evaluates to NULL when CONFIG_PM_SLEEP is disabled, leaving
the statically defined ops unreferenced and generating a compiler warning.

> +	},
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824-rockchip-jpegdec-v1-0-8011822bf500@pengutronix.de?part=2