Re: [PATCH v18 06/13] media: mediatek: jpeg: fix buffer state update timing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] media: Placebo lock and UAF in MTK JPEG worker timeout race
--

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

media: mediatek: jpeg: fix buffer state update timing

This commit delays the update of the destination buffer state until after the
decoder has selected a hardware core and successfully resumed its runtime PM
state. It aims to protect the destination buffer state update with the
hardware spinlock to avoid assigning frame tracking data to buffers that are
returned early.

> diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> index a5a1f6126af73..e152ebae0f219 100644
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c

[ ... ]

> @@ -1773,6 +1767,9 @@ static void mtk_jpegdec_worker(struct work_struct *work)
>  			      msecs_to_jiffies(MTK_JPEG_HW_TIMEOUT_MSEC));
>  
>  	spin_lock_irqsave(&comp_jpeg[hw_id]->hw_lock, flags);
> +	jpeg_dst_buf = mtk_jpeg_vb2_to_srcbuf(&dst_buf->vb2_buf);
> +	jpeg_dst_buf->curr_ctx = ctx;
> +	jpeg_dst_buf->frame_num = ctx->total_frame_num;

[Severity: High]
The commit message states the lock is used so the completion path observes a
consistent context and frame number. However, looking at the completion paths
in mtk_jpegdec_hw_irq_handler() and mtk_jpegdec_timeout_work(), they do not
appear to acquire comp_jpeg[hw_id]->hw_lock. Does this lock provide any
mutual exclusion against those paths?

Also, because schedule_delayed_work() arms the timeout before the buffer state
is initialized, is there a risk if the worker thread is preempted immediately
after arming the timer?

For example, could this sequence occur:

1. The timeout worker fires and executes without hw_lock.
2. It reads the uninitialized frame_num.
3. It adds the buffer to dst_done_queue, resets the hardware, and returns the
   buffer to userspace via v4l2_m2m_buf_done().
4. The preempted worker thread resumes and concurrently writes to
   jpeg_dst_buf->frame_num, corrupting the buffer that is already on the
   completion list or in userspace.
5. It then starts the hardware via mtk_jpeg_dec_start() on the
   already-returned buffer.

Could this lead to DMA into freed memory?

>  	ctx->total_frame_num++;
>  	mtk_jpeg_dec_reset(comp_jpeg[hw_id]->reg_base);
>  	mtk_jpeg_dec_set_config(comp_jpeg[hw_id]->reg_base,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791614327.git.kyrie.wu@mediatek.com?part=6