| Message ID | 20170702214327.26804-1-hp@tmm.cx |
|---|---|
| State | New |
| Headers | show |
On 7/2/17, Hein-Pieter van Braam <hp@tmm.cx> wrote: > Fixes: 6503 crash with fuzzed file > > Signed-off-by: Hein-Pieter van Braam <hp@tmm.cx> > --- > libavcodec/interplayvideo.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/libavcodec/interplayvideo.c b/libavcodec/interplayvideo.c > index d6f484a..86530e6 100644 > --- a/libavcodec/interplayvideo.c > +++ b/libavcodec/interplayvideo.c > @@ -972,6 +972,8 @@ static void > ipvideo_decode_format_06_opcodes(IpvideoContext *s, AVFrame *frame) > x, y, opcode, bytestream2_tell(&s->stream_ptr)); > > s->pixel_ptr = frame->data[0] + x + y * frame->linesize[0]; > + if (s->pixel_ptr > (s->pixel_ptr + > s->upper_motion_limit_offset)) > + return; This looks strange. > ipvideo_format_06_passes[pass](s, frame, opcode); > } > } > @@ -1043,6 +1045,12 @@ static void > ipvideo_decode_format_10_opcodes(IpvideoContext *s, AVFrame *frame) > for (y = 0; y < s->avctx->height; y += 8) { > for (x = 0; x < s->avctx->width; x += 8) { > s->pixel_ptr = s->cur_decode_frame->data[0] + x + y * > s->cur_decode_frame->linesize[0]; > + if (s->pixel_ptr > s->pixel_ptr + > s->upper_motion_limit_offset) > + return; This too. > + > + if (s->cur_decode_frame->width != s->avctx->width || > + s->cur_decode_frame->height != s->avctx->height) > + return; > > while (skip <= 0) { > if (skip != -0x8000 && skip) { > -- > 2.9.4 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Mon, 2017-07-03 at 11:43 +0200, Paul B Mahol wrote: > On 7/2/17, Hein-Pieter van Braam <hp@tmm.cx> wrote: > > Fixes: 6503 crash with fuzzed file > > > > Signed-off-by: Hein-Pieter van Braam <hp@tmm.cx> > > --- > > libavcodec/interplayvideo.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/libavcodec/interplayvideo.c > > b/libavcodec/interplayvideo.c > > index d6f484a..86530e6 100644 > > --- a/libavcodec/interplayvideo.c > > +++ b/libavcodec/interplayvideo.c > > @@ -972,6 +972,8 @@ static void > > ipvideo_decode_format_06_opcodes(IpvideoContext *s, AVFrame *frame) > > x, y, opcode, bytestream2_tell(&s- > > >stream_ptr)); > > > > s->pixel_ptr = frame->data[0] + x + y * frame- > > >linesize[0]; > > + if (s->pixel_ptr > (s->pixel_ptr + > > s->upper_motion_limit_offset)) > > + return; > > This looks strange. > The code already has a feature to ensure that movement vectors can't write past the end of the target AVFrame. I thought I'd reuse this to prevent writing past the end of the current AVFrame for the 'regular' pixeldata too. This code checks the value of the pointer into the AVFrame's pixeldata is not further than the last 8x8 block so that we can't write past it. Would there be a better way of doing this? Although, I suppose if I check the size of the AVFrame itself instead then it would also be safe because of the for loop. I can also implement it like that if that's preferred? - HP
On 7/3/17, Hein-Pieter van Braam <hp@tmm.cx> wrote: > On Mon, 2017-07-03 at 11:43 +0200, Paul B Mahol wrote: >> On 7/2/17, Hein-Pieter van Braam <hp@tmm.cx> wrote: >> > Fixes: 6503 crash with fuzzed file >> > >> > Signed-off-by: Hein-Pieter van Braam <hp@tmm.cx> >> > --- >> > libavcodec/interplayvideo.c | 8 ++++++++ >> > 1 file changed, 8 insertions(+) >> > >> > diff --git a/libavcodec/interplayvideo.c >> > b/libavcodec/interplayvideo.c >> > index d6f484a..86530e6 100644 >> > --- a/libavcodec/interplayvideo.c >> > +++ b/libavcodec/interplayvideo.c >> > @@ -972,6 +972,8 @@ static void >> > ipvideo_decode_format_06_opcodes(IpvideoContext *s, AVFrame *frame) >> > x, y, opcode, bytestream2_tell(&s- >> > >stream_ptr)); >> > >> > s->pixel_ptr = frame->data[0] + x + y * frame- >> > >linesize[0]; >> > + if (s->pixel_ptr > (s->pixel_ptr + >> > s->upper_motion_limit_offset)) >> > + return; >> >> This looks strange. >> > > The code already has a feature to ensure that movement vectors can't > write past the end of the target AVFrame. I thought I'd reuse this to > prevent writing past the end of the current AVFrame for the 'regular' > pixeldata too. This code checks the value of the pointer into the > AVFrame's pixeldata is not further than the last 8x8 block so that we > can't write past it. > > Would there be a better way of doing this? > > Although, I suppose if I check the size of the AVFrame itself instead > then it would also be safe because of the for loop. I can also > implement it like that if that's preferred? You are comparing pointer with same pointer increased by some value. This is wrong way to fix it.
On Mon, 2017-07-03 at 11:59 +0200, Paul B Mahol wrote: > > > You are comparing pointer with same pointer increased by some value. > > This is wrong way to fix it. Urgh, you're right, of course. I don't know what I was thinking.
diff --git a/libavcodec/interplayvideo.c b/libavcodec/interplayvideo.c index d6f484a..86530e6 100644 --- a/libavcodec/interplayvideo.c +++ b/libavcodec/interplayvideo.c @@ -972,6 +972,8 @@ static void ipvideo_decode_format_06_opcodes(IpvideoContext *s, AVFrame *frame) x, y, opcode, bytestream2_tell(&s->stream_ptr)); s->pixel_ptr = frame->data[0] + x + y * frame->linesize[0]; + if (s->pixel_ptr > (s->pixel_ptr + s->upper_motion_limit_offset)) + return; ipvideo_format_06_passes[pass](s, frame, opcode); } } @@ -1043,6 +1045,12 @@ static void ipvideo_decode_format_10_opcodes(IpvideoContext *s, AVFrame *frame) for (y = 0; y < s->avctx->height; y += 8) { for (x = 0; x < s->avctx->width; x += 8) { s->pixel_ptr = s->cur_decode_frame->data[0] + x + y * s->cur_decode_frame->linesize[0]; + if (s->pixel_ptr > s->pixel_ptr + s->upper_motion_limit_offset) + return; + + if (s->cur_decode_frame->width != s->avctx->width || + s->cur_decode_frame->height != s->avctx->height) + return; while (skip <= 0) { if (skip != -0x8000 && skip) {
Fixes: 6503 crash with fuzzed file Signed-off-by: Hein-Pieter van Braam <hp@tmm.cx> --- libavcodec/interplayvideo.c | 8 ++++++++ 1 file changed, 8 insertions(+)