[FFmpeg-devel] [PATCH 1/4] avcodec/jpeg2000htdec: Check M_b / magp before using it in a shift

[Tomas Härdin]

tor 2024-03-28 klockan 03:48 +0100 skrev Michael Niedermayer:
> On Wed, Mar 27, 2024 at 11:13:48AM +0100, Tomas Härdin wrote:
> > mån 2024-03-25 klockan 21:04 +0100 skrev Michael Niedermayer:
> > > On Mon, Mar 25, 2024 at 08:13:13PM +0100, Michael Niedermayer
> > > wrote:
> > > > On Thu, Mar 21, 2024 at 04:07:14PM +0100, Tomas Härdin wrote:
> > > > > ons 2024-03-20 klockan 21:35 +0100 skrev Tomas Härdin:
> > > > > > ons 2024-03-20 klockan 14:12 +0100 skrev Michael
> > > > > > Niedermayer:
> > > > > > > On Wed, Mar 20, 2024 at 12:20:11PM +0100, Tomas Härdin
> > > > > > > wrote:
> > > > > > > > ons 2024-03-20 klockan 03:59 +0100 skrev Michael
> > > > > > > > Niedermayer:
> > > > > > > > > Fixes: shift exponent -1 is negative
> > > > > > > > > Fixes: 65378/clusterfuzz-testcase-minimized-
> > > > > > > > > ffmpeg_AV_CODEC_ID_JPEG2000_fuzzer-5457678193197056
> > > > > > > > > 
> > > > > > > > > Found-by: continuous fuzzing process
> > > > > > > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > > > > > > Signed-off-by: Michael Niedermayer
> > > > > > > > > <michael at niedermayer.cc>
> > > > > > > > > ---
> > > > > > > > >  libavcodec/jpeg2000htdec.c | 3 +++
> > > > > > > > >  1 file changed, 3 insertions(+)
> > > > > > > > > 
> > > > > > > > > diff --git a/libavcodec/jpeg2000htdec.c
> > > > > > > > > b/libavcodec/jpeg2000htdec.c
> > > > > > > > > index 6b9898d3ff..0b94bb5da2 100644
> > > > > > > > > --- a/libavcodec/jpeg2000htdec.c
> > > > > > > > > +++ b/libavcodec/jpeg2000htdec.c
> > > > > > > > > @@ -1193,6 +1193,9 @@ ff_jpeg2000_decode_htj2k(const
> > > > > > > > > Jpeg2000DecoderContext *s, Jpeg2000CodingStyle *c
> > > > > > > > >  
> > > > > > > > >      int32_t M_b = magp;
> > > > > > > > >  
> > > > > > > > > +    if (magp >= 31)
> > > > > > > > > +        return AVERROR_INVALIDDATA;
> > > > > > > > 
> > > > > > > > This isn't where the error is, assuming it even is an
> > > > > > > > error. It's
> > > > > > > > either expn or nguardbits that are wrong, and they
> > > > > > > > should
> > > > > > > > be
> > > > > > > > detected
> > > > > > > > and reported as such in jpeg2000dec.c. Checking this in
> > > > > > > > every
> > > > > > > > call
> > > > > > > > to
> > > > > > > > ff_jpeg2000_decode_htj2k() is wasteful.
> > > > > > > > 
> > > > > > > > nguardbits can be 0..7 and expn can be 0..31. Table
> > > > > > > > A.11
> > > > > > > > indicates
> > > > > > > > that
> > > > > > > > Ssize can be up to 38 bits, so M_b >= 31 is in fact
> > > > > > > > perfectly
> > > > > > > > valid.
> > > > > > > 
> > > > > > > > A
> > > > > > > > more appropriate error might be AVERROR_PATCHWELCOME.
> > > > > > > 
> > > > > > > indeed, i will change it to AVERROR_PATCHWELCOME
> > > > > > 
> > > > > > Please also move it further up so as to not waste cycles
> > > > > > checking it
> > > > > > every time
> > > > > 
> > > > > To be more precise, get_qcx() looks like the proper place for
> > > > > it
> > > > 
> > > > will apply with teh check moved there
> > > 
> > > the values that are causing undefined behavior for htj2k are used
> > > in
> > > normal
> > > j2k knowing which type of j2k we have seems decided by
> > > COC/COD/COX
> > > 
> > > so i dont think we can check in QCX, because a later COX could
> > > make it both invalid or valid
> > > and we cannot check in COX as a later QCX can similarly change it
> > 
> > That all calls get_qcx().
> 
> yes
> 
> 
> > If you git grep for nguardbits you'll see
> > it's only ever set there when decoding, and similarly with expn.
> 
> yes
> 
> 
> > Coding
> > style and quantization style are not the same thing.
> 
> yes
> 
> 
> but still, you can try to add a check the values for both nguardbits
> and expn which lead to undefined shifts in ff_jpeg2000_decode_htj2k()
> are used in normal jpeg2000 and break these samples

Hum hum.. You seem to be speaking of cblk->nonzerobits which is used in
decode_cblk() for the Part 1 decoder. It only accepts bpno in 0..29.
bpno in turn depends on roi_shift which is never negative.

  bpno = expn + nguardbits + roi_shift - 1 - zbp

But zbp can be up to 100 if I understand correctly, so expn + guardbits
can be up to 130 and still be legal Part 1. So yeah, putting the check
in get_qcx() isn't right. But it could live in tile_codeblocks() in the
bandno loop. As a bonus we only need to compute magp once per subband
(the compiler probably already notices this).

Interestingly roi_shift is sent to ff_jpeg2000_decode_htj2k() but never
used. T.814 seems to indicate ROI is still used for HTJ2K so maybe we
should warn if it's non-zero?

/Tomas