Re: [PATCH 2/7] media: verisilicon: Complete the request on the ->run() error paths

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Unvalidated divisor derived from user-controlled AV1 tile configuration causes divide-by-zero panic.
- [Critical] Massive out-of-bounds write on a DMA coherent buffer due to unvalidated AV1 tile dimensions.
- [High] Missing NULL pointer check for destination buffer mapping in H1 JPEG encoder.
- [High] Media request control objects are leaked when device_run() fails early, causing indefinite blocking.
--

--- Patch [2]: [PATCH 2/7] media: verisilicon: Complete the request on the ->run() error paths ---
commit f5dd21ff10c1a9217ffb61543de91059d5e0444c
Author: Sascha Hauer <s.hauer@pengutronix.de>

media: verisilicon: Complete the request on the ->run() error paths

This commit modifies several Hantro codec ->run() callbacks to ensure
hantro_end_prepare_run() is called with an error code on early return paths.
This prevents media request control objects from being leaked when the run
operations fail.

[ ... ]
> The buffers are still returned to userspace, device_run() finishes the job
> with VB2_BUF_STATE_ERROR, but nothing completes the control handler object
> bound to the media request. vb2_buffer_done() only unbinds the request
> object owned by videobuf2 itself, so num_incomplete_objects never drops to
> zero and the request stays in MEDIA_REQUEST_STATE_QUEUED forever: poll() on
> the request file descriptor never returns and MEDIA_REQUEST_IOC_REINIT
> fails with -EBUSY. The request is only cleaned up when userspace closes it.

[Severity: High]
This is a pre-existing issue, but looking at device_run() in hantro_drv.c,
if it fails early before calling ctx->codec_ops->run(), it jumps to the
err_cancel_job label:

drivers/media/platform/verisilicon/hantro_drv.c:device_run() {
    ...
	if (ret < 0)
		goto err_cancel_job;

	ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
	if (ret)
		goto err_pm_put;

	v4l2_m2m_buf_copy_metadata(src, dst);

	if (ctx->codec_ops->run(ctx))
		goto err_clk_disable;

	return;
    ...
err_cancel_job:
	hantro_job_finish_no_pm(ctx->dev, ctx, VB2_BUF_STATE_ERROR);
}

Does this error path also leak the media request control object? It finishes
the job but doesn't seem to complete the control objects, which might leave
the request in MEDIA_REQUEST_STATE_QUEUED indefinitely.

[ ... ]
> diff --git a/drivers/media/platform/verisilicon/rockchip_vpu2_hw_jpeg_enc.c b/drivers/media/platform/verisilicon/rockchip_vpu2_hw_jpeg_enc.c
> index aa34bd2e47ad2..b273b0b56ced5 100644
> --- a/drivers/media/platform/verisilicon/rockchip_vpu2_hw_jpeg_enc.c
> +++ b/drivers/media/platform/verisilicon/rockchip_vpu2_hw_jpeg_enc.c
> @@ -143,8 +143,10 @@ int rockchip_vpu2_jpeg_enc_run(struct hantro_ctx *ctx)
>  
>  	memset(&jpeg_ctx, 0, sizeof(jpeg_ctx));
>  	jpeg_ctx.buffer = vb2_plane_vaddr(&dst_buf->vb2_buf, 0);
> -	if (!jpeg_ctx.buffer)
> +	if (!jpeg_ctx.buffer) {
> +		hantro_end_prepare_run(ctx, -ENOMEM);
>  		return -ENOMEM;
> +	}

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing this fix,
I noticed that hantro_h1_jpeg_enc_run() in hantro_h1_jpeg_enc.c appears to
be missing this same NULL pointer check entirely:

drivers/media/platform/verisilicon/hantro_h1_jpeg_enc.c:hantro_h1_jpeg_enc_run() {
    ...
	jpeg_ctx.buffer = vb2_plane_vaddr(&dst_buf->vb2_buf, 0);
	jpeg_ctx.width = ctx->dst_fmt.width;
	jpeg_ctx.height = ctx->dst_fmt.height;
	jpeg_ctx.quality = ctx->jpeg_quality;
	hantro_jpeg_header_assemble(&jpeg_ctx);
    ...
}

Since vb2_plane_vaddr() can return NULL if the buffer cannot be mapped,
could this cause a NULL pointer dereference in hantro_jpeg_header_assemble()
if userspace submits an unmappable buffer?

[Severity: Critical]
This is a pre-existing issue, but I noticed another potential problem related
to V4L2 control validation in the AV1 decoder:

drivers/media/platform/verisilicon/rockchip_vpu981_hw_av1_dec.c:rockchip_vpu981_av1_dec_set_tile_info() {
    ...
	int context_update_y =
	    tile_info->context_update_tile_id / tile_info->tile_cols;
	int context_update_x =
	    tile_info->context_update_tile_id % tile_info->tile_cols;
    ...
}

Userspace provides tile_cols via the V4L2_CID_STATELESS_AV1_FRAME control.
If this value is not validated by the V4L2 core or the driver, could setting
tile_cols to 0 trigger a divide-by-zero kernel panic?

[Severity: Critical]
This is another pre-existing issue in the same function. Further down,
tile_cols and tile_rows are used as bounds for a nested loop that writes
to the DMA coherent buffer:

drivers/media/platform/verisilicon/rockchip_vpu981_hw_av1_dec.c:rockchip_vpu981_av1_dec_set_tile_info() {
    ...
	for (tile0 = 0; tile0 < tile_info->tile_cols; tile0++) {
		for (tile1 = 0; tile1 < tile_info->tile_rows; tile1++) {
			int tile_id = tile1 * tile_info->tile_cols + tile0;
			u32 start, end;
			u32 y0 =
			    tile_info->height_in_sbs_minus_1[tile1] + 1;
			u32 x0 = tile_info->width_in_sbs_minus_1[tile0] + 1;

			/* tile size in SB units (width,height) */
			*dst++ = x0;
            ...
}

The av1_dec->tile_info.cpu buffer is allocated with a fixed size
(AV1_MAX_TILES * 16 bytes). If a user provides large unvalidated values
for tile_cols and tile_rows (up to 65535), could this cause a massive
out-of-bounds write and memory corruption?

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