Re: [PATCH 10/10] staging: media: cedrus: vp8: support frames wider than 2048 pixels

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] media: cedrus: vp8: stale deblk_buf_size in cedrus_vp8_start()
--

commit 2e3e9674b0e42d966116e3aba1d357709db8157f
Author: Jernej Skrabec <jernej.skrabec@gmail.com>

staging: media: cedrus: vp8: support frames wider than 2048 pixels

This commit adds support for decoding VP8 frames wider than 2048 pixels by
allocating the deblocking and intra prediction row buffers in DRAM when
they no longer fit in internal SRAM.

> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c b/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
> index 6b00efa77c223..954c0ceb98ed2 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c

[ ... ]

> @@ -849,7 +863,44 @@ static int cedrus_vp8_start(struct cedrus_ctx *ctx)
>  	memcpy(&ctx->codec.vp8.entropy_probs_buf[2048],
>  	       prob_table_init, sizeof(prob_table_init));
>  
> +	/*
> +	 * Above 2048 pixels the deblocking and intra prediction row buffers
> +	 * no longer fit into the internal SRAM. The vendor library sizes the
> +	 * deblocking buffer as 24 bytes per (32 aligned) column.
> +	 */
> +	if (ctx->src_fmt.width > 2048) {
> +		ctx->codec.vp8.deblk_buf_size =
> +			ALIGN(ctx->src_fmt.width, 32) * 24;
> +		ctx->codec.vp8.deblk_buf =
> +			dma_alloc_attrs(dev->dev, ctx->codec.vp8.deblk_buf_size,
> +					&ctx->codec.vp8.deblk_buf_dma,
> +					GFP_KERNEL, DMA_ATTR_NO_KERNEL_MAPPING);
> +		if (!ctx->codec.vp8.deblk_buf)
> +			goto err_probs;
> +
> +		ctx->codec.vp8.intra_pred_buf_size =
> +			ALIGN(ctx->src_fmt.width, 64) * 5 * 2;
> +		ctx->codec.vp8.intra_pred_buf =
> +			dma_alloc_attrs(dev->dev,
> +					ctx->codec.vp8.intra_pred_buf_size,
> +					&ctx->codec.vp8.intra_pred_buf_dma,
> +					GFP_KERNEL, DMA_ATTR_NO_KERNEL_MAPPING);
> +		if (!ctx->codec.vp8.intra_pred_buf)
> +			goto err_deblk;
> +	}
> +
>  	return 0;
> +
> +err_deblk:
> +	dma_free_attrs(dev->dev, ctx->codec.vp8.deblk_buf_size,
> +		       ctx->codec.vp8.deblk_buf, ctx->codec.vp8.deblk_buf_dma,
> +		       DMA_ATTR_NO_KERNEL_MAPPING);
> +	ctx->codec.vp8.deblk_buf_size = 0;
> +err_probs:
> +	dma_free_coherent(dev->dev, CEDRUS_ENTROPY_PROBS_SIZE,
> +			  ctx->codec.vp8.entropy_probs_buf,
> +			  ctx->codec.vp8.entropy_probs_buf_dma);
> +	return -ENOMEM;
>  }

[Severity: High]
Can ctx->codec.vp8.deblk_buf_size remain non-zero in cedrus_vp8_start()
when the DRAM buffers have not been allocated?

In cedrus_vp8_start(), ctx->codec.vp8.deblk_buf_size is set before calling
dma_alloc_attrs() for ctx->codec.vp8.deblk_buf. If that allocation fails,
execution jumps to err_probs, which skips resetting deblk_buf_size to 0
(and similarly, err_deblk does not reset intra_pred_buf_size to 0 when the
second allocation fails). If userspace then changes the OUTPUT format width
to 2048 or less and starts streaming again on the same context,
cedrus_vp8_start() skips the width > 2048 block and succeeds with
ctx->codec.vp8.deblk_buf_size still non-zero.

Similarly, because ctx->codec is a union in struct cedrus_ctx and
cedrus_vp8_start() does not clear deblk_buf_size when width <= 2048,
switching a context from another codec (such as H.264) to VP8 with
width <= 2048 can leave stale non-zero values in ctx->codec.vp8.

In both cases, cedrus_vp8_setup() and cedrus_vp8_stop() check
if (ctx->codec.vp8.deblk_buf_size), which will program VE_DBLK_DRAM_BUF_ADDR
and VE_INTRAPRED_DRAM_BUF_ADDR with unallocated or stale DMA addresses and
call dma_free_attrs() on unallocated or stale pointers when stopping.

Should cedrus_vp8_start() explicitly initialize deblk_buf_size and
intra_pred_buf_size to 0 (or reset deblk_buf_size under err_probs and
intra_pred_buf_size under err_deblk)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102170303.1973892-1-jernej.skrabec@gmail.com?part=10