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: 7175 bytes --]

Le mardi 03 novembre 2026 à 19:02 +0100, Jernej Škrabec a écrit :
> Dne ponedeljek, 2. november 2026 ob 19:44:38 Srednjeevropski standardni čas je Nicolas Dufresne napisal(a):
> > 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 ?)
> 
> You're right. This is not the best place to check size, since it can be
> bypassed as sashiko bot find out.
> 
> My suggestion for v2 is to split this to two stages. Max width and height
> check stays here, whereas buffer size check is moved to setup() callback.
> Failing there it will mark buffer with V4L2_BUF_FLAG_ERROR.

Agreed. I just looked quikcly at the Sashiko report, and it underline the fact
that you can't rely on getting an S_CTR in the first place.

Though, thinking about it, to be completely strict (keeping everything on sync
all the time like the spec states), since SPS change comes first, we should
reset both fmt from S_CTRL when not streaming, and always match the SPS from
TRY_FMT later on. Later SPS update at runtime would have to fit allocated
buffers (since we don't do SRC_CH in stateless), and probably fit the format
(since you can't reset the format while running).

If what I'm thinking is correct, we are up to patching all the drivers, since
none are strictly correct today :-(

Nicolas

> 
> What do you think?
> 
> Best regards,
> Jernej
> 
> > 
> > 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 --]