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

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

[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@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;
+
     /* codeblock size as constrained by Rec. ITU-T T.800, Table A.18 */
     av_assert0(width <= 1024U && height <= 1024U);
     av_assert0(width * height <= 4096);
-- 
2.17.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 2/4] avcodec/vmixdec: Check shift before use

[Michael Niedermayer]

Fixes: shift exponent 32 is too large for 32-bit type 'unsigned int'
Fixes: 65909/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_VMIX_fuzzer-519459745831321

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/vmixdec.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/libavcodec/vmixdec.c b/libavcodec/vmixdec.c
index d6b6e3557f..ab283d13db 100644
--- a/libavcodec/vmixdec.c
+++ b/libavcodec/vmixdec.c
@@ -235,6 +235,9 @@ static int decode_frame(AVCodecContext *avctx,
     else if (offset != 3)
         return AVERROR_INVALIDDATA;
 
+    if (s->lshift > 31)
+        return AVERROR_INVALIDDATA;
+
     q = quality[FFMIN(avpkt->data[offset - 2], FF_ARRAY_ELEMS(quality)-1)];
     for (int n = 0; n < 64; n++)
         s->factors[n] = quant[n] * q;
-- 
2.17.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/4] avcodec/vmixdec: Check shift before use

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 738 bytes --]

On Wed, Mar 20, 2024 at 03:59:21AM +0100, Michael Niedermayer wrote:
> Fixes: shift exponent 32 is too large for 32-bit type 'unsigned int'
> Fixes: 65909/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_VMIX_fuzzer-519459745831321
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/vmixdec.c | 3 +++
>  1 file changed, 3 insertions(+)

will apply patches 2-4

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The smallest minority on earth is the individual. Those who deny 
individual rights cannot claim to be defenders of minorities. - Ayn Rand

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 3/4] tools/target_dec_fuzzer: Adjust RKA threshold up further

[Michael Niedermayer]

Fixes: timeout
Fixes: 66636/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RKA_fuzzer-5030913165557760

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 tools/target_dec_fuzzer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/target_dec_fuzzer.c b/tools/target_dec_fuzzer.c
index 57ba24a308..8d19988999 100644
--- a/tools/target_dec_fuzzer.c
+++ b/tools/target_dec_fuzzer.c
@@ -274,7 +274,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
     case AV_CODEC_ID_PAF_VIDEO:   maxpixels  /= 16;    break;
     case AV_CODEC_ID_PRORES:      maxpixels  /= 256;   break;
     case AV_CODEC_ID_QPEG:        maxpixels  /= 256;   break;
-    case AV_CODEC_ID_RKA:         maxsamples /= 65536; break;
+    case AV_CODEC_ID_RKA:         maxsamples /= 1<<20; break;
     case AV_CODEC_ID_RSCC:        maxpixels  /= 256;   break;
     case AV_CODEC_ID_RASC:        maxpixels  /= 16;    break;
     case AV_CODEC_ID_RTV1:        maxpixels  /= 16;    break;
-- 
2.17.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 4/4] avformat/id3v2: read_uslt() check for the amount read

[Michael Niedermayer]

Fixes: timeout
Fixes: 66783/clusterfuzz-testcase-minimized-ffmpeg_dem_GENH_fuzzer-5356884892647424

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/id3v2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
index d83716dcf4..e0a7e3f3ea 100644
--- a/libavformat/id3v2.c
+++ b/libavformat/id3v2.c
@@ -370,7 +370,7 @@ static void read_uslt(AVFormatContext *s, AVIOContext *pb, int taglen,
     int encoding;
     int ok = 0;
 
-    if (taglen < 1)
+    if (taglen < 4)
         goto error;
 
     encoding = avio_r8(pb);
-- 
2.17.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Tomas Härdin]

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@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.

While we're on the topic, it seems get_qcx() could be more strict when
it comes to the size of JPEG2000_QCD.

I see some TODOs around this stuff as well, init_band_stepsize()

/Tomas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1734 bytes --]

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

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[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@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

/Tomas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Tomas Härdin]

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

/Tomas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 2443 bytes --]

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

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The bravest are surely those who have the clearest vision
of what is before them, glory and danger alike, and yet
notwithstanding go out to meet it. -- Thucydides

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 2852 bytes --]

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

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The worst form of inequality is to try to make unequal things equal.
-- Aristotle

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Tomas Härdin]

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@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(). If you git grep for nguardbits you'll see
it's only ever set there when decoding, and similarly with expn. Coding
style and quantization style are not the same thing.

/Tomas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 4408 bytes --]

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

because the same get_qcx() is used both for "normal" jpeg2000 and htj2k
so the check needs to know if its htj2k

inside ff_jpeg2000_decode_htj2k() thats obvious, in get_qcx() its not
one can use the coding style to tell them apart but thats not trivial
with the various ways to slice and dice things
and it might not be set when get_qcx() is run

if iam missing something please tell me how to know in get_qcx() that
teh data is only used for ht2jk
(Its very possible iam missing something given i did not read the hundreads
 of pages of spec, and mostly assume that its as insane as possible)

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you think the mosad wants you dead since a long time then you are either
wrong or dead since a long time.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [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@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".