Re: [PATCH v3 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] VIDIOC_REQBUFS with count=0 bypasses job cancellation, causing DMA use-after-free and a permanent deadlock.
- [High] File descriptor closure after device unbind causes Use-After-Free of `m2m_dev` and `jpegd`.
- [High] Hardware interrupt can fire after `m2m_dev` is freed during `rkjpegd_remove`, causing a Use-After-Free.
- [High] Watchdog delayed work can be re-armed after cancellation in `rkjpegd_remove`, causing a Use-After-Free.
- [High] NULL pointer dereference in the IRQ handler if a spurious or pending interrupt fires during probe.
- [High] The interrupt handler reads hardware registers without checking if the device is active via pm_runtime, leading to crashes on spurious interrupts.
- [High] Runtime PM usage counter and genpd reference are leaked if the device is removed while a job is running.
- [High] Missing CPU cache flush after writing to the chroma plane of a DMA buffer.
- [Medium] TOCTOU data race on `ctx->source_change` causes spurious `-EINVAL` buffer rejections.
--

commit 9f20ee21619c78c1798cf24abb6642d0ab18573c
Author: Sascha Hauer <s.hauer@pengutronix.de>
Subject: media: rockchip: Add JPEG decoder driver

This commit adds a V4L2 M2M driver for the Rockchip VDPU720 JPEG hardware decoder. It handles JPEG input to NV12 output, utilizing a DMA side buffer for hardware tables and providing error recovery.

> diff --git a/drivers/media/platform/rockchip/rkjpegd/rkjpegd.c b/drivers/media/platform/rockchip/rkjpegd/rkjpegd.c
> index 0000000000000..f154759025dd9 100644
> --- /dev/null
> +++ b/drivers/media/platform/rockchip/rkjpegd/rkjpegd.c
[ ... ]
> @@ -634,6 +634,8 @@ static void rkjpegd_job_finish_no_pm(struct rkjpegd_ctx *ctx,
>  
>  	src = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
>  	dst = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
>  	if (WARN_ON(!src) || WARN_ON(!dst))
>  		return;
>  

[Severity: Critical]
If an active job is orphaned because VIDIOC_REQBUFS was called with count=0,
the empty queues will trigger this WARN_ON and the function returns early
without calling v4l2_m2m_job_finish(). Does this leave the M2M context
permanently deadlocked in the TRANS_RUNNING state?

> @@ -653,6 +653,8 @@ static void rkjpegd_irq_done(struct rkjpegd_dev *jpegd,
>  			     enum vb2_buffer_state state)
>  {
>  	struct rkjpegd_ctx *ctx = v4l2_m2m_get_curr_priv(jpegd->m2m_dev);
>  
>  	if (!ctx)
>  		return;

[Severity: High]
This dereferences jpegd->m2m_dev. If a spurious interrupt fires during probe
before rkjpegd_v4l2_init() initializes it, or if an active job finishes and
fires an interrupt during rkjpegd_remove() after rkjpegd_v4l2_cleanup() has
freed it, could this result in a NULL pointer dereference or use-after-free?

[ ... ]
> @@ -1220,6 +1220,8 @@ static int vdpu720_fill_chroma(struct rkjpegd_ctx *ctx,
>  		return -EINVAL;
>  	}
>  
>  	memset(dst_cpu + y_size, 0x80, size - y_size);
>  
>  	return 0;

[Severity: High]
This uses a CPU memset to write to the chroma plane of the destination buffer.
If the capture buffer is an imported DMABUF that is mapped cacheable, will
these writes remain in the CPU cache and cause downstream hardware to read
stale data unless an explicit DMA cache clean operation is performed?

[ ... ]
> @@ -1332,6 +1332,8 @@ static int rkjpegd_release(struct file *filp)
>  	struct rkjpegd_ctx *ctx = file_to_rkjpegd_ctx(filp);
>  
>  	v4l2_fh_del(&ctx->fh, filp);
>  	v4l2_m2m_ctx_release(ctx->fh.m2m_ctx);
>  	rkjpegd_vdpu720_exit(ctx);

[Severity: High]
When an application keeps the file descriptor open while the device is unbound
via sysfs, this function will execute later when the fd is finally closed.
Since rkjpegd_remove() has already freed m2m_dev and the devres framework has
freed the jpegd structure, won't accessing ctx->fh.m2m_ctx and ctx->dev here
trigger a use-after-free crash?

[ ... ]
> @@ -1402,6 +1402,8 @@ static void rkjpegd_v4l2_cleanup(struct rkjpegd_dev *jpegd)
>  	media_device_unregister(&jpegd->mdev);
>  	v4l2_m2m_unregister_media_controller(jpegd->m2m_dev);
>  	video_unregister_device(&jpegd->vdev);
>  	media_device_cleanup(&jpegd->mdev);
>  	v4l2_m2m_release(jpegd->m2m_dev);
>  	v4l2_device_unregister(&jpegd->v4l2_dev);
>  }

[Severity: High]
Should the driver rely on refcounting the v4l2_device (e.g., through a release
callback) instead of unconditionally freeing m2m_dev here, ensuring the
resources stay alive if file descriptors are still held open by userspace?

[ ... ]
> @@ -1457,6 +1457,8 @@ static irqreturn_t rkjpegd_vdpu720_irq(int irq, void *dev_id)
>  	enum vb2_buffer_state state;
>  	u32 status, clr_mask;
>  
>  	status = rkjpegd_read(jpegd, VDPU720_REG_INT);
>  

[Severity: High]
This reads the VDPU720_REG_INT register unconditionally upon entry. If a
spurious interrupt or an irqpoll event occurs while the device is in a
runtime-suspended state (with clocks and power domains gated off), will this
read trigger a synchronous external abort on ARM? Should it use
pm_runtime_get_if_active() to verify the power state first?

[ ... ]
> @@ -1549,6 +1549,8 @@ static void rkjpegd_source_change(struct rkjpegd_ctx *ctx,
>  
>  	dev_dbg(ctx->dev->dev, "source change to %ux%u\n", width, height);
>  
>  	v4l2_event_queue_fh(&ctx->fh, &rkjpegd_src_change_event);
>  	ctx->source_change = true;
>  	ctx->initial_source_change = false;

[Severity: Medium]
ctx->source_change is updated locklessly here after dropping fmt_lock.
Could a concurrent VIDIOC_QBUF operation in rkjpegd_buf_prepare() observe
this flag out of order, leading to a race condition?

[ ... ]
> @@ -1602,6 +1602,8 @@ static void rkjpegd_device_run(void *priv)
>  		return;
>  	}
>  
>  	ret = pm_runtime_resume_and_get(jpegd->dev);
>  	if (ret < 0)
>  		goto err_finish;

[Severity: High]
This acquires a runtime PM reference for the duration of the job. If the
device is unbound and rkjpegd_remove() is called while this job is still in
flight, is there a mechanism that correctly waits for the job and drops this
reference, or does it permanently leak the usage counter and genpd reference?

[ ... ]
> @@ -1615,6 +1615,8 @@ static void rkjpegd_stop_streaming(struct vb2_queue *vq)
>  		if (!vbuf)
>  			break;
>  		if (V4L2_TYPE_IS_CAPTURE(vq->type))
>  			vb2_set_plane_payload(&vbuf->vb2_buf, 0, 0);
>  		v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR);
>  	}

