| Message ID | 20210406215022.13983-1-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/msp2dec: Check available space in RLE decoder | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
On Tue, Apr 06, 2021 at 11:50:22PM +0200, Michael Niedermayer wrote: > Fixes: out of array read > Fixes: 32968/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSP2_fuzzer-5315296027082752 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/msp2dec.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/msp2dec.c b/libavcodec/msp2dec.c > index cc548d218a..87057fb5e2 100644 > --- a/libavcodec/msp2dec.c > +++ b/libavcodec/msp2dec.c > @@ -68,9 +68,10 @@ static int msp2_decode_frame(AVCodecContext *avctx, > > bytestream2_init(&gb, buf, pkt_size); > x = 0; > - while (bytestream2_get_bytes_left(&gb) && x < width) { > + while (bytestream2_get_bytes_left(&gb) > 0 && x < width) { > int size = bytestream2_get_byte(&gb); > if (size) { > + size = FFMIN(size, bytestream2_get_bytes_left(&gb)); > memcpy(p->data[0] + y * p->linesize[0] + x, gb.buffer, FFMIN(size, width - x)); > bytestream2_skip(&gb, size); > } else { please apply -- Peter (A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)
Michael Niedermayer: > Fixes: out of array read > Fixes: 32968/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSP2_fuzzer-5315296027082752 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/msp2dec.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/msp2dec.c b/libavcodec/msp2dec.c > index cc548d218a..87057fb5e2 100644 > --- a/libavcodec/msp2dec.c > +++ b/libavcodec/msp2dec.c > @@ -68,9 +68,10 @@ static int msp2_decode_frame(AVCodecContext *avctx, > > bytestream2_init(&gb, buf, pkt_size); > x = 0; > - while (bytestream2_get_bytes_left(&gb) && x < width) { > + while (bytestream2_get_bytes_left(&gb) > 0 && x < width) { This decoder uses the checked bytestream2 API, so != 0 and > 0 should be equivalent for bytestream2_get_bytes_left(&gb). > int size = bytestream2_get_byte(&gb); > if (size) { > + size = FFMIN(size, bytestream2_get_bytes_left(&gb)); > memcpy(p->data[0] + y * p->linesize[0] + x, gb.buffer, FFMIN(size, width - x)); width can include seven bytes of the packet's padding, but it stays within the padding, so I wonder where the out of array read comes from. The only fishy thing in this decoder I see is that 2 * avctx->height might overflow. > bytestream2_skip(&gb, size); > } else { >
Andreas Rheinhardt: > Michael Niedermayer: >> Fixes: out of array read >> Fixes: 32968/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSP2_fuzzer-5315296027082752 >> >> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> --- >> libavcodec/msp2dec.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/libavcodec/msp2dec.c b/libavcodec/msp2dec.c >> index cc548d218a..87057fb5e2 100644 >> --- a/libavcodec/msp2dec.c >> +++ b/libavcodec/msp2dec.c >> @@ -68,9 +68,10 @@ static int msp2_decode_frame(AVCodecContext *avctx, >> >> bytestream2_init(&gb, buf, pkt_size); >> x = 0; >> - while (bytestream2_get_bytes_left(&gb) && x < width) { >> + while (bytestream2_get_bytes_left(&gb) > 0 && x < width) { > > This decoder uses the checked bytestream2 API, so != 0 and > 0 should be > equivalent for bytestream2_get_bytes_left(&gb). > >> int size = bytestream2_get_byte(&gb); >> if (size) { >> + size = FFMIN(size, bytestream2_get_bytes_left(&gb)); >> memcpy(p->data[0] + y * p->linesize[0] + x, gb.buffer, FFMIN(size, width - x)); > > width can include seven bytes of the packet's padding, but it stays > within the padding, so I wonder where the out of array read comes from. > The only fishy thing in this decoder I see is that 2 * avctx->height > might overflow. > Correction: width has no relationship with the packet's size at all; the width - x in the above FFMIN only ensures that it writes at most seven bytes into the frame's padding (is the frame actually guaranteed to be padded?). Btw: I don't see any advantage of writing in the frame's padding. - Andreas
On Wed, Apr 07, 2021 at 12:42:50AM +0200, Andreas Rheinhardt wrote: > Michael Niedermayer: > > Fixes: out of array read > > Fixes: 32968/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSP2_fuzzer-5315296027082752 > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/msp2dec.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/libavcodec/msp2dec.c b/libavcodec/msp2dec.c > > index cc548d218a..87057fb5e2 100644 > > --- a/libavcodec/msp2dec.c > > +++ b/libavcodec/msp2dec.c > > @@ -68,9 +68,10 @@ static int msp2_decode_frame(AVCodecContext *avctx, > > > > bytestream2_init(&gb, buf, pkt_size); > > x = 0; > > - while (bytestream2_get_bytes_left(&gb) && x < width) { > > + while (bytestream2_get_bytes_left(&gb) > 0 && x < width) { > > This decoder uses the checked bytestream2 API, so != 0 and > 0 should be > equivalent for bytestream2_get_bytes_left(&gb). I changed it to "> 0" because it felt clearer&more robust i will drop that as its not needed for the bugfix > > > int size = bytestream2_get_byte(&gb); > > if (size) { > > + size = FFMIN(size, bytestream2_get_bytes_left(&gb)); > > memcpy(p->data[0] + y * p->linesize[0] + x, gb.buffer, FFMIN(size, width - x)); > > width can include seven bytes of the packet's padding, but it stays > within the padding, so I wonder where the out of array read comes from. > The only fishy thing in this decoder I see is that 2 * avctx->height > might overflow. size is a value read from the bytestream, theres no guarntee that the amount that byte calls for is not beyond the end of the bytestream and its not checked by memcpy bytestream2_get_buffer() would check it and it can maybe be used instead thx [...]
On Wed, Apr 07, 2021 at 04:59:09PM +0200, Michael Niedermayer wrote: > On Wed, Apr 07, 2021 at 12:42:50AM +0200, Andreas Rheinhardt wrote: > > Michael Niedermayer: > > > Fixes: out of array read > > > Fixes: 32968/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSP2_fuzzer-5315296027082752 > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > --- > > > libavcodec/msp2dec.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/libavcodec/msp2dec.c b/libavcodec/msp2dec.c > > > index cc548d218a..87057fb5e2 100644 > > > --- a/libavcodec/msp2dec.c > > > +++ b/libavcodec/msp2dec.c > > > @@ -68,9 +68,10 @@ static int msp2_decode_frame(AVCodecContext *avctx, > > > > > > bytestream2_init(&gb, buf, pkt_size); > > > x = 0; > > > - while (bytestream2_get_bytes_left(&gb) && x < width) { > > > + while (bytestream2_get_bytes_left(&gb) > 0 && x < width) { > > > > This decoder uses the checked bytestream2 API, so != 0 and > 0 should be > > equivalent for bytestream2_get_bytes_left(&gb). > > I changed it to "> 0" because it felt clearer&more robust > i will drop that as its not needed for the bugfix > > > > > > > int size = bytestream2_get_byte(&gb); > > > if (size) { > > > + size = FFMIN(size, bytestream2_get_bytes_left(&gb)); > > > memcpy(p->data[0] + y * p->linesize[0] + x, gb.buffer, FFMIN(size, width - x)); > > > > width can include seven bytes of the packet's padding, but it stays > > within the padding, so I wonder where the out of array read comes from. > > The only fishy thing in this decoder I see is that 2 * avctx->height > > might overflow. > > size is a value read from the bytestream, theres no guarntee that > the amount that byte calls for is not beyond the end of the bytestream > and its not checked by memcpy will apply and backport [...]
On 4/7/2021 11:59 AM, Michael Niedermayer wrote: > On Wed, Apr 07, 2021 at 12:42:50AM +0200, Andreas Rheinhardt wrote: >> Michael Niedermayer: >>> Fixes: out of array read >>> Fixes: 32968/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSP2_fuzzer-5315296027082752 >>> >>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>> --- >>> libavcodec/msp2dec.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/libavcodec/msp2dec.c b/libavcodec/msp2dec.c >>> index cc548d218a..87057fb5e2 100644 >>> --- a/libavcodec/msp2dec.c >>> +++ b/libavcodec/msp2dec.c >>> @@ -68,9 +68,10 @@ static int msp2_decode_frame(AVCodecContext *avctx, >>> >>> bytestream2_init(&gb, buf, pkt_size); >>> x = 0; >>> - while (bytestream2_get_bytes_left(&gb) && x < width) { >>> + while (bytestream2_get_bytes_left(&gb) > 0 && x < width) { >> >> This decoder uses the checked bytestream2 API, so != 0 and > 0 should be >> equivalent for bytestream2_get_bytes_left(&gb). > > I changed it to "> 0" because it felt clearer&more robust > i will drop that as its not needed for the bugfix > > >> >>> int size = bytestream2_get_byte(&gb); >>> if (size) { >>> + size = FFMIN(size, bytestream2_get_bytes_left(&gb)); >>> memcpy(p->data[0] + y * p->linesize[0] + x, gb.buffer, FFMIN(size, width - x)); >> >> width can include seven bytes of the packet's padding, but it stays >> within the padding, so I wonder where the out of array read comes from. >> The only fishy thing in this decoder I see is that 2 * avctx->height >> might overflow. > > size is a value read from the bytestream, theres no guarntee that > the amount that byte calls for is not beyond the end of the bytestream > and its not checked by memcpy > > bytestream2_get_buffer() would check it and it can maybe be used instead If you knew about it, why didn't you use it? This code here basically duplicates the three lines in bytestream2_get_buffer(). > > thx > > [...] > > > _______________________________________________ > 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". >
On Fri, Apr 09, 2021 at 10:59:44PM -0300, James Almer wrote: > On 4/7/2021 11:59 AM, Michael Niedermayer wrote: > > On Wed, Apr 07, 2021 at 12:42:50AM +0200, Andreas Rheinhardt wrote: > > > Michael Niedermayer: > > > > Fixes: out of array read > > > > Fixes: 32968/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSP2_fuzzer-5315296027082752 > > > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > --- > > > > libavcodec/msp2dec.c | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/libavcodec/msp2dec.c b/libavcodec/msp2dec.c > > > > index cc548d218a..87057fb5e2 100644 > > > > --- a/libavcodec/msp2dec.c > > > > +++ b/libavcodec/msp2dec.c > > > > @@ -68,9 +68,10 @@ static int msp2_decode_frame(AVCodecContext *avctx, > > > > bytestream2_init(&gb, buf, pkt_size); > > > > x = 0; > > > > - while (bytestream2_get_bytes_left(&gb) && x < width) { > > > > + while (bytestream2_get_bytes_left(&gb) > 0 && x < width) { > > > > > > This decoder uses the checked bytestream2 API, so != 0 and > 0 should be > > > equivalent for bytestream2_get_bytes_left(&gb). > > > > I changed it to "> 0" because it felt clearer&more robust > > i will drop that as its not needed for the bugfix > > > > > > > > > > > int size = bytestream2_get_byte(&gb); > > > > if (size) { > > > > + size = FFMIN(size, bytestream2_get_bytes_left(&gb)); > > > > memcpy(p->data[0] + y * p->linesize[0] + x, gb.buffer, FFMIN(size, width - x)); > > > > > > width can include seven bytes of the packet's padding, but it stays > > > within the padding, so I wonder where the out of array read comes from. > > > The only fishy thing in this decoder I see is that 2 * avctx->height > > > might overflow. > > > > size is a value read from the bytestream, theres no guarntee that > > the amount that byte calls for is not beyond the end of the bytestream > > and its not checked by memcpy > > > > bytestream2_get_buffer() would check it and it can maybe be used instead > > If you knew about it, why didn't you use it? This code here basically > duplicates the three lines in bytestream2_get_buffer(). why i didnt use it, well ultimatly probable because i had a bugfix that i tested and that solved the issue and that was easy to backport. And noone asked me to include the simplification. Of course no question when looking at it now, its prettier to replace these 3 lines by bytestream2_get_buffer(). Thats what review is for, to spot these things. Which it now did. Feel free to replace it or ill do it tomorrow if i dont forget thx [...]
在 2021/4/11 上午5:21, Michael Niedermayer 写道: > On Fri, Apr 09, 2021 at 10:59:44PM -0300, James Almer wrote: >> On 4/7/2021 11:59 AM, Michael Niedermayer wrote: >>> On Wed, Apr 07, 2021 at 12:42:50AM +0200, Andreas Rheinhardt wrote: >>>> Michael Niedermayer: >>>>> Fixes: out of array read >>>>> Fixes: 32968/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSP2_fuzzer-5315296027082752 >>>>> >>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>>>> --- >>>>> libavcodec/msp2dec.c | 3 ++- >>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/libavcodec/msp2dec.c b/libavcodec/msp2dec.c >>>>> index cc548d218a..87057fb5e2 100644 >>>>> --- a/libavcodec/msp2dec.c >>>>> +++ b/libavcodec/msp2dec.c >>>>> @@ -68,9 +68,10 @@ static int msp2_decode_frame(AVCodecContext *avctx, >>>>> bytestream2_init(&gb, buf, pkt_size); >>>>> x = 0; >>>>> - while (bytestream2_get_bytes_left(&gb) && x < width) { >>>>> + while (bytestream2_get_bytes_left(&gb) > 0 && x < width) { >>>> This decoder uses the checked bytestream2 API, so != 0 and > 0 should be >>>> equivalent for bytestream2_get_bytes_left(&gb). >>> I changed it to "> 0" because it felt clearer&more robust >>> i will drop that as its not needed for the bugfix >>> >>> >>>>> int size = bytestream2_get_byte(&gb); >>>>> if (size) { >>>>> + size = FFMIN(size, bytestream2_get_bytes_left(&gb)); >>>>> memcpy(p->data[0] + y * p->linesize[0] + x, gb.buffer, FFMIN(size, width - x)); >>>> width can include seven bytes of the packet's padding, but it stays >>>> within the padding, so I wonder where the out of array read comes from. >>>> The only fishy thing in this decoder I see is that 2 * avctx->height >>>> might overflow. >>> size is a value read from the bytestream, theres no guarntee that >>> the amount that byte calls for is not beyond the end of the bytestream >>> and its not checked by memcpy >>> >>> bytestream2_get_buffer() would check it and it can maybe be used instead >> If you knew about it, why didn't you use it? This code here basically >> duplicates the three lines in bytestream2_get_buffer(). > why i didnt use it, well ultimatly probable because i had a bugfix that > i tested and that solved the issue and that was easy to backport. And > noone asked me to include the simplification. > Of course no question when looking at it now, its prettier to replace > these 3 lines by bytestream2_get_buffer(). > Thats what review is for, to spot these things. Which it now did. > Feel free to replace it or ill do it tomorrow if i dont forget > > thx > > [...] > > _______________________________________________ > 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".
diff --git a/libavcodec/msp2dec.c b/libavcodec/msp2dec.c index cc548d218a..87057fb5e2 100644 --- a/libavcodec/msp2dec.c +++ b/libavcodec/msp2dec.c @@ -68,9 +68,10 @@ static int msp2_decode_frame(AVCodecContext *avctx, bytestream2_init(&gb, buf, pkt_size); x = 0; - while (bytestream2_get_bytes_left(&gb) && x < width) { + while (bytestream2_get_bytes_left(&gb) > 0 && x < width) { int size = bytestream2_get_byte(&gb); if (size) { + size = FFMIN(size, bytestream2_get_bytes_left(&gb)); memcpy(p->data[0] + y * p->linesize[0] + x, gb.buffer, FFMIN(size, width - x)); bytestream2_skip(&gb, size); } else {
Fixes: out of array read Fixes: 32968/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSP2_fuzzer-5315296027082752 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/msp2dec.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)