Re: [PATCH] ALSA: usb-audio: Fix out of bounds reads when finding clock sources

[Benoît Sevens <bsevens@xxxxxxxxxx>]

Hi Takashi,

Thank you for the review. Except for one question below, this patch
looks good to me. Would you like me to send your patch as a revised
patch to this mailing list?

On Thu, 21 Nov 2024 at 15:53, Takashi Iwai <tiwai@xxxxxxx> wrote:
>
> On Thu, 21 Nov 2024 15:06:13 +0100,
> Benoît Sevens wrote:
> >
> > A bogus device can provide a clock selector descriptor that contains a
> > bNrInPins that is larger than the actual size of baCSourceID. This can
> > lead to out-of-bound reads in __uac_clock_find_source. These out-of-bound
> > values can be leaked back to the device via the uac_clock_selector_get_val
> > calls.
> >
> > Fixes: 79f920fbff56 ("ALSA: usb-audio: parse clock topology of UAC2 devices")
> > CC: stable@xxxxxxxxxx
> > Signed-off-by: Benoît Sevens <bsevens@xxxxxxxxxx>
> > ---
> >  sound/usb/clock.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/sound/usb/clock.c b/sound/usb/clock.c
> > index 8f85200292f3..94fb628f116e 100644
> > --- a/sound/usb/clock.c
> > +++ b/sound/usb/clock.c
> > @@ -270,7 +270,7 @@ static int __uac_clock_find_source(struct snd_usb_audio *chip,
> >       union uac23_clock_source_desc *source;
> >       union uac23_clock_selector_desc *selector;
> >       union uac23_clock_multiplier_desc *multiplier;
> > -     int ret, i, cur, err, pins, clock_id;
> > +     int ret, i, cur, err, length, pins, clock_id;
> >       const u8 *sources;
> >       int proto = fmt->protocol;
> >       bool readable, writeable;
> > @@ -301,11 +301,19 @@ static int __uac_clock_find_source(struct snd_usb_audio *chip,
> >
> >       selector = snd_usb_find_clock_selector(chip, entity_id, fmt);
> >       if (selector) {
> > +             length = GET_VAL(selector, proto, bLength);
> >               pins = GET_VAL(selector, proto, bNrInPins);
> >               clock_id = GET_VAL(selector, proto, bClockID);
> >               sources = GET_VAL(selector, proto, baCSourceID);
> >               cur = 0;
> >
> > +             if (length < sizeof(selector) + pins) {
>
> This happens to be correct because the size of both
> uac_clock_selector_descriptor and uac3_clock_selector_descriptor are
> same.  Otherwise we'd have to pass sizeof() of the corresponding
> struct depending on the proto value.  So it's worth to comment here.
>
> BTW, now I noticed that we didn't check the size at traversing the
> descriptors.  This means that the driver might read at a wrong
> position even before the point this patch addresses.  For covering it,
> we need additional checks in the validator code.  And this size check
> of clock selectors can be put there, too.
>
> That is, something like below (totally untested).
>
> It'll lead to a different behavior from your patch, though; currently,
> when no suitable clock selector is found, it check the multiplier and
> this might match.  So I'm not entirely sure, but just wanted to tell
> you that it's another option.
>
>
> thanks,
>
> Takashi
>
> --- a/sound/usb/clock.c
> +++ b/sound/usb/clock.c
> @@ -36,6 +36,12 @@ union uac23_clock_multiplier_desc {
>         struct uac_clock_multiplier_descriptor v3;
>  };
>
> +/* check whether the descriptor bLength has the minimal length */
> +#define DESC_LENGTH_CHECK(p, proto) \
> +       ((proto) == UAC_VERSION_3 ? \
> +        ((p)->v3.bLength >= sizeof((p)->v3)) : \
> +        ((p)->v2.bLength >= sizeof((p)->v2)))
> +
>  #define GET_VAL(p, proto, field) \
>         ((proto) == UAC_VERSION_3 ? (p)->v3.field : (p)->v2.field)
>
> @@ -58,6 +64,8 @@ static bool validate_clock_source(void *p, int id, int proto)
>  {
>         union uac23_clock_source_desc *cs = p;
>
> +       if (!DESC_LENGTH_CHECK(cs, proto))
> +               return false;
>         return GET_VAL(cs, proto, bClockID) == id;
>  }
>
> @@ -65,13 +73,23 @@ static bool validate_clock_selector(void *p, int id, int proto)
>  {
>         union uac23_clock_selector_desc *cs = p;
>
> -       return GET_VAL(cs, proto, bClockID) == id;
> +       if (!DESC_LENGTH_CHECK(cs, proto))
> +               return false;
> +       if (GET_VAL(cs, proto, bClockID) != id)
> +               return false;
> +       /* additional length check for baCSourceID, bmControls and iClockSelector */
> +       if (proto == UAC_VERSION_3)
> +               return cs->v3.bLength >= sizeof(cs->v3.bLength) + cs->v3.bNrInPins + 6;

Why can't we just do this here?

 return cs->v3.bLength >= sizeof(cs->v3) + cs->v3.bNrInPins;

> +       else
> +               return cs->v2.bLength >= sizeof(cs->v2.bLength) + cs->v2.bNrInPins + 2;

And same question here, why not:

 return cs->v2.bLength >= sizeof(cs->v2) + cs->v2.bNrInPins;

>  }
>
>  static bool validate_clock_multiplier(void *p, int id, int proto)
>  {
>         union uac23_clock_multiplier_desc *cs = p;
>
> +       if (!DESC_LENGTH_CHECK(cs, proto))
> +               return false;
>         return GET_VAL(cs, proto, bClockID) == id;
>  }
>

Thanks,

Benoit