Re: [PATCH] media: imx-jpeg: Cancel timeout work on release

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Concurrent execution of `v4l2_m2m_ctx_release()` and `v4l2_m2m_job_finish()` causes a use-after-free of `m2m_ctx`.
- [High] Race condition in `mxc_jpeg_device_run_timeout()` causes a NULL pointer dereference if it executes after its context's job finishes and another context starts.
--

commit ba4456d4cd6b4b2046f0bf363757a56e4661b019
Author: Kazuki Hanai <hnkz.64@gmail.com>

media: imx-jpeg: Cancel timeout work on release

This commit adds a synchronous cancellation of the timeout work during
device release to prevent a use-after-free. It places the cancellation
after the mem2mem context release.

> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> @@ -2805,6 +2805,7 @@ static int mxc_jpeg_release(struct file *file)
>  			ctx->slot);
>  	v4l2_ctrl_handler_free(&ctx->ctrl_handler);
>  	v4l2_m2m_ctx_release(ctx->fh.m2m_ctx);
> +	cancel_delayed_work_sync(&ctx->task_timer);

[Severity: High]
This is a pre-existing issue, but does placing the sync cancel after
the mem2mem release still leave a use-after-free window?

If mxc_jpeg_release() calls v4l2_m2m_ctx_release(), it blocks in
v4l2_m2m_cancel_job() waiting for the hung hardware job to finish.

Concurrently, the timeout work mxc_jpeg_device_run_timeout() can
execute and call v4l2_m2m_job_finish(). Inside v4l2_m2m_job_finish(),
the job is marked finished and the wait queue is woken up, which
unblocks v4l2_m2m_ctx_release().

The release function then immediately frees m2m_ctx.

Meanwhile, v4l2_m2m_job_finish() continues on the timeout thread,
checks schedule_next, and calls v4l2_m2m_schedule_next_job(m2m_dev, m2m_ctx).
Will this access the newly freed m2m_ctx?

[Severity: High]
This is also a pre-existing issue, but since we are dealing with the
timeout work cancellation, is there a race condition in
mxc_jpeg_device_run_timeout() that can cause a NULL pointer dereference?

If a job for Context A finishes successfully, the IRQ handler
mxc_jpeg_dec_irq() calls cancel_delayed_work() (which is not synchronous).
If Context A's delayed work has already started executing but was preempted
before acquiring hw_lock, cancel_delayed_work() returns without stopping it.

The IRQ handler schedules the next job for Context B, which acquires hw_lock
and sets ctx->mxc_jpeg->slot_data.used = true.

Context A's timeout work then resumes, acquires hw_lock, and incorrectly
observes the globally shared slot_data.used as true. Assuming it timed out,
it calls mxc_jpeg_job_finish(ctxA, ...).

Since Context A has no active job, v4l2_m2m_next_src_buf() returns NULL.
When mxc_jpeg_job_finish() passes this NULL pointer to v4l2_m2m_buf_done(),
will it cause an unconditional dereference in vb2_buffer_done()?

>  	v4l2_fh_del(&ctx->fh, file);
>  	v4l2_fh_exit(&ctx->fh);
>  	kfree(ctx);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828051452.1940559-1-hnkz.64@gmail.com?part=1