linux-kernel - Re: Re: [PATCH] media: cedrus: hevc: Add check for invalid timestamp

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 for Android: free password hash cracker in your pocket

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

Message-ID: <b588b53fcfe539692a0a55b9fd0e97528def1d96.camel@collabora.com>
Date:   Mon, 18 Jul 2022 15:37:31 -0400
From:   Nicolas Dufresne <nicolas.dufresne@...labora.com>
To:     Jernej Škrabec <jernej.skrabec@...il.com>,
        mripard@...nel.org, paul.kocialkowski@...tlin.com
Cc:     mchehab@...nel.org, gregkh@...uxfoundation.org, wens@...e.org,
        samuel@...lland.org, hverkuil-cisco@...all.nl,
        ezequiel@...guardiasur.com.ar, linux-media@...r.kernel.org,
        linux-staging@...ts.linux.dev,
        linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev,
        linux-kernel@...r.kernel.org
Subject: Re: Re: [PATCH] media: cedrus: hevc: Add check for invalid timestamp

Le lundi 18 juillet 2022 à 19:57 +0200, Jernej Škrabec a écrit :
> Dne ponedeljek, 18. julij 2022 ob 19:41:48 CEST je Nicolas Dufresne 
> napisal(a):
> > Le lundi 18 juillet 2022 à 18:56 +0200, Jernej Skrabec a écrit :
> > > Not all DPB entries will be used most of the time. Unused entries will
> > > thus have invalid timestamps. They will produce negative buffer index
> > > which is not specifically handled. This works just by chance in current
> > > code. It will even produce bogus pointer, but since it's not used, it
> > > won't do any harm.
> > > 
> > > Let's fix that brittle design by skipping writing DPB entry altogether
> > > if timestamp is invalid.
> > > 
> > > Fixes: 86caab29da78 ("media: cedrus: Add HEVC/H.265 decoding support")
> > > Signed-off-by: Jernej Skrabec <jernej.skrabec@...il.com>
> > > ---
> > > 
> > >  drivers/staging/media/sunxi/cedrus/cedrus_h265.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > > b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c index
> > > 1afc6797d806..687f87598f78 100644
> > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > > @@ -147,6 +147,9 @@ static void cedrus_h265_frame_info_write_dpb(struct
> > > cedrus_ctx *ctx,> 
> > >  			dpb[i].pic_order_cnt_val
> > >  		
> > >  		};
> > > 
> > > +		if (buffer_index < 0)
> > > +			continue;
> > 
> > When I compare to other codecs, when the buffer_index does not exist, the
> > addr 0 is being programmed into the HW. With this implementation is is left
> > to whatever it was set for the previous decode operation. I think its is
> > nicer done the other way.
> 
> It's done the same way as it's done in vendor lib. As I stated in commit 
> message, actual values don't matter for unused entries. If it is used by 
> accident, HW reaction on all zero pointers can only be worse than using old, 
> but valid entry.
> 
> Due to no real documentation and Allwinner unwillingness to share details, 
> we'll probably never know what's best response for each error. Some things can 
> be deduced from vendor code, but not all.
> 
> I would rather not complicate this fix, especially since it's candidate for 
> backporting.

I am simply trying to highlight that this is not consistant with how the H264
part is done. Why do we reset the register for one codec and not the other ? 

Perhaps you should sync to your preference the driver as a whole. It also seems
that before your patch, some bits would be 0 and some other would be very large
values. Between this and leaving random value, I don't really see any gain or
reason for a backport. It neither break or fix anything as far as I understand. 

My general opinion, is that we fixe the unused address (like to 0) then when
something goes wrong, as least it will go wrong consistently.

> 
> Best regards,
> Jernej
> 
> > 
> > > +
> > > 
> > >  		cedrus_h265_frame_info_write_single(ctx, i, 
> dpb[i].field_pic,
> > >  		
> > >  						    
> pic_order_cnt,
> > >  						    
> buffer_index);
> 
>