| Message ID | GV1P250MB07378F41C030CC0726A137AE8F529@GV1P250MB0737.EURP250.PROD.OUTLOOK.COM |
|---|---|
| State | Accepted |
| Commit | fff010591b35874b4c7a7e9fe00d7541f0b7c994 |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/jpeg2000dwt: Fix left shift of negative number | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
tis 2022-09-27 klockan 01:11 +0200 skrev Andreas Rheinhardt: > Fixes the j2k-dwt FATE-test; also fixes #9945. > (I don't know whether the multiplication can overflow.) The 5/3 transform is used in lossless mode and therefore shouldn't overflow for normal use cases. But someone can of course craft a malicious file > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavcodec/jpeg2000dwt.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavcodec/jpeg2000dwt.c b/libavcodec/jpeg2000dwt.c > index f2da7307c4..34e33553f7 100644 > --- a/libavcodec/jpeg2000dwt.c > +++ b/libavcodec/jpeg2000dwt.c > @@ -81,7 +81,7 @@ static void sd_1d53(int *p, int i0, int i1) > > if (i1 <= i0 + 1) { > if (i0 == 1) > - p[1] <<= 1; > + p[1] *= 2; To trigger an actual overflow here you need enough coefficient bits and enough decomposition levels, meaning also huge resolution. Resolution is capped at what 32k x 32k currently? That means you need 17-bit coefficients at the lowest levels to get over INT_MAX. I'm not actually sure what the limits for that in jpeg2000 is, but 12-bit lossless would certaily hit these levels at 5 or more decomp levels. I have samples that use 6, and it's easy to generate ones that have even more. To be really safe we'd need to use something like https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html and maybe define fallbacks for other compilers. This is part of why I've been harping about formal verification. /Tomas
Tomas Härdin: > tis 2022-09-27 klockan 01:11 +0200 skrev Andreas Rheinhardt: >> Fixes the j2k-dwt FATE-test; also fixes #9945. >> (I don't know whether the multiplication can overflow.) > > The 5/3 transform is used in lossless mode and therefore shouldn't > overflow for normal use cases. But someone can of course craft a > malicious file > >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >> --- >> libavcodec/jpeg2000dwt.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libavcodec/jpeg2000dwt.c b/libavcodec/jpeg2000dwt.c >> index f2da7307c4..34e33553f7 100644 >> --- a/libavcodec/jpeg2000dwt.c >> +++ b/libavcodec/jpeg2000dwt.c >> @@ -81,7 +81,7 @@ static void sd_1d53(int *p, int i0, int i1) >> >> if (i1 <= i0 + 1) { >> if (i0 == 1) >> - p[1] <<= 1; >> + p[1] *= 2; > > To trigger an actual overflow here you need enough coefficient bits and > enough decomposition levels, meaning also huge resolution. Resolution > is capped at what 32k x 32k currently? That means you need 17-bit > coefficients at the lowest levels to get over INT_MAX. I'm not actually > sure what the limits for that in jpeg2000 is, but 12-bit lossless would > certaily hit these levels at 5 or more decomp levels. I have samples > that use 6, and it's easy to generate ones that have even more. > FYI: This is not triggered by an actual jpeg2000 sample (not even a malicious one), this is triggered by the jpeg2000dwt test tool (libavcodec/tests/jpeg2000dwt.c). It might entirely be possible that the tool tests something that can't happen during decoding (and not only during decoding of spec-compliant files). > To be really safe we'd need to use something like > https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html > and maybe define fallbacks for other compilers. > Take a look at av_sat_add64_c() and similar functions. > This is part of why I've been harping about formal verification. >
tis 2022-09-27 klockan 13:40 +0200 skrev Andreas Rheinhardt: > Tomas Härdin: > > tis 2022-09-27 klockan 01:11 +0200 skrev Andreas Rheinhardt: > > > Fixes the j2k-dwt FATE-test; also fixes #9945. > > > (I don't know whether the multiplication can overflow.) > > > > The 5/3 transform is used in lossless mode and therefore shouldn't > > overflow for normal use cases. But someone can of course craft a > > malicious file > > > > > > > > Signed-off-by: Andreas Rheinhardt > > > <andreas.rheinhardt@outlook.com> > > > --- > > > libavcodec/jpeg2000dwt.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/libavcodec/jpeg2000dwt.c b/libavcodec/jpeg2000dwt.c > > > index f2da7307c4..34e33553f7 100644 > > > --- a/libavcodec/jpeg2000dwt.c > > > +++ b/libavcodec/jpeg2000dwt.c > > > @@ -81,7 +81,7 @@ static void sd_1d53(int *p, int i0, int i1) > > > > > > if (i1 <= i0 + 1) { > > > if (i0 == 1) > > > - p[1] <<= 1; > > > + p[1] *= 2; > > > > To trigger an actual overflow here you need enough coefficient bits > > and > > enough decomposition levels, meaning also huge resolution. > > Resolution > > is capped at what 32k x 32k currently? That means you need 17-bit > > coefficients at the lowest levels to get over INT_MAX. I'm not > > actually > > sure what the limits for that in jpeg2000 is, but 12-bit lossless > > would > > certaily hit these levels at 5 or more decomp levels. I have > > samples > > that use 6, and it's easy to generate ones that have even more. > > > > FYI: This is not triggered by an actual jpeg2000 sample (not even a > malicious one), this is triggered by the jpeg2000dwt test tool Yeah, I had the test uncover some interesting bugs on my end when developing, that probably don't happen with real files. But malicious files potentially triggering UB is something we shouldn't ignore > > To be really safe we'd need to use something like > > https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html > > and maybe define fallbacks for other compilers. > > > > Take a look at av_sat_add64_c() and similar functions. We don't need saturation here, only that the behavior is not undefined. Wrapping around is fine. The only place where saturation is performed is when converting decoded and idwt'd coefficients to actual pixel data in write_frame_*() It's possible that for sufficiently large 16-bit frames with enough decomposition levels that "lossless" encoding is not actually lossless unless the encoder uses 64-bit integers. j2kenc supports RGB48, nreslevels=7, which can run into this problem at resolutions as low as 255x255 I think. /Tomas
Tomas Härdin: > tis 2022-09-27 klockan 13:40 +0200 skrev Andreas Rheinhardt: >> Tomas Härdin: >>> tis 2022-09-27 klockan 01:11 +0200 skrev Andreas Rheinhardt: >>>> Fixes the j2k-dwt FATE-test; also fixes #9945. >>>> (I don't know whether the multiplication can overflow.) >>> >>> The 5/3 transform is used in lossless mode and therefore shouldn't >>> overflow for normal use cases. But someone can of course craft a >>> malicious file >>> >>>> >>>> Signed-off-by: Andreas Rheinhardt >>>> <andreas.rheinhardt@outlook.com> >>>> --- >>>> libavcodec/jpeg2000dwt.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/libavcodec/jpeg2000dwt.c b/libavcodec/jpeg2000dwt.c >>>> index f2da7307c4..34e33553f7 100644 >>>> --- a/libavcodec/jpeg2000dwt.c >>>> +++ b/libavcodec/jpeg2000dwt.c >>>> @@ -81,7 +81,7 @@ static void sd_1d53(int *p, int i0, int i1) >>>> >>>> if (i1 <= i0 + 1) { >>>> if (i0 == 1) >>>> - p[1] <<= 1; >>>> + p[1] *= 2; >>> >>> To trigger an actual overflow here you need enough coefficient bits >>> and >>> enough decomposition levels, meaning also huge resolution. >>> Resolution >>> is capped at what 32k x 32k currently? That means you need 17-bit >>> coefficients at the lowest levels to get over INT_MAX. I'm not >>> actually >>> sure what the limits for that in jpeg2000 is, but 12-bit lossless >>> would >>> certaily hit these levels at 5 or more decomp levels. I have >>> samples >>> that use 6, and it's easy to generate ones that have even more. >>> >> >> FYI: This is not triggered by an actual jpeg2000 sample (not even a >> malicious one), this is triggered by the jpeg2000dwt test tool > > Yeah, I had the test uncover some interesting bugs on my end when > developing, that probably don't happen with real files. But malicious > files potentially triggering UB is something we shouldn't ignore > >>> To be really safe we'd need to use something like >>> https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html >>> and maybe define fallbacks for other compilers. >>> >> >> Take a look at av_sat_add64_c() and similar functions. > > We don't need saturation here, only that the behavior is not undefined. Does this mean that this patch is ok? > Wrapping around is fine. The only place where saturation is performed > is when converting decoded and idwt'd coefficients to actual pixel data > in write_frame_*() > > It's possible that for sufficiently large 16-bit frames with enough > decomposition levels that "lossless" encoding is not actually lossless > unless the encoder uses 64-bit integers. > > j2kenc supports RGB48, nreslevels=7, which can run into this problem at > resolutions as low as 255x255 I think. >
tis 2022-09-27 klockan 21:20 +0200 skrev Andreas Rheinhardt: > Tomas Härdin: > > tis 2022-09-27 klockan 13:40 +0200 skrev Andreas Rheinhardt: > > > Tomas Härdin: > > > > tis 2022-09-27 klockan 01:11 +0200 skrev Andreas Rheinhardt: > > > > > Fixes the j2k-dwt FATE-test; also fixes #9945. > > > > > (I don't know whether the multiplication can overflow.) > > > > > > > > The 5/3 transform is used in lossless mode and therefore > > > > shouldn't > > > > overflow for normal use cases. But someone can of course craft > > > > a > > > > malicious file > > > > > > > > > > > > > > Signed-off-by: Andreas Rheinhardt > > > > > <andreas.rheinhardt@outlook.com> > > > > > --- > > > > > libavcodec/jpeg2000dwt.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/libavcodec/jpeg2000dwt.c > > > > > b/libavcodec/jpeg2000dwt.c > > > > > index f2da7307c4..34e33553f7 100644 > > > > > --- a/libavcodec/jpeg2000dwt.c > > > > > +++ b/libavcodec/jpeg2000dwt.c > > > > > @@ -81,7 +81,7 @@ static void sd_1d53(int *p, int i0, int i1) > > > > > > > > > > if (i1 <= i0 + 1) { > > > > > if (i0 == 1) > > > > > - p[1] <<= 1; > > > > > + p[1] *= 2; > > > > > > > > To trigger an actual overflow here you need enough coefficient > > > > bits > > > > and > > > > enough decomposition levels, meaning also huge resolution. > > > > Resolution > > > > is capped at what 32k x 32k currently? That means you need 17- > > > > bit > > > > coefficients at the lowest levels to get over INT_MAX. I'm not > > > > actually > > > > sure what the limits for that in jpeg2000 is, but 12-bit > > > > lossless > > > > would > > > > certaily hit these levels at 5 or more decomp levels. I have > > > > samples > > > > that use 6, and it's easy to generate ones that have even more. > > > > > > > > > > FYI: This is not triggered by an actual jpeg2000 sample (not even > > > a > > > malicious one), this is triggered by the jpeg2000dwt test tool > > > > Yeah, I had the test uncover some interesting bugs on my end when > > developing, that probably don't happen with real files. But > > malicious > > files potentially triggering UB is something we shouldn't ignore > > > > > > To be really safe we'd need to use something like > > > > https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html > > > > and maybe define fallbacks for other compilers. > > > > > > > > > > Take a look at av_sat_add64_c() and similar functions. > > > > We don't need saturation here, only that the behavior is not > > undefined. > > Does this mean that this patch is ok? Sure, it's better than before /Tomas
diff --git a/libavcodec/jpeg2000dwt.c b/libavcodec/jpeg2000dwt.c index f2da7307c4..34e33553f7 100644 --- a/libavcodec/jpeg2000dwt.c +++ b/libavcodec/jpeg2000dwt.c @@ -81,7 +81,7 @@ static void sd_1d53(int *p, int i0, int i1) if (i1 <= i0 + 1) { if (i0 == 1) - p[1] <<= 1; + p[1] *= 2; return; }
Fixes the j2k-dwt FATE-test; also fixes #9945. (I don't know whether the multiplication can overflow.) Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- libavcodec/jpeg2000dwt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)