Re: [PATCH v8 13/14] media: mediatek: decoder: fill av1 buffer size with picinfo

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] media: mtk-vcodec: OOB DMA write due to concurrent global picinfo mutation in stateless AV1 decoder
--

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

media: mediatek: decoder: fill av1 buffer size with picinfo

This patch modifies the AV1 decoder to use the buffer sizes calculated
in the vcp/scp, filling each frame buffer size directly from picinfo
rather than computing it locally from the frame width and height.

> diff --git a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_av1_req_lat_if.c b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_av1_req_lat_if.c
> --- a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_av1_req_lat_if.c
> +++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_av1_req_lat_if.c
[ ... ]
> @@ -1811,18 +1811,19 @@ static int vdec_av1_slice_setup_core_buffer(struct vdec_av1_slice_instance *inst
>  {
[ ... ]
>  	/* frame buffer */
>  	vsi->fb.y.dma_addr = fb->base_y.dma_addr;
> +
> +	vsi->fb.y.size = instance->ctx->picinfo.fb_sz[0];
> +	vsi->fb.c.size = instance->ctx->picinfo.fb_sz[1];
> +

[Severity: Critical]
Does this introduce a race condition with concurrent requests?

The driver uses an asynchronous architecture where requests are processed
sequentially in the lat phase by mtk_vdec_worker, but core work is queued
and executed asynchronously.

If mtk_vdec_worker processes a new request, it synchronously updates the
global instance->ctx->picinfo to match the new sequence controls.
Concurrently, if the core worker processing the previous request executes
vdec_av1_slice_setup_core_buffer(), it will now read the mutated
instance->ctx->picinfo.fb_sz instead of the request-local dimension data.

>  	if (plane == 1)
> -		vsi->fb.c.dma_addr = fb->base_y.dma_addr + size;
> +		vsi->fb.c.dma_addr = fb->base_y.dma_addr + vsi->fb.y.size;

[Severity: Critical]
Can this regression lead to an out-of-bounds dma write?

If the subsequent request specifies a larger resolution, vsi->fb.y.size here
will be larger than the actually allocated vb2 capture buffer for the current
request.

Because this calculated c-plane dma address is not validated against the
physical capacity of the underlying vb2 buffer, the hardware could perform an
out-of-bounds write when it processes this offset. Userspace could potentially
trigger this by queuing sequential requests with different resolutions.

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