[alsa-devel] [PATCH v2] ASoC: dapm: fix out-of-bounds accesses to DAPM lookup tables
KASAN reports and additional traces point to out-of-bounds accesses to the dapm_up_seq and dapm_down_seq lookup tables. The indices used are larger than the array definition.
Fix by adding missing entries for the new widget types in these two lookup tables, and align them with PGA values.
Also the sequences for the following widgets were not defined. Since their values defaulted to zero, assign them explicitly
snd_soc_dapm_input snd_soc_dapm_output snd_soc_dapm_vmid snd_soc_dapm_siggen snd_soc_dapm_sink
Fixes: 8a70b4544ef4 ('ASoC: dapm: Add new widget type for constructing DAPM graphs on DSPs.'). Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- v2: fixed mistake with mic (was already handled) use values recommended by Mark
sound/soc/soc-dapm.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index 5b74dffc9c11..2bf5bab579de 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -70,12 +70,16 @@ static int dapm_up_seq[] = { [snd_soc_dapm_clock_supply] = 1, [snd_soc_dapm_supply] = 2, [snd_soc_dapm_micbias] = 3, + [snd_soc_dapm_vmid] = 3, [snd_soc_dapm_dai_link] = 2, [snd_soc_dapm_dai_in] = 4, [snd_soc_dapm_dai_out] = 4, [snd_soc_dapm_aif_in] = 4, [snd_soc_dapm_aif_out] = 4, [snd_soc_dapm_mic] = 5, + [snd_soc_dapm_siggen] = 5, + [snd_soc_dapm_input] = 5, + [snd_soc_dapm_output] = 5, [snd_soc_dapm_mux] = 6, [snd_soc_dapm_demux] = 6, [snd_soc_dapm_dac] = 7, @@ -83,11 +87,19 @@ static int dapm_up_seq[] = { [snd_soc_dapm_mixer] = 8, [snd_soc_dapm_mixer_named_ctl] = 8, [snd_soc_dapm_pga] = 9, + [snd_soc_dapm_buffer] = 9, + [snd_soc_dapm_scheduler] = 9, + [snd_soc_dapm_effect] = 9, + [snd_soc_dapm_src] = 9, + [snd_soc_dapm_asrc] = 9, + [snd_soc_dapm_encoder] = 9, + [snd_soc_dapm_decoder] = 9, [snd_soc_dapm_adc] = 10, [snd_soc_dapm_out_drv] = 11, [snd_soc_dapm_hp] = 11, [snd_soc_dapm_spk] = 11, [snd_soc_dapm_line] = 11, + [snd_soc_dapm_sink] = 11, [snd_soc_dapm_kcontrol] = 12, [snd_soc_dapm_post] = 13, }; @@ -100,13 +112,25 @@ static int dapm_down_seq[] = { [snd_soc_dapm_spk] = 3, [snd_soc_dapm_line] = 3, [snd_soc_dapm_out_drv] = 3, + [snd_soc_dapm_sink] = 3, [snd_soc_dapm_pga] = 4, + [snd_soc_dapm_buffer] = 4, + [snd_soc_dapm_scheduler] = 4, + [snd_soc_dapm_effect] = 4, + [snd_soc_dapm_src] = 4, + [snd_soc_dapm_asrc] = 4, + [snd_soc_dapm_encoder] = 4, + [snd_soc_dapm_decoder] = 4, [snd_soc_dapm_switch] = 5, [snd_soc_dapm_mixer_named_ctl] = 5, [snd_soc_dapm_mixer] = 5, [snd_soc_dapm_dac] = 6, [snd_soc_dapm_mic] = 7, + [snd_soc_dapm_siggen] = 7, + [snd_soc_dapm_input] = 7, + [snd_soc_dapm_output] = 7, [snd_soc_dapm_micbias] = 8, + [snd_soc_dapm_vmid] = 8, [snd_soc_dapm_mux] = 9, [snd_soc_dapm_demux] = 9, [snd_soc_dapm_aif_in] = 10,
On Mon, 04 Feb 2019 14:59:14 +0100, Pierre-Louis Bossart wrote:
KASAN reports and additional traces point to out-of-bounds accesses to the dapm_up_seq and dapm_down_seq lookup tables. The indices used are larger than the array definition.
Fix by adding missing entries for the new widget types in these two lookup tables, and align them with PGA values.
Also the sequences for the following widgets were not defined. Since their values defaulted to zero, assign them explicitly
snd_soc_dapm_input snd_soc_dapm_output snd_soc_dapm_vmid snd_soc_dapm_siggen snd_soc_dapm_sink
Fixes: 8a70b4544ef4 ('ASoC: dapm: Add new widget type for constructing DAPM graphs on DSPs.'). Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
v2: fixed mistake with mic (was already handled) use values recommended by Mark
This patch itself looks fine, but maybe a safer implementation would be to define snd_soc_dapm_max, and define dapm_up_seq[] as dapm_up_seq[snd_soc_dapm_max].
Also, another worthy change would be to set the priority non-zero, and trigger WARN_ON() if it hits a zero, i.e. undefined entry.
thanks,
Takashi
sound/soc/soc-dapm.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index 5b74dffc9c11..2bf5bab579de 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -70,12 +70,16 @@ static int dapm_up_seq[] = { [snd_soc_dapm_clock_supply] = 1, [snd_soc_dapm_supply] = 2, [snd_soc_dapm_micbias] = 3,
- [snd_soc_dapm_vmid] = 3, [snd_soc_dapm_dai_link] = 2, [snd_soc_dapm_dai_in] = 4, [snd_soc_dapm_dai_out] = 4, [snd_soc_dapm_aif_in] = 4, [snd_soc_dapm_aif_out] = 4, [snd_soc_dapm_mic] = 5,
- [snd_soc_dapm_siggen] = 5,
- [snd_soc_dapm_input] = 5,
- [snd_soc_dapm_output] = 5, [snd_soc_dapm_mux] = 6, [snd_soc_dapm_demux] = 6, [snd_soc_dapm_dac] = 7,
@@ -83,11 +87,19 @@ static int dapm_up_seq[] = { [snd_soc_dapm_mixer] = 8, [snd_soc_dapm_mixer_named_ctl] = 8, [snd_soc_dapm_pga] = 9,
- [snd_soc_dapm_buffer] = 9,
- [snd_soc_dapm_scheduler] = 9,
- [snd_soc_dapm_effect] = 9,
- [snd_soc_dapm_src] = 9,
- [snd_soc_dapm_asrc] = 9,
- [snd_soc_dapm_encoder] = 9,
- [snd_soc_dapm_decoder] = 9, [snd_soc_dapm_adc] = 10, [snd_soc_dapm_out_drv] = 11, [snd_soc_dapm_hp] = 11, [snd_soc_dapm_spk] = 11, [snd_soc_dapm_line] = 11,
- [snd_soc_dapm_sink] = 11, [snd_soc_dapm_kcontrol] = 12, [snd_soc_dapm_post] = 13,
}; @@ -100,13 +112,25 @@ static int dapm_down_seq[] = { [snd_soc_dapm_spk] = 3, [snd_soc_dapm_line] = 3, [snd_soc_dapm_out_drv] = 3,
- [snd_soc_dapm_sink] = 3, [snd_soc_dapm_pga] = 4,
- [snd_soc_dapm_buffer] = 4,
- [snd_soc_dapm_scheduler] = 4,
- [snd_soc_dapm_effect] = 4,
- [snd_soc_dapm_src] = 4,
- [snd_soc_dapm_asrc] = 4,
- [snd_soc_dapm_encoder] = 4,
- [snd_soc_dapm_decoder] = 4, [snd_soc_dapm_switch] = 5, [snd_soc_dapm_mixer_named_ctl] = 5, [snd_soc_dapm_mixer] = 5, [snd_soc_dapm_dac] = 6, [snd_soc_dapm_mic] = 7,
- [snd_soc_dapm_siggen] = 7,
- [snd_soc_dapm_input] = 7,
- [snd_soc_dapm_output] = 7, [snd_soc_dapm_micbias] = 8,
- [snd_soc_dapm_vmid] = 8, [snd_soc_dapm_mux] = 9, [snd_soc_dapm_demux] = 9, [snd_soc_dapm_aif_in] = 10,
-- 2.17.1
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
This patch itself looks fine, but maybe a safer implementation would be to define snd_soc_dapm_max, and define dapm_up_seq[] as dapm_up_seq[snd_soc_dapm_max].
Yes I thought about this but didn't know why the array was declared with an implicit length.
Also, another worthy change would be to set the priority non-zero, and trigger WARN_ON() if it hits a zero, i.e. undefined entry.
Unfortunately the zero is a legit value today, so we'd have to move all existing values by one. Not sure if it's worth it.
Maybe an alternate way to fix this is to define snd_soc_dapm_max and check if the ARRAY_SIZE of dapm_up_seq and dapm_down_seq match. That would trap any changes in the enum that isn't reflected in the _seq look-up tables.
On Mon, Feb 04, 2019 at 09:08:41AM -0600, Pierre-Louis Bossart wrote:
Yes I thought about this but didn't know why the array was declared with an implicit length.
It's been that way since the first commit (which predates me) so I don't think it was a super thought through decision.
Unfortunately the zero is a legit value today, so we'd have to move all existing values by one. Not sure if it's worth it.
It's not hard.
Maybe an alternate way to fix this is to define snd_soc_dapm_max and check if the ARRAY_SIZE of dapm_up_seq and dapm_down_seq match. That would trap any changes in the enum that isn't reflected in the _seq look-up tables.
We could do that and another thing together for maximum robustness!
On 2/4/19 10:11 AM, Mark Brown wrote:
On Mon, Feb 04, 2019 at 09:08:41AM -0600, Pierre-Louis Bossart wrote:
Yes I thought about this but didn't know why the array was declared with an implicit length.
It's been that way since the first commit (which predates me) so I don't think it was a super thought through decision.
Unfortunately the zero is a legit value today, so we'd have to move all existing values by one. Not sure if it's worth it.
It's not hard.
It's not hard indeed, I am just not hot on changing the values at the same time as adding new definitions. I managed to screw-up the mic values the first time, this would make the changes hard to check.
Maybe do this in a second patch, along with the array size checks?
Maybe an alternate way to fix this is to define snd_soc_dapm_max and check if the ARRAY_SIZE of dapm_up_seq and dapm_down_seq match. That would trap any changes in the enum that isn't reflected in the _seq look-up tables.
We could do that and another thing together for maximum robustness!
On Mon, Feb 04, 2019 at 10:20:10AM -0600, Pierre-Louis Bossart wrote:
It's not hard indeed, I am just not hot on changing the values at the same time as adding new definitions. I managed to screw-up the mic values the first time, this would make the changes hard to check.
Maybe do this in a second patch, along with the array size checks?
Sure.
participants (3)
-
Mark Brown -
Pierre-Louis Bossart -
Takashi Iwai