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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <4725382.GXAFRqVoOG@kista>
Date:   Mon, 18 Jul 2022 19:57:49 +0200
From:   Jernej Škrabec <jernej.skrabec@...il.com>
To:     mripard@...nel.org, paul.kocialkowski@...tlin.com,
        Nicolas Dufresne <nicolas.dufresne@...labora.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

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.

Best regards,
Jernej

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