Re: [PATCH v16 02/12] media: mediatek: jpeg: fix jpeg buffer payload size setting

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Use of uppercase MIN() macro for variable arguments bypasses kernel type checking.

Pre-existing issues:
- [Critical] Premature `v4l2_m2m_job_finish` in worker threads breaks `stop_streaming` synchronization, leading to Use-After-Free of `ctx` and DMA into freed memory.
--

--- Patch [2]: [PATCH v16 02/12] media: mediatek: jpeg: fix jpeg buffer payload size setting ---
commit b1d3acc23e7ae23715523f548df3f13e353a48e9
Author: Kyrie Wu <kyrie.wu@mediatek.com>

media: mediatek: jpeg: fix jpeg buffer payload size setting

For multi-core jpegdec, if one core gets resolution change event,
the payload size, representing the size of Y/C data, needs to change.
But others are decoding at the same time and it can not be changed
immediately, which results in the payload size not matching the real
buffer length.
The payload size must be less than the real buffer length to avoid
warning logs.

> diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> index 41c4cf8dc2626..34135706a4ac3 100644
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c

[ ... ]

> @@ -710,12 +711,20 @@ static int mtk_jpeg_buf_prepare(struct vb2_buffer *vb)
>  
>  	for (i = 0; i < q_data->fmt->colplanes; i++) {
>  		plane_fmt = q_data->pix_mp.plane_fmt[i];
> +		max_size = plane_fmt.sizeimage;
> +
>  		if (ctx->enable_exif &&
> -		    q_data->fmt->fourcc == V4L2_PIX_FMT_JPEG)
> -			vb2_set_plane_payload(vb, i, plane_fmt.sizeimage +
> -					      MTK_JPEG_MAX_EXIF_SIZE);
> -		else
> -			vb2_set_plane_payload(vb, i,  plane_fmt.sizeimage);
> +			q_data->fmt->fourcc == V4L2_PIX_FMT_JPEG) {
> +			max_size += MTK_JPEG_MAX_EXIF_SIZE;
> +
> +			vb2_set_plane_payload(vb, i,
> +					      MIN(vb->planes[i].length,
> +						  max_size));

[Severity: Low]
Is it appropriate to use the uppercase MIN() macro here? 

The MIN() macro in linux/minmax.h is generally intended for obvious constants
because it skips type checking.

Could this bypass type safety between size_t (max_size) and unsigned int
(length)? 

Would it be better to use min() with a cast, or min_t(size_t, ...) instead?

> +		} else {
> +			vb2_set_plane_payload(vb, i,
> +					      MIN(plane_fmt.sizeimage,
> +						  vb->planes[i].length));

[Severity: Low]
Does this also have the same type checking concern with the MIN() macro as
mentioned above?

> +		}
>  	}
>  
>  	return 0;

[Severity: Critical]
While reviewing this patch, I noticed a separate issue in the worker threads.
This isn't a bug introduced by this patch, but it appears that premature job
finishing could lead to a Use-After-Free.

In mtk_jpegdec_worker():

    mtk_jpeg_dec_start(comp_jpeg[hw_id]->reg_base);
    v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);

If the worker calls v4l2_m2m_job_finish() and removes buffers from V4L2
context tracking while the hardware is still processing them asynchronously,
could an unprivileged user closing the file descriptor trigger a release?

If mtk_jpeg_release() is called, it frees the context:

    kfree(ctx);

Then when the hardware interrupt fires later in mtk_jpegdec_hw_irq_handler():

    ctx = jpeg->hw_param.curr_ctx;
    ...
    pm_runtime_put(ctx->jpeg->dev);

Doesn't this result in the interrupt handler unconditionally accessing the
freed 'ctx' pointer and invoking V4L2 completion on freed buffers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720133432.1042046-1-kyrie.wu@mediatek.com?part=2