Re: [PATCH 2/3] media: verisilicon: hantro: cancel watchdog work before m2m release

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incomplete Fix: Pending V4L2 m2m jobs can re-arm the watchdog after cancellation, causing a Use-After-Free.
- [High] Hardware interrupt is not masked or freed before freeing `m2m_dev`, leading to a Use-After-Free and Bus Error.
- [High] Incorrect object lifetime management causes devres and `v4l2_m2m_put()` to unconditionally free `m2m_dev` and `vpu` while open file descriptors still exist.
- [High] Missing cancellation of `watchdog_work` in the `hantro_probe()` error paths.
--

--- Patch [2]: [PATCH 2/3] media: verisilicon: hantro: cancel watchdog work before m2m release ---
commit 36d662041efb45f2c47f99503f908090f9994b94
Author: Shengzhuo Wei <me@cherr.cc>
Subject: media: verisilicon: hantro: cancel watchdog work before m2m release

This commit adds a cancel_delayed_work_sync() call to hantro_remove() to
drain pending watchdog work before releasing the m2m device. This aims to
prevent a use-after-free where the watchdog dereferences the m2m device after
hantro_remove() drops the final reference.

> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
> --- a/drivers/media/platform/verisilicon/hantro_drv.c
> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
> @@ -1284,6 +1284,7 @@ static void hantro_remove(struct platform_device *pdev)
>  	hantro_remove_dec_func(vpu);
>  	hantro_remove_enc_func(vpu);
>  	media_device_cleanup(&vpu->mdev);
> +	cancel_delayed_work_sync(&vpu->watchdog_work);

[Severity: High]
This isn't a bug introduced by this patch, but is this fix complete?

Because the driver does not abort the m2m job queue or stop the hardware
before cancelling the watchdog, an active or pending m2m job can run
concurrently with driver removal.

If a job is processing, the m2m framework executes it via m2m_dev->job_work,
which eventually calls hantro_end_prepare_run(). That function explicitly
calls schedule_delayed_work(&ctx->dev->watchdog_work, ...).

Could this allow the watchdog to be re-armed immediately after the
cancel_delayed_work_sync() call here, leading to the exact use-after-free
this patch intends to resolve?

>  	v4l2_m2m_put(vpu->m2m_dev);

[Severity: High]
This is a pre-existing issue, but does freeing the m2m_dev here leave us
vulnerable to an interrupt handler use-after-free?

The hardware interrupt is managed by devres and is not explicitly masked or
freed before this point. If a hardware interrupt fires during this teardown
window, hantro_irq_done() will execute and call
v4l2_m2m_get_curr_priv(vpu->m2m_dev), dereferencing the freed m2m_dev.

[Severity: High]
This is also a pre-existing issue, but is it safe to unconditionally drop the
m2m_dev reference here while userspace might still have open file descriptors?

The vpu and func structures are allocated with devm_kzalloc(), and the V4L2
device release callback is stubbed out (video_device_release_empty). This
means when hantro_remove() returns, devres will immediately free these
structures.

If userspace later closes an open file descriptor, hantro_release() will call
v4l2_m2m_ctx_release(), which then calls v4l2_m2m_cancel_job(). This will
access the already freed m2m_dev and vpu.

Could this also cause the kernel workqueue subsystem to dereference a freed
work_struct if the m2m_dev's embedded job_work is pending on the system
workqueue when it is freed?

>  	v4l2_device_unregister(&vpu->v4l2_dev);
>  	clk_bulk_unprepare(vpu->variant->num_clocks, vpu->clocks);

Since clk_bulk_unprepare() is called right after dropping the m2m_dev, could
an active interrupt handler accessing registers with disabled clocks cause a
synchronous bus error?

There is another pre-existing issue in the hantro_probe() error path.

In hantro_probe(), the video devices are registered and exposed to userspace
before the rest of the initialization completes. If a later step in probe
fails, the error path tears down the device and allows devres to free the vpu
structure:

hantro_probe() {
    ...
err_rm_dec_func:
    hantro_remove_dec_func(vpu);
err_rm_enc_func:
    hantro_remove_enc_func(vpu);
err_m2m_rel:
    media_device_cleanup(&vpu->mdev);
    v4l2_m2m_put(vpu->m2m_dev);
    ...
}

[Severity: High]
Since cancel_delayed_work_sync(&vpu->watchdog_work) is missing from this
error path, could a job queued by userspace during the brief registration
window arm the watchdog, leading to a use-after-free when the pending timer
executes on the freed vpu memory?

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