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

[Takashi Iwai <tiwai@suse.de>]

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(-)
> > 
>