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

[James Almer]

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;
>  }