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

[James Almer]

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.

> 
> LGTM
> 
> thx

Will apply.