| Message ID | 20201002210709.15688-1-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | 92233a63444001477acb70f2afa43a05f10b5fd5 |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/mobiclip: Check that Motion vectors are within the input frame | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On Fri, Oct 02, 2020 at 11:07:09PM +0200, Michael Niedermayer wrote: > The MV checks did not consider the width and height of the block, also they > had some off by 1 errors. This resulted in undefined behavior and crashes. > This commit instead errors out on these > > Fixes: out of array read > Fixes: 26080/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MOBICLIP_fuzzer-5758146355920896 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/mobiclip.c | 30 ++++++------------------------ > 1 file changed, 6 insertions(+), 24 deletions(-) > > diff --git a/libavcodec/mobiclip.c b/libavcodec/mobiclip.c > index f890cb2599..a03ef8fb7d 100644 > --- a/libavcodec/mobiclip.c > +++ b/libavcodec/mobiclip.c > @@ -1189,14 +1189,14 @@ static int predict_motion(AVCodecContext *avctx, > dst_linesize = s->pic[s->current_pic]->linesize[i]; > dst = s->pic[s->current_pic]->data[i] + offsetx + offsety * dst_linesize; > > + if (offsetx + (mv.x >> 1) < 0 || > + offsety + (mv.y >> 1) < 0 || > + offsetx + width + (mv.x + 1 >> 1) > fwidth || > + offsety + height + (mv.y + 1 >> 1) > fheight) > + return AVERROR_INVALIDDATA; > + > switch (method) { > case 0: > - if (offsety + (mv.y >> 1) < 0 || > - offsety + (mv.y >> 1) >= fheight || > - offsetx + (mv.x >> 1) < 0 || > - offsetx + (mv.x >> 1) >= fwidth) > - return AVERROR_INVALIDDATA; > - > src = s->pic[sidx]->data[i] + offsetx + (mv.x >> 1) + > (offsety + (mv.y >> 1)) * src_linesize; > for (int y = 0; y < height; y++) { > @@ -1207,12 +1207,6 @@ static int predict_motion(AVCodecContext *avctx, > } > break; > case 1: > - if (offsety + (mv.y >> 1) < 0 || > - offsety + (mv.y >> 1) >= fheight || > - offsetx + (mv.x >> 1) < 0 || > - offsetx + (mv.x >> 1) >= fwidth) > - return AVERROR_INVALIDDATA; > - > src = s->pic[sidx]->data[i] + offsetx + (mv.x >> 1) + > (offsety + (mv.y >> 1)) * src_linesize; > for (int y = 0; y < height; y++) { > @@ -1225,12 +1219,6 @@ static int predict_motion(AVCodecContext *avctx, > } > break; > case 2: > - if (offsety + (mv.y >> 1) < 0 || > - offsety + (mv.y >> 1) >= fheight - 1 || > - offsetx + (mv.x >> 1) < 0 || > - offsetx + (mv.x >> 1) >= fwidth) > - return AVERROR_INVALIDDATA; > - > src = s->pic[sidx]->data[i] + offsetx + (mv.x >> 1) + > (offsety + (mv.y >> 1)) * src_linesize; > for (int y = 0; y < height; y++) { > @@ -1243,12 +1231,6 @@ static int predict_motion(AVCodecContext *avctx, > } > break; > case 3: > - if (offsety + (mv.y >> 1) < 0 || > - offsety + (mv.y >> 1) >= fheight - 1 || > - offsetx + (mv.x >> 1) < 0 || > - offsetx + (mv.x >> 1) >= fwidth) > - return AVERROR_INVALIDDATA; > - You completely ignore this case, here it need one scanline extra. > src = s->pic[sidx]->data[i] + offsetx + (mv.x >> 1) + > (offsety + (mv.y >> 1)) * src_linesize; > for (int y = 0; y < height; y++) { > -- > 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".
On Sat, Oct 03, 2020 at 10:43:04AM +0200, Paul B Mahol wrote: > On Fri, Oct 02, 2020 at 11:07:09PM +0200, Michael Niedermayer wrote: > > The MV checks did not consider the width and height of the block, also they > > had some off by 1 errors. This resulted in undefined behavior and crashes. > > This commit instead errors out on these > > > > Fixes: out of array read > > Fixes: 26080/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MOBICLIP_fuzzer-5758146355920896 > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/mobiclip.c | 30 ++++++------------------------ > > 1 file changed, 6 insertions(+), 24 deletions(-) > > > > diff --git a/libavcodec/mobiclip.c b/libavcodec/mobiclip.c > > index f890cb2599..a03ef8fb7d 100644 > > --- a/libavcodec/mobiclip.c > > +++ b/libavcodec/mobiclip.c > > @@ -1189,14 +1189,14 @@ static int predict_motion(AVCodecContext *avctx, > > dst_linesize = s->pic[s->current_pic]->linesize[i]; > > dst = s->pic[s->current_pic]->data[i] + offsetx + offsety * dst_linesize; > > > > + if (offsetx + (mv.x >> 1) < 0 || > > + offsety + (mv.y >> 1) < 0 || > > + offsetx + width + (mv.x + 1 >> 1) > fwidth || > > + offsety + height + (mv.y + 1 >> 1) > fheight) > > + return AVERROR_INVALIDDATA; [...] > > @@ -1243,12 +1231,6 @@ static int predict_motion(AVCodecContext *avctx, > > } > > break; > > case 3: > > - if (offsety + (mv.y >> 1) < 0 || > > - offsety + (mv.y >> 1) >= fheight - 1 || > > - offsetx + (mv.x >> 1) < 0 || > > - offsetx + (mv.x >> 1) >= fwidth) > > - return AVERROR_INVALIDDATA; > > - > > You completely ignore this case, here it need one scanline extra. case 3 implies method= 3 implies mv.y is odd which makes (mv.y + 1 >> 1) 1 larger and that makes the test require 1 line more thx [...]
On Sat, Oct 03, 2020 at 04:46:37PM +0200, Michael Niedermayer wrote: > On Sat, Oct 03, 2020 at 10:43:04AM +0200, Paul B Mahol wrote: > > On Fri, Oct 02, 2020 at 11:07:09PM +0200, Michael Niedermayer wrote: > > > The MV checks did not consider the width and height of the block, also they > > > had some off by 1 errors. This resulted in undefined behavior and crashes. > > > This commit instead errors out on these > > > > > > Fixes: out of array read > > > Fixes: 26080/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MOBICLIP_fuzzer-5758146355920896 > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > --- > > > libavcodec/mobiclip.c | 30 ++++++------------------------ > > > 1 file changed, 6 insertions(+), 24 deletions(-) > > > > > > diff --git a/libavcodec/mobiclip.c b/libavcodec/mobiclip.c > > > index f890cb2599..a03ef8fb7d 100644 > > > --- a/libavcodec/mobiclip.c > > > +++ b/libavcodec/mobiclip.c > > > @@ -1189,14 +1189,14 @@ static int predict_motion(AVCodecContext *avctx, > > > dst_linesize = s->pic[s->current_pic]->linesize[i]; > > > dst = s->pic[s->current_pic]->data[i] + offsetx + offsety * dst_linesize; > > > > > > + if (offsetx + (mv.x >> 1) < 0 || > > > + offsety + (mv.y >> 1) < 0 || > > > + offsetx + width + (mv.x + 1 >> 1) > fwidth || > > > + offsety + height + (mv.y + 1 >> 1) > fheight) > > > + return AVERROR_INVALIDDATA; > [...] > > > > @@ -1243,12 +1231,6 @@ static int predict_motion(AVCodecContext *avctx, > > > } > > > break; > > > case 3: > > > - if (offsety + (mv.y >> 1) < 0 || > > > - offsety + (mv.y >> 1) >= fheight - 1 || > > > - offsetx + (mv.x >> 1) < 0 || > > > - offsetx + (mv.x >> 1) >= fwidth) > > > - return AVERROR_INVALIDDATA; > > > - > > > > You completely ignore this case, here it need one scanline extra. > > case 3 implies method= 3 implies mv.y is odd which makes > (mv.y + 1 >> 1) 1 larger and that makes the test require 1 line more will apply [...]
diff --git a/libavcodec/mobiclip.c b/libavcodec/mobiclip.c index f890cb2599..a03ef8fb7d 100644 --- a/libavcodec/mobiclip.c +++ b/libavcodec/mobiclip.c @@ -1189,14 +1189,14 @@ static int predict_motion(AVCodecContext *avctx, dst_linesize = s->pic[s->current_pic]->linesize[i]; dst = s->pic[s->current_pic]->data[i] + offsetx + offsety * dst_linesize; + if (offsetx + (mv.x >> 1) < 0 || + offsety + (mv.y >> 1) < 0 || + offsetx + width + (mv.x + 1 >> 1) > fwidth || + offsety + height + (mv.y + 1 >> 1) > fheight) + return AVERROR_INVALIDDATA; + switch (method) { case 0: - if (offsety + (mv.y >> 1) < 0 || - offsety + (mv.y >> 1) >= fheight || - offsetx + (mv.x >> 1) < 0 || - offsetx + (mv.x >> 1) >= fwidth) - return AVERROR_INVALIDDATA; - src = s->pic[sidx]->data[i] + offsetx + (mv.x >> 1) + (offsety + (mv.y >> 1)) * src_linesize; for (int y = 0; y < height; y++) { @@ -1207,12 +1207,6 @@ static int predict_motion(AVCodecContext *avctx, } break; case 1: - if (offsety + (mv.y >> 1) < 0 || - offsety + (mv.y >> 1) >= fheight || - offsetx + (mv.x >> 1) < 0 || - offsetx + (mv.x >> 1) >= fwidth) - return AVERROR_INVALIDDATA; - src = s->pic[sidx]->data[i] + offsetx + (mv.x >> 1) + (offsety + (mv.y >> 1)) * src_linesize; for (int y = 0; y < height; y++) { @@ -1225,12 +1219,6 @@ static int predict_motion(AVCodecContext *avctx, } break; case 2: - if (offsety + (mv.y >> 1) < 0 || - offsety + (mv.y >> 1) >= fheight - 1 || - offsetx + (mv.x >> 1) < 0 || - offsetx + (mv.x >> 1) >= fwidth) - return AVERROR_INVALIDDATA; - src = s->pic[sidx]->data[i] + offsetx + (mv.x >> 1) + (offsety + (mv.y >> 1)) * src_linesize; for (int y = 0; y < height; y++) { @@ -1243,12 +1231,6 @@ static int predict_motion(AVCodecContext *avctx, } break; case 3: - if (offsety + (mv.y >> 1) < 0 || - offsety + (mv.y >> 1) >= fheight - 1 || - offsetx + (mv.x >> 1) < 0 || - offsetx + (mv.x >> 1) >= fwidth) - return AVERROR_INVALIDDATA; - src = s->pic[sidx]->data[i] + offsetx + (mv.x >> 1) + (offsety + (mv.y >> 1)) * src_linesize; for (int y = 0; y < height; y++) {
The MV checks did not consider the width and height of the block, also they had some off by 1 errors. This resulted in undefined behavior and crashes. This commit instead errors out on these Fixes: out of array read Fixes: 26080/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MOBICLIP_fuzzer-5758146355920896 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/mobiclip.c | 30 ++++++------------------------ 1 file changed, 6 insertions(+), 24 deletions(-)