Re: [PATCH 02/10] staging: media: cedrus: reject too large H.264/HEVC SPS

[Nicolas Dufresne <nicolas.dufresne@collabora.com>]

[-- Attachment #1: Type: text/plain, Size: 5334 bytes --]

Le lundi 02 novembre 2026 à 18:02 +0100, Jernej Skrabec a écrit :
> The hardware decodes whatever picture size the SPS describes. An SPS
> beyond CEDRUS_MAX_WIDTH/HEIGHT, or beyond the capture buffers once they
> are allocated, makes the hardware overrun memory and hang. On H616, the
> JCT-VC PICSIZE_A..D streams (1056x8440, 8440x1056, 528x4216 and
> 4216x528) leave the hardware wedged, so that every following job times
> out until reboot.
> 
> Validate the SPS picture size in try_ctrl. Move the size limits to
> cedrus_video.h for that.
> 
> Fixes: 6eb9b758e307 ("media: cedrus: Add H264 decoding support")
> Fixes: 86caab29da78 ("media: cedrus: Add HEVC/H.265 decoding support")
> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>  drivers/staging/media/sunxi/cedrus/cedrus.c   | 40 ++++++++++++++++++-
>  .../staging/media/sunxi/cedrus/cedrus_video.c |  5 ---
>  .../staging/media/sunxi/cedrus/cedrus_video.h |  5 +++
>  3 files changed, 44 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
> index c3331b4f3f02..45deebddb630 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
> @@ -28,10 +28,38 @@
>  #include "cedrus_dec.h"
>  #include "cedrus_hw.h"
>  
> +/*
> + * The hardware decodes whatever picture size the bitstream headers describe.
> + * Reject sizes beyond the hardware limits, or beyond the capture buffers once
> + * they are allocated, instead of letting the hardware overrun memory and hang.
> + */
> +static int cedrus_check_pic_size(struct cedrus_ctx *ctx, u32 width, u32 height)
> +{
> +	struct vb2_queue *vq;
> +
> +	if (width > CEDRUS_MAX_WIDTH || height > CEDRUS_MAX_HEIGHT)
> +		return -EINVAL;
> +
> +	/*
> +	 * The hardware decodes whole 16x16 macroblocks, and all capture
> +	 * formats pad the buffers to at least 16 pixels in both directions.
> +	 */
> +	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
> +	if (vb2_is_busy(vq) &&
> +	    (width > ALIGN(ctx->src_fmt.width, 16) ||
> +	     height > ALIGN(ctx->src_fmt.height, 16)))
> +		return -EINVAL;

If my memory is correct, Hans rejected this kind of construct in try_ctrl,
because if you are only doing try() it shouldn't fail like this. Instead we can
validate in the real s_ctrl, but I believe the downside is that it makes the
error asynchronous when a request is used (perhaps that's actually wanted ?)

Nicolas

> +
> +	return 0;
> +}
> +
>  static int cedrus_try_ctrl(struct v4l2_ctrl *ctrl)
>  {
> +	struct cedrus_ctx *ctx = container_of(ctrl->handler, struct cedrus_ctx, hdl);
> +
>  	if (ctrl->id == V4L2_CID_STATELESS_H264_SPS) {
>  		const struct v4l2_ctrl_h264_sps *sps = ctrl->p_new.p_h264_sps;
> +		u32 height;
>  
>  		if (sps->chroma_format_idc != 1)
>  			/* Only 4:2:0 is supported */
> @@ -42,9 +70,15 @@ static int cedrus_try_ctrl(struct v4l2_ctrl *ctrl)
>  		if (sps->bit_depth_luma_minus8 != 0)
>  			/* Only 8-bit is supported */
>  			return -EINVAL;
> +		height = (sps->pic_height_in_map_units_minus1 + 1) * 16;
> +		if (!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY))
> +			height *= 2;
> +		if (cedrus_check_pic_size(ctx,
> +					  (sps->pic_width_in_mbs_minus1 + 1) * 16,
> +					  height))
> +			return -EINVAL;
>  	} else if (ctrl->id == V4L2_CID_STATELESS_HEVC_SPS) {
>  		const struct v4l2_ctrl_hevc_sps *sps = ctrl->p_new.p_hevc_sps;
> -		struct cedrus_ctx *ctx = container_of(ctrl->handler, struct cedrus_ctx, hdl);
>  		unsigned int bit_depth, max_depth;
>  		struct vb2_queue *vq;
>  
> @@ -52,6 +86,10 @@ static int cedrus_try_ctrl(struct v4l2_ctrl *ctrl)
>  			/* Only 4:2:0 is supported */
>  			return -EINVAL;
>  
> +		if (cedrus_check_pic_size(ctx, sps->pic_width_in_luma_samples,
> +					  sps->pic_height_in_luma_samples))
> +			return -EINVAL;
> +
>  		bit_depth = max(sps->bit_depth_luma_minus8,
>  				sps->bit_depth_chroma_minus8) + 8;
>  
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> index adc7f255d7cf..2521315abdb7 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> @@ -29,11 +29,6 @@
>  #define CEDRUS_DECODE_SRC	BIT(0)
>  #define CEDRUS_DECODE_DST	BIT(1)
>  
> -#define CEDRUS_MIN_WIDTH	16U
> -#define CEDRUS_MIN_HEIGHT	16U
> -#define CEDRUS_MAX_WIDTH	4096U
> -#define CEDRUS_MAX_HEIGHT	2304U
> -
>  static struct cedrus_format cedrus_formats[] = {
>  	{
>  		.pixelformat	= V4L2_PIX_FMT_MPEG2_SLICE,
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.h b/drivers/staging/media/sunxi/cedrus/cedrus_video.h
> index 8e1afc16a6a1..367e52158f01 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.h
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.h
> @@ -22,6 +22,11 @@ struct cedrus_format {
>  	unsigned int	capabilities;
>  };
>  
> +#define CEDRUS_MIN_WIDTH	16U
> +#define CEDRUS_MIN_HEIGHT	16U
> +#define CEDRUS_MAX_WIDTH	4096U
> +#define CEDRUS_MAX_HEIGHT	2304U
> +
>  extern const struct v4l2_ioctl_ops cedrus_ioctl_ops;
>  
>  int cedrus_queue_init(void *priv, struct vb2_queue *src_vq,

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]