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

[Hans de Goede <hdegoede@redhat.com>]

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