PR #21115 opened by James Almer (jamrial) URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/21115 Patch URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/21115.patch
An ASAN heap-buffer-overflow in scalable_channel_layout_config was caused by an unchecked assumption that the channel layout of a scalable audio layer is a superset of the previous layer's channel layout. `scalable_channel_layout_config` constructs a channel layout map by copying channels from the previous layer and adding new ones. The memory allocation is based on the target loudspeaker_layout. However, if the target layout doesn't encompass all previous channels (e.g., Mono to Stereo), copying previous channels followed by adding current ones could exceed the allocated size, causing a heap buffer overflow. This PR adds an exception for the know case of Mono -> Stereo, and a check to ensure the previous layer's channel layout is a subset of the current layer's layout by comparing their masks. If the condition isn't met, an error is returned. This supersedes #21107 >From ea3d8ad34d47dbf18ed0c4af47162263e1eb1db1 Mon Sep 17 00:00:00 2001 From: James Almer <[email protected]> Date: Fri, 5 Dec 2025 22:40:40 -0300 Subject: [PATCH 1/2] avformat/iamf_parse: fix parsing of Scalable layouts with Mono and Stereo layers And ASAN heap-buffer-overflow in scalable_channel_layout_config was caused by an unchecked assumption that the channel layout of a scalable audio layer is a superset of the previous layer's channel layout. scalable_channel_layout_config constructs a channel layout map by copying channels from the previous layer and adding new ones. The memory allocation is based on the target loudspeaker_layout. However, if the target layout doesn't encompass all previous channels (e.g., Mono to Stereo), copying previous channels followed by adding current ones could exceed the allocated size, causing a heap buffer overflow. This commit adds an exception for the know case of Mono -> Stereo, and a check to ensure the previous layer's channel layout is a subset of the current layer's layout by comparing their masks. If the condition isn't met, an error is returned. Co-authored-by: Oliver Chang <[email protected]> Signed-off-by: James Almer <[email protected]> --- libavformat/iamf_parse.c | 79 ++++++++++++++++++++++++---------------- 1 file changed, 47 insertions(+), 32 deletions(-) diff --git a/libavformat/iamf_parse.c b/libavformat/iamf_parse.c index 597d800be0..3d78533faf 100644 --- a/libavformat/iamf_parse.c +++ b/libavformat/iamf_parse.c @@ -347,6 +347,41 @@ static int update_extradata(AVCodecParameters *codecpar) return 0; } +static int parse_coupled_substream(AVChannelLayout *out, AVChannelLayout *in, int n) +{ + if (in->u.mask & AV_CH_LAYOUT_STEREO) { + out->u.map[n++].id = AV_CHAN_FRONT_LEFT; + out->u.map[n++].id = AV_CHAN_FRONT_RIGHT; + in->u.mask &= ~AV_CH_LAYOUT_STEREO; + } else if (in->u.mask & (AV_CH_FRONT_LEFT_OF_CENTER|AV_CH_FRONT_RIGHT_OF_CENTER)) { + out->u.map[n++].id = AV_CHAN_FRONT_LEFT_OF_CENTER; + out->u.map[n++].id = AV_CHAN_FRONT_RIGHT_OF_CENTER; + in->u.mask &= ~(AV_CH_FRONT_LEFT_OF_CENTER|AV_CH_FRONT_RIGHT_OF_CENTER); + } else if (in->u.mask & (AV_CH_SIDE_LEFT|AV_CH_SIDE_RIGHT)) { + out->u.map[n++].id = AV_CHAN_SIDE_LEFT; + out->u.map[n++].id = AV_CHAN_SIDE_RIGHT; + in->u.mask &= ~(AV_CH_SIDE_LEFT|AV_CH_SIDE_RIGHT); + } else if (in->u.mask & (AV_CH_BACK_LEFT|AV_CH_BACK_RIGHT)) { + out->u.map[n++].id = AV_CHAN_BACK_LEFT; + out->u.map[n++].id = AV_CHAN_BACK_RIGHT; + in->u.mask &= ~(AV_CH_BACK_LEFT|AV_CH_BACK_RIGHT); + } else if (in->u.mask & (AV_CH_TOP_FRONT_LEFT|AV_CH_TOP_FRONT_RIGHT)) { + out->u.map[n++].id = AV_CHAN_TOP_FRONT_LEFT; + out->u.map[n++].id = AV_CHAN_TOP_FRONT_RIGHT; + in->u.mask &= ~(AV_CH_TOP_FRONT_LEFT|AV_CH_TOP_FRONT_RIGHT); + } else if (in->u.mask & (AV_CH_TOP_SIDE_LEFT|AV_CH_TOP_SIDE_RIGHT)) { + out->u.map[n++].id = AV_CHAN_TOP_SIDE_LEFT; + out->u.map[n++].id = AV_CHAN_TOP_SIDE_RIGHT; + in->u.mask &= ~(AV_CH_TOP_SIDE_LEFT|AV_CH_TOP_SIDE_RIGHT); + } else if (in->u.mask & (AV_CH_TOP_BACK_LEFT|AV_CH_TOP_BACK_RIGHT)) { + out->u.map[n++].id = AV_CHAN_TOP_BACK_LEFT; + out->u.map[n++].id = AV_CHAN_TOP_BACK_RIGHT; + in->u.mask &= ~(AV_CH_TOP_BACK_LEFT|AV_CH_TOP_BACK_RIGHT); + } + + return n; +} + static int scalable_channel_layout_config(void *s, AVIOContext *pb, IAMFAudioElement *audio_element, const IAMFCodecConfig *codec_config) @@ -395,12 +430,19 @@ static int scalable_channel_layout_config(void *s, AVIOContext *pb, if (!i && loudspeaker_layout == 15) expanded_loudspeaker_layout = avio_r8(pb); - if (expanded_loudspeaker_layout > 0 && expanded_loudspeaker_layout < 13) { + if (expanded_loudspeaker_layout >= 0 && expanded_loudspeaker_layout < 13) { av_channel_layout_copy(&ch_layout, &ff_iamf_expanded_scalable_ch_layouts[expanded_loudspeaker_layout]); } else if (loudspeaker_layout < 10) { av_channel_layout_copy(&ch_layout, &ff_iamf_scalable_ch_layouts[loudspeaker_layout]); - if (i) - ch_layout.u.mask &= ~av_channel_layout_subset(&audio_element->element->layers[i-1]->ch_layout, UINT64_MAX); + if (i) { + uint64_t mask = av_channel_layout_subset(&audio_element->element->layers[i-1]->ch_layout, UINT64_MAX); + // When the first layer is Mono, the second layer may not have the C channel (e.g. Stereo) + if (audio_element->element->layers[i-1]->ch_layout.nb_channels == 1) + n--; + else if ((ch_layout.u.mask & mask) != mask) + return AVERROR_INVALIDDATA; + ch_layout.u.mask &= ~mask; + } } else ch_layout = (AVChannelLayout){ .order = AV_CHANNEL_ORDER_UNSPEC, .nb_channels = substream_count + @@ -430,38 +472,11 @@ static int scalable_channel_layout_config(void *s, AVIOContext *pb, coupled_substream_count = audio_element->layers[i].coupled_substream_count; while (coupled_substream_count--) { - if (ch_layout.u.mask & AV_CH_LAYOUT_STEREO) { - layer->ch_layout.u.map[n++].id = AV_CHAN_FRONT_LEFT; - layer->ch_layout.u.map[n++].id = AV_CHAN_FRONT_RIGHT; - ch_layout.u.mask &= ~AV_CH_LAYOUT_STEREO; - } else if (ch_layout.u.mask & (AV_CH_FRONT_LEFT_OF_CENTER|AV_CH_FRONT_RIGHT_OF_CENTER)) { - layer->ch_layout.u.map[n++].id = AV_CHAN_FRONT_LEFT_OF_CENTER; - layer->ch_layout.u.map[n++].id = AV_CHAN_FRONT_RIGHT_OF_CENTER; - ch_layout.u.mask &= ~(AV_CH_FRONT_LEFT_OF_CENTER|AV_CH_FRONT_RIGHT_OF_CENTER); - } else if (ch_layout.u.mask & (AV_CH_SIDE_LEFT|AV_CH_SIDE_RIGHT)) { - layer->ch_layout.u.map[n++].id = AV_CHAN_SIDE_LEFT; - layer->ch_layout.u.map[n++].id = AV_CHAN_SIDE_RIGHT; - ch_layout.u.mask &= ~(AV_CH_SIDE_LEFT|AV_CH_SIDE_RIGHT); - } else if (ch_layout.u.mask & (AV_CH_BACK_LEFT|AV_CH_BACK_RIGHT)) { - layer->ch_layout.u.map[n++].id = AV_CHAN_BACK_LEFT; - layer->ch_layout.u.map[n++].id = AV_CHAN_BACK_RIGHT; - ch_layout.u.mask &= ~(AV_CH_BACK_LEFT|AV_CH_BACK_RIGHT); - } else if (ch_layout.u.mask & (AV_CH_TOP_FRONT_LEFT|AV_CH_TOP_FRONT_RIGHT)) { - layer->ch_layout.u.map[n++].id = AV_CHAN_TOP_FRONT_LEFT; - layer->ch_layout.u.map[n++].id = AV_CHAN_TOP_FRONT_RIGHT; - ch_layout.u.mask &= ~(AV_CH_TOP_FRONT_LEFT|AV_CH_TOP_FRONT_RIGHT); - } else if (ch_layout.u.mask & (AV_CH_TOP_SIDE_LEFT|AV_CH_TOP_SIDE_RIGHT)) { - layer->ch_layout.u.map[n++].id = AV_CHAN_TOP_SIDE_LEFT; - layer->ch_layout.u.map[n++].id = AV_CHAN_TOP_SIDE_RIGHT; - ch_layout.u.mask &= ~(AV_CH_TOP_SIDE_LEFT|AV_CH_TOP_SIDE_RIGHT); - } else if (ch_layout.u.mask & (AV_CH_TOP_BACK_LEFT|AV_CH_TOP_BACK_RIGHT)) { - layer->ch_layout.u.map[n++].id = AV_CHAN_TOP_BACK_LEFT; - layer->ch_layout.u.map[n++].id = AV_CHAN_TOP_BACK_RIGHT; - ch_layout.u.mask &= ~(AV_CH_TOP_BACK_LEFT|AV_CH_TOP_BACK_RIGHT); - } + n = parse_coupled_substream(&layer->ch_layout, &ch_layout, n); } substream_count -= audio_element->layers[i].coupled_substream_count; + n = parse_coupled_substream(&layer->ch_layout, &ch_layout, n); // In case the first layer is Mono while (substream_count--) { if (ch_layout.u.mask & AV_CH_FRONT_CENTER) { layer->ch_layout.u.map[n++].id = AV_CHAN_FRONT_CENTER; -- 2.49.1 >From b319de8b62e475ca493ddf302ea995ec5e7535ad Mon Sep 17 00:00:00 2001 From: James Almer <[email protected]> Date: Fri, 5 Dec 2025 22:53:22 -0300 Subject: [PATCH 2/2] avformat/iamf_parse: add a few extra sanity checks Signed-off-by: James Almer <[email protected]> --- libavformat/iamf_parse.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/libavformat/iamf_parse.c b/libavformat/iamf_parse.c index 3d78533faf..756d9f0f09 100644 --- a/libavformat/iamf_parse.c +++ b/libavformat/iamf_parse.c @@ -418,7 +418,8 @@ static int scalable_channel_layout_config(void *s, AVIOContext *pb, substream_count = avio_r8(pb); coupled_substream_count = avio_r8(pb); - if (substream_count + k > audio_element->nb_substreams) + if (!substream_count || coupled_substream_count > substream_count || + substream_count + k > audio_element->nb_substreams) return AVERROR_INVALIDDATA; audio_element->layers[i].substream_count = substream_count; @@ -488,6 +489,9 @@ static int scalable_channel_layout_config(void *s, AVIOContext *pb, } } + if (n != ch_layout.nb_channels) + return AVERROR_INVALIDDATA; + ret = av_channel_layout_retype(&layer->ch_layout, AV_CHANNEL_ORDER_NATIVE, 0); if (ret < 0 && ret != AVERROR(ENOSYS)) return ret; -- 2.49.1 _______________________________________________ ffmpeg-devel mailing list -- [email protected] To unsubscribe send an email to [email protected]
