Re: [PATCH 3/3] media: mediatek: jpeg: cancel job timeout work before m2m release

[sashiko-bot@kernel.org]

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

New issues:
- [High] Cancelling the job timeout work in remove leaves stalled jobs in TRANS_RUNNING state forever, causing a deadlock on file close.
- [Medium] Calling cancel_delayed_work_sync() on an uninitialized delayed_work struct for multi-core variants causes a kernel WARNING.

Pre-existing issues:
- [High] Releasing m2m_dev in mtk_jpeg_remove() causes a Use-After-Free when open file descriptors are eventually closed.
- [High] Missing cleanup of sub-device job_timeout_work in multi-core mode causes a use-after-free.
- [High] Pending multi-core workers in jpeg->workqueue cause a use-after-free during devres cleanup.
- [High] Missing hardware stop and IRQ disable before freeing m2m_dev causes UAF in IRQ handler.
- [High] A race condition between the hardware IRQ handler and the timeout worker can result in a NULL pointer dereference.
- [Medium] Missing PM usage counter balance on cancellation and error paths.
--

--- Patch [3]: [PATCH 3/3] media: mediatek: jpeg: cancel job timeout work before m2m release ---
commit 783b89e56f061c0e51e278c73a5f2399a27451fa
Author: Shengzhuo Wei <me@cherr.cc>

media: mediatek: jpeg: cancel job timeout work before m2m release

This patch attempts to fix a use-after-free by cancelling the job timeout
work before releasing the m2m device in mtk_jpeg_remove().

> diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> index d147ec4830811..f0fe14e83d515 100644
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> @@ -1405,6 +1405,7 @@ static void mtk_jpeg_remove(struct platform_device *pdev)
>  
>  	pm_runtime_disable(&pdev->dev);
>  	video_unregister_device(jpeg->vdev);
> +	cancel_delayed_work_sync(&jpeg->job_timeout_work);

[Severity: High]
Does cancelling the job timeout work here leave stalled jobs in the
TRANS_RUNNING state forever?

If a job is currently hanging when the driver is unbound, the timeout work
is cancelled and will never execute. Since the hardware is hung, no IRQ
will arrive to complete the job, and the driver does not implement the
job_abort m2m callback.

When the user subsequently closes the file descriptor,
v4l2_m2m_ctx_release() invokes v4l2_m2m_cancel_job(), which indefinitely
blocks in wait_event() waiting for TRANS_RUNNING to be cleared. Could this
create an unkillable process in the D state?

[Severity: Medium]
Will this unconditionally pass a zero-initialized work struct to the
workqueue subsystem for multi-core variants?

In mtk_jpeg_probe(), jpeg->job_timeout_work is only initialized via
INIT_DELAYED_WORK if jpeg->variant->multi_core is false. For multi-core
variants, it remains uninitialized.

When cancel_delayed_work_sync() executes on multi-core variants during
unbind, could it hit a WARN_ON(!work->func) inside flush_work() and
trigger debug object warnings?

[Severity: Medium]
Does unconditionally cancelling the timeout worker here leak the PM usage
counter?

When a job starts, it takes a PM reference via pm_runtime_get_sync(). If
the job stalls, the timeout work normally balances this reference by
calling pm_runtime_put(). By cancelling the worker without issuing a
compensating pm_runtime_put(), the usage counter may never be balanced.

This also relates to a pre-existing issue in mtk_jpegenc_worker() and
mtk_jpegdec_worker(). If pm_runtime_get_sync() fails, the code branches to
the error path without calling pm_runtime_put_noidle(). Since
pm_runtime_get_sync() increments the counter even on failure, does this
also permanently leak the usage counter?

>  	v4l2_m2m_release(jpeg->m2m_dev);

[Severity: High]
This is a pre-existing issue, but does releasing m2m_dev here cause a
use-after-free when open file descriptors are eventually closed?

video_unregister_device() does not forcefully close existing open file
descriptors. If a user-space process holds an FD open during device
unbind, jpeg->m2m_dev is freed while ctx->fh.m2m_ctx remains active.

When the user eventually closes the FD, mtk_jpeg_release() accesses the
freed jpeg struct and calls v4l2_m2m_ctx_release(). This internally
accesses the freed m2m_dev (e.g., locking m2m_dev->job_spinlock in
v4l2_m2m_cancel_job()), which seems reachable by unprivileged users simply
keeping the FD open.

[Severity: High]
This is a pre-existing issue, but does freeing m2m_dev here race with
pending multi-core workers in jpeg->workqueue?

Active multi-core jobs are executed on a devm-managed workqueue
(jpeg->workqueue). This workqueue is destroyed via devres, which flushes
pending work after mtk_jpeg_remove() has completed and freed
jpeg->m2m_dev.

When the workqueue is flushed during devres destruction, delayed workers
like mtk_jpegenc_worker() are forced to execute. Do they dereference the
already freed jpeg->m2m_dev via v4l2_m2m_next_src_buf() and
v4l2_m2m_job_finish(), resulting in a use-after-free?

[Severity: High]
This is a pre-existing issue, but is m2m_dev released here while the IRQ
is still active and hardware might still be running?

Because the interrupt was registered using devm_request_irq(), it remains
fully active during and after mtk_jpeg_remove() executes. If a hardware
job completes shortly after v4l2_m2m_release(), the IRQ handler
mtk_jpeg_enc_irq() will execute and call mtk_jpeg_enc_done().

Does this unconditionally dereference the already freed jpeg->m2m_dev in
the interrupt handler?

>  	v4l2_device_unregister(&jpeg->v4l2_dev);
>  }

[Severity: High]
This is a pre-existing issue, but does the lack of a remove callback for
sub-devices cause a use-after-free for their timeout works?

Multi-core variants use timeout works embedded in component hardware
devices, such as dev->job_timeout_work in mtk_jpeg_dec_hw.c. However,
component drivers like mtk_jpegdec_hw_driver lack a remove callback.

When the master device unbinds, devres frees the sub-devices' memory
without cancelling their timeout works. If a multi-core job is stalled
during removal, will the uncancelled component timeout work eventually
execute on freed memory?

[Severity: High]
This is a pre-existing issue, but is there a race condition between the
hardware IRQ handler and the timeout worker that can result in a NULL
pointer dereference?

In mtk_jpeg_job_timeout_work(), buffers are removed via
v4l2_m2m_src_buf_remove() and passed directly to v4l2_m2m_buf_done(). The
IRQ handlers, like mtk_jpeg_enc_irq(), call cancel_delayed_work(), which
is asynchronous and returns immediately if the timeout worker is already
running.

The IRQ handler then proceeds to remove buffers from the same context. If
the timeout worker and IRQ handler execute concurrently on different CPUs,
they will both pop from the queue. One will receive a valid buffer and the
other will receive NULL.

Since the one receiving NULL passes it to v4l2_m2m_buf_done(), which
directly dereferences buf->vb2_buf without a NULL check, could this cause
a kernel panic?

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