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]

Date:   Tue, 7 Nov 2023 23:56:57 +0100
From:   Jonas Karlman <jonas@...boo.se>
To:     Nicolas Dufresne <nicolas.dufresne@...labora.com>,
        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

On 2023-11-07 23:01, Nicolas Dufresne wrote:
> 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.

According to the Initialization section of the V4L2 stateless
documentation a client is supposed to S_CTRL(SPS) after S_FMT(OUTPUT).

https://docs.kernel.org/userspace-api/media/v4l/dev-stateless-decoder.html#initialization

I guess that all stateless decoders probably should reset all ctrls to
default value on S_FMT(OUTPUT) or decoders may end up in an unexpected
state?

Is resetting a ctrl value back to default something that is supported by
v4l2 ctrl core? Did not find any obvious way to reset a ctrl value.

Will probably just drop this patch in v5.

Regards,
Jonas

> 
> 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)
>