[PATCH 0/4] ASoC: Intel: byt*: Avoid OOB array read from the map name

[PATCH 0/4] ASoC: Intel: byt*: Avoid OOB array read from the map name

[Takashi Iwai]

While reading bug reports, I casually stubmled on a UBSAN warning
about the array OOB access, and this looks like a real bug in ASoC
Intel driver code.  So here is a series of quick fixes for them.


Takashi

====

Takashi Iwai (4):
  ASoC: Intel: bytcht_es8316: Avoid OOB array read from the map name
  ASoC: Intel: bytcr_rt5640: Avoid OOB array read from the map name
  ASoC: Intel: bytcr_rt5651: Avoid OOB array read from the map name
  ASoC: Intel: bytcr_wm5102: Avoid OOB array read from the map name

 sound/soc/intel/boards/bytcht_es8316.c | 11 ++++++++---
 sound/soc/intel/boards/bytcr_rt5640.c  | 11 ++++++++---
 sound/soc/intel/boards/bytcr_rt5651.c  | 11 ++++++++---
 sound/soc/intel/boards/bytcr_wm5102.c  | 27 ++++++++++++++++++++------
 4 files changed, 45 insertions(+), 15 deletions(-)

-- 
2.49.0

[PATCH 1/4] ASoC: Intel: bytcht_es8316: Avoid OOB array read from the map name

[Takashi Iwai]

Since the quirk bits can be passed arbitrarily via a module option, it
may lead to the OOB access over the map name string array.
Fix it by conditionally reading the array, instead.

Fixes: 249d2fc9e55c ("ASoC: Intel: bytcht_es8316: Set card long_name based on quirks")
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/intel/boards/bytcht_es8316.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/sound/soc/intel/boards/bytcht_es8316.c b/sound/soc/intel/boards/bytcht_es8316.c
index 62594e7966ab..808142250ca1 100644
--- a/sound/soc/intel/boards/bytcht_es8316.c
+++ b/sound/soc/intel/boards/bytcht_es8316.c
@@ -518,10 +518,15 @@ static int byt_cht_es8316_get_quirks_from_dsm(struct byt_cht_es8316_private *pri
 	return 0;
 }
 
+static const char *mic_name(int n)
+{
+	static const char * const mic_names[] = { "in1", "in2" };
+	return n < ARRAY_SIZE(mic_names) ? mic_names[n] : "none";
+}
+
 static int snd_byt_cht_es8316_mc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	static const char * const mic_name[] = { "in1", "in2" };
 	struct snd_soc_acpi_mach *mach = dev_get_platdata(dev);
 	struct property_entry props[MAX_NO_PROPS] = {};
 	struct byt_cht_es8316_private *priv;
@@ -648,12 +653,12 @@ static int snd_byt_cht_es8316_mc_probe(struct platform_device *pdev)
 	snprintf(components_string, sizeof(components_string),
 		 "cfg-spk:%s cfg-mic:%s",
 		 (quirk & BYT_CHT_ES8316_MONO_SPEAKER) ? "1" : "2",
-		 mic_name[BYT_CHT_ES8316_MAP(quirk)]);
+		 mic_name(BYT_CHT_ES8316_MAP(quirk)));
 	byt_cht_es8316_card.components = components_string;
 #if !IS_ENABLED(CONFIG_SND_SOC_INTEL_USER_FRIENDLY_LONG_NAMES)
 	snprintf(long_name, sizeof(long_name), "bytcht-es8316-%s-spk-%s-mic",
 		 (quirk & BYT_CHT_ES8316_MONO_SPEAKER) ? "mono" : "stereo",
-		 mic_name[BYT_CHT_ES8316_MAP(quirk)]);
+		 mic_name(BYT_CHT_ES8316_MAP(quirk)));
 	byt_cht_es8316_card.long_name = long_name;
 #endif
 
-- 
2.49.0

[PATCH 2/4] ASoC: Intel: bytcr_rt5640: Avoid OOB array read from the map name

[Takashi Iwai]

Since the quirk bits can be passed arbitrarily via a module option, it
may lead to the OOB access over the map name string array.
Fix it by conditionally reading the array, instead.

