Re: [PATCH v16 03/12] media: mediatek: jpeg: fix buffer structure size and layout

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Closing the device while a multi-core encoding/decoding job is running leads to Use-After-Free of the context and vb2 buffers in the interrupt handler.
- [High] Orphaned vb2 buffers in `dst_done_queue` are leaked on STREAMOFF.
- [High] Failure to cancel `job_timeout_work` in driver remove causes Use-After-Free.
- [High] Flawed reorder queue iteration in `mtk_jpegenc_put_buf` leads to stranded buffers and application hangs when hardware completes frames out of order.
- [Medium] The custom buffer structure `mtk_jpeg_src_buf` manually replicates the fields of `struct v4l2_m2m_buffer` instead of embedding it, which is an architectural anti-pattern and risks future memory corruption.
--

--- Patch [3]: [PATCH v16 03/12] media: mediatek: jpeg: fix buffer structure size and layout ---
commit 4b1c7af5910f25b2a11680a53e22df54bd4fcacd
Author: Kyrie Wu <kyrie.wu@mediatek.com>

    media: mediatek: jpeg: fix buffer structure size and layout
    
    Updates the buf_struct_size in the JPEG driver to use the correct
    structure mtk_jpeg_src_buf instead of v4l2_m2m_buffer, ensuring
    proper handling of source buffers. The previous buffer size led to
    mismatches and potential issues during video buffer handling.
    
    Moves frame_num in the mtk_jpeg_src_buf structure to align with
    other members appropriately. The first field of the driver-specific
    buffer structure must be the subsystem-specificstruct
    (vb2_v4l2_buffer in the case of V4L2).
    
    Fixes: 5fb1c2361e56 ("mtk-jpegenc: add jpeg encode worker interface")
    Signed-off-by: Kyrie Wu <kyrie.wu@mediatek.com>
    Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

> diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> index 34135706a4ac3..7856a556ce68e 100644
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> @@ -1092,7 +1092,7 @@ static int mtk_jpeg_queue_init(void *priv, struct vb2_queue *src_vq,
>  	dst_vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
>  	dst_vq->io_modes = VB2_DMABUF | VB2_MMAP;
>  	dst_vq->drv_priv = ctx;
> -	dst_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
> +	dst_vq->buf_struct_size = sizeof(struct mtk_jpeg_src_buf);
>  	dst_vq->ops = jpeg->variant->qops;
>  	dst_vq->mem_ops = &vb2_dma_contig_memops;
>  	dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.h b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.h
> index 6be5cf30dea1b..148fd41759b75 100644
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.h
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.h
> @@ -85,10 +85,10 @@ struct mtk_jpeg_variant {
>  };
>  
>  struct mtk_jpeg_src_buf {
> -	u32 frame_num;
>  	struct vb2_v4l2_buffer b;
>  	struct list_head list;
>  	u32 bs_size;
> +	u32 frame_num;
>  	struct mtk_jpeg_dec_param dec_param;
>  
>  	struct mtk_jpeg_ctx *curr_ctx;
>  };

[Severity: Medium]
This isn't a bug introduced by this patch, but is it safe to manually
replicate the fields of struct v4l2_m2m_buffer here instead of embedding it
directly?

Since v4l2_m2m_buf_queue casts mtk_jpeg_src_buf to v4l2_m2m_buffer, if
upstream modifies struct v4l2_m2m_buffer (for example, by adding a new
field), this implicit structural casting could cause v4l2_m2m_buf_queue to
access invalid memory and lead to silent kernel heap corruption.

There are also a few pre-existing regressions in this driver that I noticed
while reviewing the code.

[Severity: High]
This is a pre-existing issue, but does closing the device while a multi-core
encoding or decoding job is running lead to a use-after-free?

In drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c:mtk_jpegenc_worker():

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

The worker prematurely calls v4l2_m2m_job_finish() and clears the job running
state while the hardware is still running. If the user closes the device fd,
mtk_jpeg_release() will not wait for hardware completion and will immediately
free the context and queues.

When the hardware interrupt fires later:

drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c:mtk_jpegenc_hw_irq_handler():

	ctx = jpeg->hw_param.curr_ctx;
	src_buf = jpeg->hw_param.src_buffer;

The interrupt handler dereferences the context and buffers which may have
already been freed. Could this cause memory corruption?


[Severity: High]
This is a pre-existing issue, but can orphaned vb2 buffers leak when
streaming is stopped?

Looking at mtk_jpeg_enc_stop_streaming() in mtk_jpeg_core.c:

	while ((vb = mtk_jpeg_buf_remove(ctx, q->type)))
		v4l2_m2m_buf_done(vb, VB2_BUF_STATE_ERROR);

The function retrieves buffers from the active v4l2_m2m queues but appears to
fail to flush the ctx->dst_done_queue. If hardware operations complete out of
order, buffers wait in ctx->dst_done_queue. Does stopping the stream abandon
these pending buffers and cause a leak?


[Severity: High]
This is a pre-existing issue, but does the driver teardown logic fail to
cancel the delayed timeout work?

In drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c:mtk_jpeg_remove():

static void mtk_jpeg_remove(struct platform_device *pdev)
{
	struct mtk_jpeg_dev *jpeg = platform_get_drvdata(pdev);

	pm_runtime_disable(&pdev->dev);
        ...

The driver initializes a delayed work task (jpeg->job_timeout_work) to handle
hardware timeouts, but mtk_jpeg_remove() does not appear to call
cancel_delayed_work_sync(). If the driver is unbound while a job is timing
out, could the delayed work fire after the device structures are freed and
cause a use-after-free?


[Severity: High]
This is a pre-existing issue, but does the queue iteration logic in
mtk_jpegenc_put_buf() fail to restart its scan, leaving buffers stranded?

In drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c:mtk_jpegenc_put_buf():

	while (!list_empty(&ctx->dst_done_queue) &&
	       (pos != &ctx->dst_done_queue)) {
		list_for_each_prev_safe(pos, temp_entry, &ctx->dst_done_queue) {
                    ...
                }
        }

When frames complete out of order (e.g., 8, 9, 7), the outer while loop checks
pos != &ctx->dst_done_queue. However, list_for_each_prev_safe() always
terminates with pos pointing to the list head. Since there is no break inside
the outer loop, the outer condition evaluates to false immediately after one
pass, failing to restart the scan.

Could this leave frames indefinitely stranded in the queue, causing the
application to hang? This same pattern also appears in the decoder side in
mtk_jpegdec_put_buf() in mtk_jpeg_dec_hw.c.

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