Re: [PATCH 1/3] media: nxp: imx-jpeg: cancel task_timer before freeing ctx

[sashiko-bot@kernel.org]

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

New issues:
- [High] Canceling the timeout timer before releasing the V4L2 mem2mem context causes an uninterruptible kernel hang if the hardware is hung, and fails to fully prevent the original Use-After-Free.

Pre-existing issues:
- [High] The timeout handler checks the shared device-level slot state rather than context-specific state, leading to a race condition that causes a NULL pointer dereference.
- [High] Error paths in mxc_jpeg_device_run() skip calling v4l2_m2m_job_finish(), stalling the M2M queue and causing a deadlock on release.
--

--- Patch [1]: [PATCH 1/3] media: nxp: imx-jpeg: cancel task_timer before freeing ctx ---
commit 80a788b1c14931407edfcc537272f62c4cc6d308
Author: Shengzhuo Wei <me@cherr.cc>

media: nxp: imx-jpeg: cancel task_timer before freeing ctx

This commit attempts to fix a use-after-free issue by canceling the
ctx->task_timer before the ctx is torn down in mxc_jpeg_release(). The timer
is canceled before acquiring the mxc_jpeg->lock to avoid deadlocks with
workers.

> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> index 725e941528848..fbb64a1ecb518 100644
> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> @@ -2796,6 +2796,8 @@ static int mxc_jpeg_release(struct file *file)
>  	struct mxc_jpeg_ctx *ctx = mxc_jpeg_file_to_ctx(file);
>  	struct device *dev = mxc_jpeg->dev;
>  
> +	cancel_delayed_work_sync(&ctx->task_timer);
> +

[Severity: High]
Does canceling the timeout timer before releasing the mem2mem context create a
potential deadlock and leave a race window?

If the hardware has hung, the TRANS_RUNNING flag remains set. When
v4l2_m2m_ctx_release() is called, v4l2_m2m_cancel_job() will wait indefinitely
for the running flag to clear because this driver lacks a job_abort callback.
Since the timeout timer was already canceled, it will never fire to clear the
flag, causing the close() operation to hang in an uninterruptible state.

Additionally, because the V4L2 M2M queues are not paused during the sync
cancel, could a concurrent worker thread dequeue a pending job and call
mxc_jpeg_device_run(), re-arming the timer after the cancel but before the
context release? This would re-introduce the use-after-free this patch seeks
to fix.

>  	mutex_lock(&mxc_jpeg->lock);
>  	if (mxc_jpeg->mode == MXC_JPEG_DECODE)
>  		dev_dbg(dev, "Release JPEG decoder instance on slot %d.",
[ ... ]
>  	v4l2_ctrl_handler_free(&ctx->ctrl_handler);
>  	v4l2_m2m_ctx_release(ctx->fh.m2m_ctx);
>  	v4l2_fh_del(&ctx->fh, file);


[Severity: High]
This isn't a bug introduced by this patch, but does the timeout handler check
the wrong state flag?

In mxc_jpeg_device_run_timeout():
	spin_lock_irqsave(&ctx->mxc_jpeg->hw_lock, flags);
	if (ctx->mxc_jpeg->slot_data.used) {
...
		mxc_jpeg_job_finish(ctx, VB2_BUF_STATE_ERROR, true);

If mxc_jpeg_dec_irq() completes its job, sets jpeg->slot_data.used = false,
unlocks hw_lock, and schedules a new job for a different context,
jpeg->slot_data.used could be set back to true before the timeout handler
acquires the lock.

When the timeout handler acquires the lock, it checks this shared device-level
flag instead of its own context state. Will it incorrectly assume its own job
timed out and call mxc_jpeg_job_finish() with NULL buffers (since they were
already removed by the IRQ handler), leading to a NULL pointer dereference in
v4l2_m2m_buf_done()?


[Severity: High]
This is a pre-existing issue, but do the error paths in mxc_jpeg_device_run()
leak the running state?

In mxc_jpeg_device_run():
	if (!mxc_jpeg_alloc_slot_data(jpeg)) {
		dev_err(dev, "Cannot allocate slot data\n");
		goto end;
	}
...
end:
	spin_unlock_irqrestore(&ctx->mxc_jpeg->hw_lock, flags);

If mxc_jpeg_alloc_slot_data() fails, the function jumps to the end label and
returns without calling v4l2_m2m_job_finish() and without arming the timeout
timer. Does this leave the job in the TRANS_RUNNING state? If so, when
mxc_jpeg_release() is later called, v4l2_m2m_cancel_job() will wait
indefinitely for the running flag to clear.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825-media-timeout-work-v1-0-ebfebbeb6c31@cherr.cc?part=1