Fixes: 063422ca2a9d ("ASoC: Intel: bytcr_rt5640: Set card long_name based on quirks")
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/intel/boards/bytcr_rt5640.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/sound/soc/intel/boards/bytcr_rt5640.c b/sound/soc/intel/boards/bytcr_rt5640.c
index 6446cda0f857..1ba9caa272c0 100644
--- a/sound/soc/intel/boards/bytcr_rt5640.c
+++ b/sound/soc/intel/boards/bytcr_rt5640.c
@@ -1676,10 +1676,15 @@ struct acpi_chan_package {   /* ACPICA seems to require 64 bit integers */
 	u64 mclock_value;    /* usually 25MHz (0x17d7940), ignored */
 };
 
+static const char *map_name(int n)
+{
+	static const char * const map_names[] = { "dmic1", "dmic2", "in1", "in3" };
+	return n < ARRAY_SIZE(map_names) ? map_names[n] : "none";
+}
+
 static int snd_byt_rt5640_mc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	static const char * const map_name[] = { "dmic1", "dmic2", "in1", "in3", "none" };
 	struct snd_soc_acpi_mach *mach = dev_get_platdata(dev);
 	__maybe_unused const char *spk_type;
 	const struct dmi_system_id *dmi_id;
@@ -1903,13 +1908,13 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev)
 
 	snprintf(byt_rt5640_components, sizeof(byt_rt5640_components),
 		 "cfg-spk:%s cfg-mic:%s aif:%d%s%s", cfg_spk,
-		 map_name[BYT_RT5640_MAP(byt_rt5640_quirk)], aif,
+		 map_name(BYT_RT5640_MAP(byt_rt5640_quirk)), aif,
 		 lineout_string, headset2_string);
 	byt_rt5640_card.components = byt_rt5640_components;
 #if !IS_ENABLED(CONFIG_SND_SOC_INTEL_USER_FRIENDLY_LONG_NAMES)
 	snprintf(byt_rt5640_long_name, sizeof(byt_rt5640_long_name),
 		 "bytcr-rt5640-%s-spk-%s-mic", spk_type,
-		 map_name[BYT_RT5640_MAP(byt_rt5640_quirk)]);
+		 map_name(BYT_RT5640_MAP(byt_rt5640_quirk)));
 	byt_rt5640_card.long_name = byt_rt5640_long_name;
 #endif
 
-- 
2.49.0

[PATCH 3/4] ASoC: Intel: bytcr_rt5651: Avoid OOB array read from the map name

[Takashi Iwai]

Since the quirk bits can be passed arbitrarily via a module option, it
may lead to the OOB access over the map name string array.
Fix it by conditionally reading the array, instead.

Fixes: 64484ccee7af ("ASoC: Intel: bytcr_rt5651: Set card long_name based on quirks")
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/intel/boards/bytcr_rt5651.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c
index 67c62844ca2a..53f64f01fb22 100644
--- a/sound/soc/intel/boards/bytcr_rt5651.c
+++ b/sound/soc/intel/boards/bytcr_rt5651.c
@@ -881,10 +881,15 @@ struct acpi_chan_package {   /* ACPICA seems to require 64 bit integers */
 	u64 mclock_value;    /* usually 25MHz (0x17d7940), ignored */
 };
 
+static const char *mic_name(int n)
+{
+	static const char * const mic_names[] = { "dmic", "in1", "in2", "in12" };
+	return n < ARRAY_SIZE(mic_names) ? mic_names[n] : "none";
+}
+
 static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	static const char * const mic_name[] = { "dmic", "in1", "in2", "in12" };
 	struct snd_soc_acpi_mach *mach = dev_get_platdata(dev);
 	struct byt_rt5651_private *priv;
 	const char *platform_name;
@@ -1071,7 +1076,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 	snprintf(byt_rt5651_components, sizeof(byt_rt5651_components),
 		 "cfg-spk:%s cfg-mic:%s%s",
 		 (byt_rt5651_quirk & BYT_RT5651_MONO_SPEAKER) ? "1" : "2",
-		 mic_name[BYT_RT5651_MAP(byt_rt5651_quirk)],
+		 mic_name(BYT_RT5651_MAP(byt_rt5651_quirk)),
 		 (byt_rt5651_quirk & BYT_RT5651_HP_LR_SWAPPED) ?
 			" cfg-hp:lrswap" : "");
 	byt_rt5651_card.components = byt_rt5651_components;
