linux-kernel - Re: [PATCH v3 17/24] media: rkvdec: h264: Fix reference frame

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:   Mon, 25 Apr 2022 14:55:04 -0400
From:   Nicolas Dufresne <nicolas.dufresne@...labora.com>
To:     Hans Verkuil <hverkuil@...all.nl>,
        Ezequiel Garcia <ezequiel@...guardiasur.com.ar>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     kernel@...labora.com, linux-kernel@...r.kernel.org,
        Jonas Karlman <jonas@...boo.se>,
        Ezequiel Garcia <ezequiel@...labora.com>,
        Sebastian Fricke <sebastian.fricke@...labora.com>,
        linux-media@...r.kernel.org, linux-rockchip@...ts.infradead.org,
        linux-staging@...ts.linux.dev
Subject: Re: [PATCH v3 17/24] media: rkvdec: h264: Fix reference frame_num
 wrap for second field

Le vendredi 22 avril 2022 à 09:43 +0200, Hans Verkuil a écrit :
> On 05/04/2022 22:44, Nicolas Dufresne wrote:
> > From: Jonas Karlman <jonas@...boo.se>
> > 
> > When decoding the second field in a complementary field pair the second
> > field is sharing the same frame_num with the first field.
> > 
> > Currently the frame_num for the first field is wrapped when it matches the
> > field being decoded, this cause issues to decode the second field in a
> 
> cause issues to decode -> caused issues decoding
> 
> > complementary field pair.
> > 
> > Fix this by using inclusive comparison, less than or equal.
> 
> I would change this last sentence to:
> 
> 	Fix this by using inclusive comparison: 'less than or equal'.
> 
> It makes it a bit easier to parse.
> 
> > 
> > Signed-off-by: Jonas Karlman <jonas@...boo.se>
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@...labora.com>
> > Reviewed-by: Ezequiel Garcia <ezequiel@...labora.com>
> > Reviewed-by: Sebastian Fricke <sebastian.fricke@...labora.com>
> > ---
> >  drivers/staging/media/rkvdec/rkvdec-h264.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > index f081b476340f..60eaf06b6e25 100644
> > --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> > +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > @@ -781,7 +781,7 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
> >  			continue;
> >  
> >  		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM ||
> > -		    dpb[i].frame_num < dec_params->frame_num) {
> > +		    dpb[i].frame_num <= dec_params->frame_num) {
> 
> I wonder if a comment should be added here, explaining the reason for '<='.
> 
> It doesn't seem obvious to me. Up to you, though.

I guess I could, the algo for wrapping in the spec is (formula 8-27):

    if( FrameNum > frame_num )
        FrameNumWrap = FrameNum − MaxFrameNum
    else
        FrameNumWrap = FrameNum

Our implementation has the branch condition flip over, and the flipped version of that is:

    if( FrameNum <= frame_num )
        FrameNumWrap = FrameNum
    else
        FrameNumWrap = FrameNum − MaxFrameNum

There is no deeper rationale since we simply follow the recipe described in the
spec. This is done so that we can share that condition with that long term
reference handling.

> 
> >  			p[i] = dpb[i].frame_num;
> >  			continue;
> >  		}
> 
> Regards,
> 
> 	Hans