Re: [PATCH 1/7] media: verisilicon: Fix the cleanup when a codec ->run() fails

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing calls to `hantro_end_prepare_run(ctx, error)` on early error paths in multiple codec `->run()` callbacks, leading to permanently incomplete V4L2 control requests.
- [High] Missing NULL pointer check for the return value of `vb2_plane_vaddr()` before dereferencing it during JPEG header assembly.
- [High] Missing synchronization to cancel `vpu->watchdog_work` during device removal, leading to a Use-After-Free.
- [High] Race condition between late hardware IRQ delivery and `hantro_watchdog()` execution causes the watchdog of the *next* job to be cancelled and the next job to be prematurely finished.
--

--- Patch [1]: [PATCH 1/7] media: verisilicon: Fix the cleanup when a codec ->run() fails ---
commit 67d0bec622e69de9d51450bf1c4cea20e9bb249c
Author: Sascha Hauer <s.hauer@pengutronix.de>
media: verisilicon: Fix the cleanup when a codec ->run() fails

This commit pairs hantro_start_prepare_run() and hantro_end_prepare_run()
on error paths to ensure V4L2 requests are properly completed. It adds an
error argument to hantro_end_prepare_run() so the watchdog is only armed
when the hardware actually starts, preventing unrelated jobs from being
aborted. Resources are also properly released in device_run() on failure.

Fixes: 892bb6ecead9 ("media: hantro: do a PM resume earlier")
Fixes: e2da465455ce ("media: hantro: Support VP9 on the G2 core")
Fixes: 727a400686a2 ("media: verisilicon: Add Rockchip AV1 decoder")

> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
> index 2e81877f640fb..d9936f6979d2c 100644
> --- a/drivers/media/platform/verisilicon/hantro_drv.c
> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
[ ... ]
> @@ -162,6 +179,9 @@ void hantro_end_prepare_run(struct hantro_ctx *ctx)
>  	v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req,
>  				   &ctx->ctrl_handler);
>  
> +	if (error)
> +		return;
> +
>  	/* Kick the watchdog. */
>  	schedule_delayed_work(&ctx->dev->watchdog_work,
>  			      msecs_to_jiffies(2000));

[Severity: High]
This is a pre-existing issue, but does the device removal process in
hantro_remove() lack synchronization to cancel vpu->watchdog_work?

The driver schedules vpu->watchdog_work when a job starts. If a
userspace application is streaming and a timeout occurs precisely when
the device is unbound, the watchdog work might execute after the vpu
object is freed by devres. This could lead to a use-after-free since
cancel_delayed_work_sync() is not called during teardown.

[Severity: High]
This isn't a bug introduced by this patch, but can a late hardware IRQ
delivery cause a race condition with hantro_watchdog()?

If a job times out, hantro_watchdog() resets the hardware and completes
the job. The M2M framework then schedules the next job and arms its
watchdog. If the delayed hardware IRQ for the original job fires,
hantro_irq_done() targets the current context (the new job) and cancels
its watchdog. Could this cause the IRQ handler to erroneously call
hantro_job_finish() on the new job while the hardware is still processing
it, leading to state corruption and premature buffer returns?

> diff --git a/drivers/media/platform/verisilicon/hantro_g1_h264_dec.c b/drivers/media/platform/verisilicon/hantro_g1_h264_dec.c
> index ad5c1a6634f5c..30c2ac32cd0b0 100644
> --- a/drivers/media/platform/verisilicon/hantro_g1_h264_dec.c
> +++ b/drivers/media/platform/verisilicon/hantro_g1_h264_dec.c
[ ... ]
> @@ -264,7 +264,7 @@ int hantro_g1_h264_dec_run(struct hantro_ctx *ctx)
>  	set_ref(ctx);
>  	set_buffers(ctx, src_buf);
>  
> -	hantro_end_prepare_run(ctx);
> +	hantro_end_prepare_run(ctx, 0);

[Severity: High]
This is a pre-existing issue, but do the early error paths in codec run
callbacks bypass hantro_end_prepare_run() after hantro_start_prepare_run()
is invoked internally?

If context preparation or reference setup fails in hantro_g1_h264_dec_run(),
hantro_g2_hevc_dec_run(), or hantro_g1_vp8_dec_run(), early returns might
miss the required call to hantro_end_prepare_run(). This can leave the
V4L2 control requests incomplete indefinitely, which could lead to a resource
leak and userspace application hang when malformed streams or invalid V4L2
parameters trigger these paths.

> diff --git a/drivers/media/platform/verisilicon/hantro_h1_jpeg_enc.c b/drivers/media/platform/verisilicon/hantro_h1_jpeg_enc.c
> index 86cc1a07026f0..78f51e92fb2e3 100644
> --- a/drivers/media/platform/verisilicon/hantro_h1_jpeg_enc.c
> +++ b/drivers/media/platform/verisilicon/hantro_h1_jpeg_enc.c
[ ... ]
> @@ -148,7 +148,7 @@ int hantro_h1_jpeg_enc_run(struct hantro_ctx *ctx)
>  		| H1_REG_ENC_PIC_INTRA
>  		| H1_REG_ENC_CTRL_EN_BIT;
>  
> -	hantro_end_prepare_run(ctx);
> +	hantro_end_prepare_run(ctx, 0);

[Severity: High]
This isn't a bug introduced by this patch, but is there a missing NULL
pointer check for the return value of vb2_plane_vaddr() in
hantro_h1_jpeg_enc_run() before it is dereferenced?

Earlier in this function, vb2_plane_vaddr() assigns the virtual address
of the destination buffer to jpeg_ctx.buffer. If the buffer mapping fails
due to an invalid exporter or vmalloc exhaustion, it returns NULL. The
pointer is then unconditionally passed to hantro_jpeg_header_assemble(),
which calls memcpy() on it, potentially causing a kernel panic. Should
there be a check here to safely handle mapping failures?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260819-rk3588-jpegdec-v1-0-33d74cdf369c@pengutronix.de?part=1