[Severity: Critical]
If an application calls VIDIOC_REQBUFS with count=0 while a decode job is
actively running, this loop removes all buffers and returns them with
VB2_BUF_STATE_ERROR. Since v4l2_m2m_ioctl_reqbufs() does not wait for the
hardware to finish, does this allow the hardware to write decoded video data
via DMA into memory pages that have already been freed back to the system?

[ ... ]
> @@ -1790,6 +1790,8 @@ static int rkjpegd_buf_prepare(struct vb2_buffer *vb)
>  
>  	if (ctx->source_change && vb2_is_streaming(vq))
>  		return 0;
>  
>  	mutex_lock(&ctx->fmt_lock);
>  	sizeimage = ctx->dst_fmt.plane_fmt[0].sizeimage;
>  	mutex_unlock(&ctx->fmt_lock);
>  
>  	if (vb2_plane_size(vb, 0) < sizeimage)
>  		return -EINVAL;

[Severity: Medium]
Because ctx->source_change is read locklessly above, if a concurrent thread sees
it as false, grabs fmt_lock, and reads the newly updated larger sizeimage, will
it incorrectly reject a valid queued buffer with -EINVAL?

[ ... ]
> @@ -2243,6 +2243,8 @@ static int rkjpegd_probe(struct platform_device *pdev)
>  	if (jpegd->irq < 0)
>  		return jpegd->irq;
>  
>  	ret = devm_request_irq(&pdev->dev, jpegd->irq, rkjpegd_vdpu720_irq, 0,
>  			       dev_name(&pdev->dev), jpegd);
>  	if (ret)
>  		return dev_err_probe(&pdev->dev, ret, "failed to request irq\n");

[Severity: High]
The IRQ is requested and unmasked here before rkjpegd_v4l2_init() initializes
jpegd->m2m_dev. If a pending or spurious interrupt fires immediately, could it
cause a NULL pointer dereference in rkjpegd_irq_done() when it accesses the
uninitialized m2m_dev?

[ ... ]
> @@ -2275,6 +2275,8 @@ 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]
The watchdog delayed work is canceled here, but rkjpegd_v4l2_cleanup() is called
before active jobs are flushed. Since V4L2 M2M ioctls and background jobs can
still be running, could a concurrent job dispatch re-arm the watchdog, causing a
use-after-free when devres frees the structure?

[Severity: High]
Furthermore, rkjpegd_v4l2_cleanup() frees m2m_dev before reset_control_assert()
is called and the IRQ is disabled. If an active hardware job completes in this
window, will the interrupt handler dereference the m2m_dev that was just freed?

>  	reset_control_assert(jpegd->resets);
>  	pm_runtime_dont_use_autosuspend(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
>  }

[Severity: High]
This unconditionally disables runtime PM without waiting for active jobs to finish
or dropping their PM references. Does this permanently leak the genpd and runtime
PM usage counters if the device is removed while a decode is in flight?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914-rockchip-jpegdec-v3-0-3583c376d0d2@pengutronix.de?part=2