[FFmpeg-devel] [PATCH v2] avcodec/mv30: Check the input length before allocation

[Michael Niedermayer]

On Thu, Aug 10, 2023 at 05:58:24PM +0200, Paul B Mahol wrote:
> On Thu, Aug 10, 2023 at 5:46 PM Michael Niedermayer <michael at niedermayer.cc>
> wrote:
> 
> > On Thu, Aug 10, 2023 at 12:12:51PM +0200, Paul B Mahol wrote:
> > > On Thu, Aug 10, 2023 at 11:34 AM Michael Niedermayer <
> > michael at niedermayer.cc>
> > > wrote:
> > >
> > > > On Wed, Aug 09, 2023 at 11:20:43PM +0200, Paul B Mahol wrote:
> > > > > On Wed, Aug 9, 2023 at 9:30 PM Michael Niedermayer <
> > > > michael at niedermayer.cc>
> > > > > wrote:
> > > > >
> > > > > > Hi Paul
> > > > > >
> > > > > > On Wed, Aug 09, 2023 at 08:53:03PM +0200, Paul B Mahol wrote:
> > > > > > > This is not correct, and please stop writing such patches.
> > Thanks.
> > > > > >
> > > > > > If there is a problem in the bugfix, please explain what the
> > problem
> > > > is.
> > > > > > If you do not like the specific fix, you can fix it differently
> > too or
> > > > > > tell me what you prefer.
> > > > > > Simply saying "no" with no explanation repeatedly is rude
> > > > > >
> > > > >
> > > > > Patch breaks valid files.
> > > >
> > > > Does the patch break files you create intentionally or files
> > > > pre-existing ?
> > > > The check can fail if 2 data segments overlap, one can craft
> > > > a file with that. The previous patches are implemented differently
> > > > and dont have that behavior, you rejected them too and at the time
> > > > you did call them "hacky" and did not mention that they break anything
> > > > and also ignored all further questions
> > > >
> > > > I just implemented this one differently as the other way was rejected
> > > > by you with no comment
> > > >
> > > > Also please provide the files this breaks so the issue can be
> > > > fixed
> > > >
> > > >
> > > Why not same thing for AV1 codec?
> > > Just reduce max resolutions for mv30 to 32x32 and be done.
> >
> > Limiting the resolution to max 32x32 would break real samples
> > for example V-codecs/mv30.avi
> >
> > if you suggest to limit it only for the fuzzer, well, that would not
> > fix the timeout outside the fuzzer.
> > For some decoders limiting the resolution in the fuzzer is the only
> > practical
> > option. But for mv30 this timeout really occurs because the input is not
> > checked/validated.
> >
> 
> But mv30 bitstream is not so trivial and can not be checked that easily
> with your patches.

for decode_intra the loop reads from mgb look like this:

    for (int y = 0; y < avctx->height; y += 16) {
...
        for (int x = 0; x < avctx->width; x += 16) {
...
            for (int b = 0; b < 6; b++) {
                int mode = get_bits_le(&mgb, 2);
...
            }
        }

the only way to escape this loop is through error exit
so 2*6* number of 16x16 blocks is read unconditionally
and its read from a place that has s->mode_size * 8 bits
alternatively get_bits_left() can be used


for decode inter:
there is this at the begin
    mask = *gb;
    skip_bits_long(gb, mask_size * 8);
    mgb = *gb;
    skip_bits_long(gb, s->mode_size * 8);

so we have at minimum (mask_size + s->mode_size) * 8 bits

It does seem reasonable to me to check the input to contain enough bits
for what is consumed
If it breaks some real files, i very much would like to know what these
files are doing


> 
> Also whole fuzzing idea of your work is flawed.

what do you mean by "whole fuzzing idea" ?

thx

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

Frequently ignored answer#1 FFmpeg bugs should be sent to our bugtracker. User
questions about the command line tools should be sent to the ffmpeg-user ML.
And questions about how to use libav* should be sent to the libav-user ML.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20230812/466cc5ea/attachment.sig>