linux-kernel - Re: [PATCH v4 05/11] media: rkvdec: h264: Remove SPS validation at streaming start

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c75c894a09292775773ad338121ee81924337cf0.camel@collabora.com>
Date:   Tue, 07 Nov 2023 17:01:02 -0500
From:   Nicolas Dufresne <nicolas.dufresne@...labora.com>
To:     Jonas Karlman <jonas@...boo.se>,
        Ezequiel Garcia <ezequiel@...guardiasur.com.ar>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Hans Verkuil <hverkuil-cisco@...all.nl>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Alex Bee <knaerzche@...il.com>,
        Benjamin Gaignard <benjamin.gaignard@...labora.com>,
        Sebastian Fricke <sebastian.fricke@...labora.com>,
        Christopher Obbard <chris.obbard@...labora.com>,
        linux-media@...r.kernel.org, linux-rockchip@...ts.infradead.org,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 05/11] media: rkvdec: h264: Remove SPS validation at
 streaming start

Le dimanche 05 novembre 2023 à 16:55 +0000, Jonas Karlman a écrit :
> SPS parameters is validated in try_ctrl() ops so there is no need to

                 are

> re-validate when streaming starts.
> 
> Remove the unnecessary call to validate sps at streaming start.

This patch is not working since user may change the format after the
control have been set. The proper fix should actually reset the SPS
(well all header controls) to match the the newly set format. Then this
validation won't be needed anymore.

The sequence that is problematic after this patch is:

S_FMT (OUTPUT, width, height);
S_CTRL (SPS) // valid
S_FMT(OUTPUT, width', height'); // SPS is no longer valid

One suggestion I may make is to add a ops so that each codec
implementation can reset their header controls to make it valid against
the new resolution. With that in place you'll be able drop the check.

Nicolas

p.s. you can also just drop this patch from the series and revisit it
later, though I think its worth fixing.

> 
> Suggested-by: Ezequiel Garcia <ezequiel@...guardiasur.com.ar>
> Signed-off-by: Jonas Karlman <jonas@...boo.se>
> ---
> v4:
> - No change
> 
> v3:
> - New patch
> 
>  drivers/staging/media/rkvdec/rkvdec-h264.c | 19 ++-----------------
>  1 file changed, 2 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> index 8bce8902b8dd..815d5359ddd5 100644
> --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> @@ -1070,17 +1070,6 @@ static int rkvdec_h264_start(struct rkvdec_ctx *ctx)
>  	struct rkvdec_dev *rkvdec = ctx->dev;
>  	struct rkvdec_h264_priv_tbl *priv_tbl;
>  	struct rkvdec_h264_ctx *h264_ctx;
> -	struct v4l2_ctrl *ctrl;
> -	int ret;
> -
> -	ctrl = v4l2_ctrl_find(&ctx->ctrl_hdl,
> -			      V4L2_CID_STATELESS_H264_SPS);
> -	if (!ctrl)
> -		return -EINVAL;
> -
> -	ret = rkvdec_h264_validate_sps(ctx, ctrl->p_new.p_h264_sps);
> -	if (ret)
> -		return ret;
>  
>  	h264_ctx = kzalloc(sizeof(*h264_ctx), GFP_KERNEL);
>  	if (!h264_ctx)
> @@ -1089,8 +1078,8 @@ static int rkvdec_h264_start(struct rkvdec_ctx *ctx)
>  	priv_tbl = dma_alloc_coherent(rkvdec->dev, sizeof(*priv_tbl),
>  				      &h264_ctx->priv_tbl.dma, GFP_KERNEL);
>  	if (!priv_tbl) {
> -		ret = -ENOMEM;
> -		goto err_free_ctx;
> +		kfree(h264_ctx);
> +		return -ENOMEM;
>  	}
>  
>  	h264_ctx->priv_tbl.size = sizeof(*priv_tbl);
> @@ -1100,10 +1089,6 @@ static int rkvdec_h264_start(struct rkvdec_ctx *ctx)
>  
>  	ctx->priv = h264_ctx;
>  	return 0;
> -
> -err_free_ctx:
> -	kfree(h264_ctx);
> -	return ret;
>  }
>  
>  static void rkvdec_h264_stop(struct rkvdec_ctx *ctx)