[FFmpeg-devel] [PATCH 2/3] avcodec/wmalosslessdec: Check channel mask against num channels

[James Almer]

On 3/29/2022 4:02 PM, Michael Niedermayer wrote:
> On Sat, Mar 19, 2022 at 06:52:34PM +0100, Michael Niedermayer wrote:
>> On Fri, Mar 18, 2022 at 08:38:28AM -0300, James Almer wrote:
>>>
>>>
>>> On 3/18/2022 8:27 AM, Michael Niedermayer wrote:
>>>> On Thu, Mar 17, 2022 at 10:00:18PM -0300, James Almer wrote:
>>>>>
>>>>>
>>>>> On 3/17/2022 9:07 PM, James Almer wrote:
>>>>>>
>>>>>>
>>>>>> On 3/17/2022 8:52 PM, James Almer wrote:
>>>>>>> On 3/17/2022 8:30 PM, Michael Niedermayer wrote:
>>>>>>>> Fixes: Out of array write
>>>>>>>> Fixes: 45613/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_WMALOSSLESS_fuzzer-4539073606320128
>>>>>>>>
>>>>>>>>
>>>>>>>> Found-by: continuous fuzzing process
>>>>>>>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>>>>>>> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
>>>>>>>> ---
>>>>>>>>     libavcodec/wmalosslessdec.c | 3 +++
>>>>>>>>     1 file changed, 3 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/libavcodec/wmalosslessdec.c b/libavcodec/wmalosslessdec.c
>>>>>>>> index cd05b22689..1728920729 100644
>>>>>>>> --- a/libavcodec/wmalosslessdec.c
>>>>>>>> +++ b/libavcodec/wmalosslessdec.c
>>>>>>>> @@ -281,6 +281,9 @@ static av_cold int
>>>>>>>> decode_init(AVCodecContext *avctx)
>>>>>>>>         av_channel_layout_uninit(&avctx->ch_layout);
>>>>>>>>         av_channel_layout_from_mask(&avctx->ch_layout, channel_mask);
>>>>>>>> +    if (s->num_channels != avctx->ch_layout.nb_channels)
>>>>>>>> +        return AVERROR_PATCHWELCOME; //are there non fuzzed
>>>>>>>> files with this or is it an error ?
>>>>>>>
>>>>>>> s->num_channels at this point is set to the channels count the user
>>>>>>> set before calling avcodec_open2() (Normally from lavf), but it
>>>>>>> could be anything.
>>>>>>> If channel_mask is taken from extradata, maybe it should be used to
>>>>>>> set s->num_channels instead of aborting because the user set value
>>>>>>> and extradata disagreed.
>>>>>>>
>>>>>>> Also, can you reproduce this crash before 3c933af493?
>>>>>>> s->num_channels was being set to the user set channel count too,
>>>>>>> same as now.
>>>>>>
>>>>>> Right, before that commit s->num_channels and avctx->channels were
>>>>>> always the same, but avctx->channel_layout was whatever came from
>>>>>> extradata, and its popcnt could be != avctx->channels.
>>>>>> After it, avctx->ch_layout.nb_channels is always the same as
>>>>>> popcnt(avctx->ch_layout.u.mask), which can be different than
>>>>>> s->num_channels.
>>>>>>
>>>>>> I think my suggestion above to use the extradata channel mask and
>>>>>> ignoring the user set channel count is the best approach for this.
>>>>>
>>>>> Like this maybe (channel_mask could in theory be zero, so in that case the
>>>>> user set value should be used).
>>>>>
>>>>>> diff --git a/libavcodec/wmalosslessdec.c b/libavcodec/wmalosslessdec.c
>>>>>> index cd05b22689..915add1962 100644
>>>>>> --- a/libavcodec/wmalosslessdec.c
>>>>>> +++ b/libavcodec/wmalosslessdec.c
>>>>>> @@ -197,15 +197,6 @@ static av_cold int decode_init(AVCodecContext *avctx)
>>>>>>            return AVERROR_PATCHWELCOME;
>>>>>>        }
>>>>>>
>>>>>> -    s->max_frame_size = MAX_FRAMESIZE * avctx->ch_layout.nb_channels;
>>>>>> -    s->frame_data = av_mallocz(s->max_frame_size + AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>> -    if (!s->frame_data)
>>>>>> -        return AVERROR(ENOMEM);
>>>>>> -
>>>>>> -    s->avctx = avctx;
>>>>>> -    ff_llauddsp_init(&s->dsp);
>>>>>> -    init_put_bits(&s->pb, s->frame_data, s->max_frame_size);
>>>>>> -
>>>>>>        if (avctx->extradata_size >= 18) {
>>>>>>            s->decode_flags    = AV_RL16(edata_ptr + 14);
>>>>>>            channel_mask       = AV_RL32(edata_ptr +  2);
>>>>>> @@ -230,6 +221,33 @@ static av_cold int decode_init(AVCodecContext *avctx)
>>>>>>            return AVERROR_PATCHWELCOME;
>>>>>>        }
>>>>>>
>>>>>> +    if (channel_mask) {
>>>>>> +        av_channel_layout_uninit(&avctx->ch_layout);
>>>>>> +        av_channel_layout_from_mask(&avctx->ch_layout, channel_mask);
>>>>>> +    } else
>>>>>> +        avctx->ch_layout.order = AV_CHANNEL_ORDER_UNSPEC;
>>>>>> +
>>>>>> +    s->num_channels = avctx->ch_layout.nb_channels;
>>>>>> +
>>>>>> +    /* extract lfe channel position */
>>>>>> +    s->lfe_channel = -1;
>>>>>> +
>>>>>> +    if (channel_mask & 8) {
>>>>>> +        unsigned int mask;
>>>>>> +        for (mask = 1; mask < 16; mask <<= 1)
>>>>>> +            if (channel_mask & mask)
>>>>>> +                ++s->lfe_channel;
>>>>>> +    }
>>>>>> +
>>>>>> +    s->max_frame_size = MAX_FRAMESIZE * avctx->ch_layout.nb_channels;
>>>>>> +    s->frame_data = av_mallocz(s->max_frame_size + AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>> +    if (!s->frame_data)
>>>>>> +        return AVERROR(ENOMEM);
>>>>>> +
>>>>>> +    s->avctx = avctx;
>>>>>> +    ff_llauddsp_init(&s->dsp);
>>>>>> +    init_put_bits(&s->pb, s->frame_data, s->max_frame_size);
>>>>>> +
>>>>>>        /* generic init */
>>>>>>        s->log2_frame_size = av_log2(avctx->block_align) + 4;
>>>>>>
>>>>>> @@ -263,24 +281,10 @@ static av_cold int decode_init(AVCodecContext *avctx)
>>>>>>            return AVERROR_INVALIDDATA;
>>>>>>        }
>>>>>>
>>>>>> -    s->num_channels = avctx->ch_layout.nb_channels;
>>>>>> -
>>>>>> -    /* extract lfe channel position */
>>>>>> -    s->lfe_channel = -1;
>>>>>> -
>>>>>> -    if (channel_mask & 8) {
>>>>>> -        unsigned int mask;
>>>>>> -        for (mask = 1; mask < 16; mask <<= 1)
>>>>>> -            if (channel_mask & mask)
>>>>>> -                ++s->lfe_channel;
>>>>>> -    }
>>>>>> -
>>>>>>        s->frame = av_frame_alloc();
>>>>>>        if (!s->frame)
>>>>>>            return AVERROR(ENOMEM);
>>>>>>
>>>>>> -    av_channel_layout_uninit(&avctx->ch_layout);
>>>>>> -    av_channel_layout_from_mask(&avctx->ch_layout, channel_mask);
>>>>>>        return 0;
>>>>>>    }
>>>>
>>>> this will change the output of the "lossless" decoder if this case ever
>>>> occurs besides this
>>>
>>> As i interpret it, you could initialize the decoder using any arbitrary
>>> number of channels in avctx. And if channel_mask in extradata is not zero,
>>> then it must be considered to be the valid channel count for the stream, and
>>> the value passed in avctx should be ignored.
>>
>> without a specification and no non fuzzed testfile its really not possible
>> to say for sure what this combination is intended to produce on the output
>>
>> What you say sounds reasonable but it can also be something else
> 
> ping, i think your patch or something similar should be applied
> The fuzzer found
> 46008/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_WMALOSSLESS_fuzzer-4681245747970048
> which too crashes because of the channel number
> 
> thx

Applied.