Re: [PATCH v15 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] Misuse of the untyped MIN() macro instead of standard min_t().

Pre-existing issues:
- [High] V4L2 API violation and data corruption due to unconditionally overwriting and clamping the payload size for OUTPUT buffers.
--

commit d7dd9a5c943d9d5a39c948601677f1b3e207a5e1
Author: Kyrie Wu <kyrie.wu@mediatek.com>

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

For multi-core jpegdec, a resolution change event requires the payload size to
change, but concurrent decoding by other cores prevents immediate updates.
This results in payload sizes mismatching the real buffer length. This patch
clamps the payload size to be less than the real buffer length to suppress
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 1e014fd698ae0..3960ad21d7f0c 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));
> +		} else {
> +			vb2_set_plane_payload(vb, i,
> +					      MIN(plane_fmt.sizeimage,
> +						  vb->planes[i].length));

[Severity: High]
This is a pre-existing issue, but does unconditionally clamping and
overwriting the payload size here violate the V4L2 API for output buffers?

For the compressed bitstream queue, userspace sets bytesused to the exact
bitstream length during QBUF. mtk_jpeg_buf_prepare() unconditionally
overwrites this with the clamped value. Because mtk_jpeg_set_dec_src()
calculates the hardware DMA end_addr using vb2_get_plane_payload():

mtk_jpeg_set_dec_src()
    bs->end_addr = bs->str_addr +
                   round_up(vb2_get_plane_payload(src_buf, 0), 16);

the hardware and software parsers are forced to read up to the full buffer
capacity, which can lead to parsing uninitialized memory or trailing garbage.

Additionally, for capture buffers during a resolution change, if userspace
queues an old, undersized buffer, plane_fmt.sizeimage will exceed
vb->planes[i].length. Instead of returning -EINVAL to synchronously reject
the buffer as V4L2 requires, this clamping merely silences the framework's
WARN_ON_ONCE. The undersized buffer is accepted and will fail asynchronously
later.


[Severity: Low]
Is it safer to use min_t() instead of the untyped MIN() macro here?

vb->planes[i].length is an unsigned int, while max_size (and
plane_fmt.sizeimage) is a size_t. Using min_t(size_t, ...) maintains proper
type safety and adheres to kernel coding standards, avoiding potential type
mismatch issues.

> +		}
>  	}
>  
>  	return 0;

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