@@ -1080,7 +1085,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 		 "bytcr-rt5651-%s-spk-%s-mic%s",
 		 (byt_rt5651_quirk & BYT_RT5651_MONO_SPEAKER) ?
 			"mono" : "stereo",
-		 mic_name[BYT_RT5651_MAP(byt_rt5651_quirk)],
+		 mic_name(BYT_RT5651_MAP(byt_rt5651_quirk)),
 		 (byt_rt5651_quirk & BYT_RT5651_HP_LR_SWAPPED) ?
 			"-hp-swapped" : "");
 	byt_rt5651_card.long_name = byt_rt5651_long_name;
-- 
2.49.0

[PATCH 4/4] ASoC: Intel: bytcr_wm5102: Avoid OOB array read from the map name

[Takashi Iwai]

Since the quirk bits can be passed arbitrarily via a module option, it
may lead to the OOB access over the map name string array.
Fix it by conditionally reading the array, instead.

Fixes: c556d202bef1 ("ASoC: Intel: bytcr_wm5102: Add BYT_WM5102_OUT_MAP quirk")
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/intel/boards/bytcr_wm5102.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/sound/soc/intel/boards/bytcr_wm5102.c b/sound/soc/intel/boards/bytcr_wm5102.c
index a6dfbcfdf74e..15a7302a1753 100644
--- a/sound/soc/intel/boards/bytcr_wm5102.c
+++ b/sound/soc/intel/boards/bytcr_wm5102.c
@@ -514,11 +514,26 @@ static struct snd_soc_card byt_wm5102_card = {
 
 static char byt_wm5102_components[64]; /* = "cfg-spk:* cfg-int-mic:* cfg-hs-mic:* ..." */
 
+static const char *out_map_name(int n)
+{
+	static const char * const map_names[] = { "spk", "hpout2" };
+	return n < ARRAY_SIZE(map_names) ? map_names[n] : "none";
+}
+
+static const char *intmic_map_name(int n)
+{
+	static const char * const map_names[] = { "in3l", "in1l" };
+	return n < ARRAY_SIZE(map_names) ? map_names[n] : "none";
+}
+
+static const char *hsmic_map_name(int n)
+{
+	static const char * const map_names[] = { "in1l", "in2l" };
+	return n < ARRAY_SIZE(map_names) ? map_names[n] : "none";
+}
+
 static int snd_byt_wm5102_mc_probe(struct platform_device *pdev)
 {
-	static const char * const out_map_name[] = { "spk", "hpout2" };
-	static const char * const intmic_map_name[] = { "in3l", "in1l" };
-	static const char * const hsmic_map_name[] = { "in1l", "in2l" };
 	char codec_name[SND_ACPI_I2C_ID_LEN];
 	struct device *dev = &pdev->dev;
 	struct byt_wm5102_private *priv;
@@ -594,9 +609,9 @@ static int snd_byt_wm5102_mc_probe(struct platform_device *pdev)
 
 	snprintf(byt_wm5102_components, sizeof(byt_wm5102_components),
 		 "cfg-spk:%s cfg-intmic:%s cfg-hsmic:%s",
-		 out_map_name[FIELD_GET(BYT_WM5102_OUT_MAP, quirk)],
-		 intmic_map_name[FIELD_GET(BYT_WM5102_IN_MAP, quirk)],
-		 hsmic_map_name[FIELD_GET(BYT_WM5102_IN_MAP, quirk)]);
+		 out_map_name(FIELD_GET(BYT_WM5102_OUT_MAP, quirk)),
+		 intmic_map_name(FIELD_GET(BYT_WM5102_IN_MAP, quirk)),
+		 hsmic_map_name(FIELD_GET(BYT_WM5102_IN_MAP, quirk)));
 	byt_wm5102_card.components = byt_wm5102_components;
 
 	/* find index of codec dai */
-- 
2.49.0

Re: [PATCH 0/4] ASoC: Intel: byt*: Avoid OOB array read from the map name

[Hans de Goede]

Hi Takashi,

On 15-Apr-25 10:31 AM, Takashi Iwai wrote:
> While reading bug reports, I casually stubmled on a UBSAN warning
> about the array OOB access, and this looks like a real bug in ASoC
> Intel driver code.  So here is a series of quick fixes for them.

Thank you for your work on this.

If we are going to do this I think we should also try to make
the handling of invalid map values set as quirk consistent
between the drivers. ATM we have:

bytcht_es8316: invalid map does not log anything, behaves as "INTMIC_IN1_MAP"
bytcr_rt5640:  invalid map gets logged as an error, but not fixed, behaves as "none"
bytcr_rt5651:  invalid map does not log anything, behaves as "DMIC_MAP"
bytcr_wm5102:  invalid maps get logged as warn_once, overriden by a default map

Note the "behaves as" leaves out the problematic OOB array access,
this is for the rest of the code.

The above means that your fixes for the bytcht_es8316 and bytcr_rt5651
are not entirely correct since you use a map name of "none" for invalid
values which does not match the behavior.

And for the bytcr_wm5102 code your fixes are not necessary because
it does:

static void log_quirks(struct device *dev)
{
        switch (quirk & BYT_WM5102_IN_MAP) {
        case BYT_WM5102_INTMIC_IN3L_HSMIC_IN1L:
                dev_info_once(dev, "quirk INTMIC_IN3L_HSMIC_IN1L enabled\n");
                break;
        case BYT_WM5102_INTMIC_IN1L_HSMIC_IN2L:
                dev_info_once(dev, "quirk INTMIC_IN1L_HSMIC_IN2L enabled\n");
                break;
        default:
                dev_warn_once(dev, "quirk sets invalid input map: 0x%lx, defaulting to INTMIC_
                              quirk & BYT_WM5102_IN_MAP);
                quirk &= ~BYT_WM5102_IN_MAP;
                quirk |= BYT_WM5102_INTMIC_IN3L_HSMIC_IN1L;
                break;
        }
        switch (quirk & BYT_WM5102_OUT_MAP) {
        case BYT_WM5102_SPK_SPK_MAP:
                dev_info_once(dev, "quirk SPK_SPK_MAP enabled\n");
                break;
        case BYT_WM5102_SPK_HPOUT2_MAP:
                dev_info_once(dev, "quirk SPK_HPOUT2_MAP enabled\n");
                break;
        default:
                dev_warn_once(dev, "quirk sets invalid output map: 0x%lx, defaulting to SPK_SP
                              quirk & BYT_WM5102_OUT_MAP);
                quirk &= ~BYT_WM5102_OUT_MAP;
                quirk |= BYT_WM5102_SPK_SPK_MAP;
                break;
        }
	...
}

and log_quirks() gets called before using FIELD_GET(BYT_WM5102_OUT_MAP, quirk) /
FIELD_GET(BYT_WM5102_IN_MAP, quirk) as array indexes.

IMHO it would be best to drop patch 4/4 and for the other 3 machine
drivers I would prefer to instead modify their log_quirks() to be like
the bytcr_wm5102 code both for consistency and so that the behavior
of the code is guaranteed to match the map-name from the array.

Regards,

Hans




> ====
> 
> Takashi Iwai (4):
>   ASoC: Intel: bytcht_es8316: Avoid OOB array read from the map name
>   ASoC: Intel: bytcr_rt5640: Avoid OOB array read from the map name
>   ASoC: Intel: bytcr_rt5651: Avoid OOB array read from the map name
>   ASoC: Intel: bytcr_wm5102: Avoid OOB array read from the map name
> 
>  sound/soc/intel/boards/bytcht_es8316.c | 11 ++++++++---
>  sound/soc/intel/boards/bytcr_rt5640.c  | 11 ++++++++---
>  sound/soc/intel/boards/bytcr_rt5651.c  | 11 ++++++++---
>  sound/soc/intel/boards/bytcr_wm5102.c  | 27 ++++++++++++++++++++------
>  4 files changed, 45 insertions(+), 15 deletions(-)
> 

Re: [PATCH 0/4] ASoC: Intel: byt*: Avoid OOB array read from the map name

[Takashi Iwai]

On Tue, 15 Apr 2025 11:12:56 +0200,
Hans de Goede wrote:
> 
> Hi Takashi,
> 
> On 15-Apr-25 10:31 AM, Takashi Iwai wrote:
> > While reading bug reports, I casually stubmled on a UBSAN warning
> > about the array OOB access, and this looks like a real bug in ASoC
> > Intel driver code.  So here is a series of quick fixes for them.
> 
> Thank you for your work on this.
> 
> If we are going to do this I think we should also try to make
> the handling of invalid map values set as quirk consistent
> between the drivers. ATM we have:
> 
> bytcht_es8316: invalid map does not log anything, behaves as "INTMIC_IN1_MAP"
> bytcr_rt5640:  invalid map gets logged as an error, but not fixed, behaves as "none"
> bytcr_rt5651:  invalid map does not log anything, behaves as "DMIC_MAP"
> bytcr_wm5102:  invalid maps get logged as warn_once, overriden by a default map
> 
> Note the "behaves as" leaves out the problematic OOB array access,
> this is for the rest of the code.
> 
> The above means that your fixes for the bytcht_es8316 and bytcr_rt5651
> are not entirely correct since you use a map name of "none" for invalid
> values which does not match the behavior.
> 
> And for the bytcr_wm5102 code your fixes are not necessary because
> it does:
> 
> static void log_quirks(struct device *dev)
> {
>         switch (quirk & BYT_WM5102_IN_MAP) {
>         case BYT_WM5102_INTMIC_IN3L_HSMIC_IN1L:
>                 dev_info_once(dev, "quirk INTMIC_IN3L_HSMIC_IN1L enabled\n");
>                 break;
>         case BYT_WM5102_INTMIC_IN1L_HSMIC_IN2L:
>                 dev_info_once(dev, "quirk INTMIC_IN1L_HSMIC_IN2L enabled\n");
>                 break;
>         default:
>                 dev_warn_once(dev, "quirk sets invalid input map: 0x%lx, defaulting to INTMIC_
>                               quirk & BYT_WM5102_IN_MAP);
>                 quirk &= ~BYT_WM5102_IN_MAP;
>                 quirk |= BYT_WM5102_INTMIC_IN3L_HSMIC_IN1L;
>                 break;
>         }
>         switch (quirk & BYT_WM5102_OUT_MAP) {
>         case BYT_WM5102_SPK_SPK_MAP:
>                 dev_info_once(dev, "quirk SPK_SPK_MAP enabled\n");
>                 break;
>         case BYT_WM5102_SPK_HPOUT2_MAP:
>                 dev_info_once(dev, "quirk SPK_HPOUT2_MAP enabled\n");
>                 break;
>         default:
>                 dev_warn_once(dev, "quirk sets invalid output map: 0x%lx, defaulting to SPK_SP
>                               quirk & BYT_WM5102_OUT_MAP);
>                 quirk &= ~BYT_WM5102_OUT_MAP;
>                 quirk |= BYT_WM5102_SPK_SPK_MAP;
>                 break;
>         }
> 	...
> }
> 
> and log_quirks() gets called before using FIELD_GET(BYT_WM5102_OUT_MAP, quirk) /
> FIELD_GET(BYT_WM5102_IN_MAP, quirk) as array indexes.
> 
> IMHO it would be best to drop patch 4/4 and for the other 3 machine
> drivers I would prefer to instead modify their log_quirks() to be like
> the bytcr_wm5102 code both for consistency and so that the behavior
> of the code is guaranteed to match the map-name from the array.

I have no preference, and it'd be appreciated if you can just take
over and resubmit as you like ;)


thanks,

Takashi

> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> > ====
> > 
> > Takashi Iwai (4):
> >   ASoC: Intel: bytcht_es8316: Avoid OOB array read from the map name
> >   ASoC: Intel: bytcr_rt5640: Avoid OOB array read from the map name
> >   ASoC: Intel: bytcr_rt5651: Avoid OOB array read from the map name
> >   ASoC: Intel: bytcr_wm5102: Avoid OOB array read from the map name
> > 
> >  sound/soc/intel/boards/bytcht_es8316.c | 11 ++++++++---
> >  sound/soc/intel/boards/bytcr_rt5640.c  | 11 ++++++++---
> >  sound/soc/intel/boards/bytcr_rt5651.c  | 11 ++++++++---
> >  sound/soc/intel/boards/bytcr_wm5102.c  | 27 ++++++++++++++++++++------
> >  4 files changed, 45 insertions(+), 15 deletions(-)
> > 
>