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

[Benoît Sevens <bsevens@xxxxxxxxxx>]

On Fri, 22 Nov 2024 at 13:09, Takashi Iwai <tiwai@xxxxxxx> wrote:
>
> On Fri, 22 Nov 2024 11:06:57 +0100,
> Benoît Sevens wrote:
> >
> > 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?
>
> I'm going to submit the proper patch if the fix works for you.
>
> > On Thu, 21 Nov 2024 at 15:53, Takashi Iwai <tiwai@xxxxxxx> wrote:
> > > @@ -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;
>
> Argh, it was just a wrong check.
>
> But the clock selector descriptor must have two more fields in
> addition to baCSourceID array.  Those two are 6 bytes (= 4 bytes for
> bmControls + 2 bytes for wCSelectorDescrStr) for UAC3, while...
>
> > > +       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;
>
> ... for UAC2, they are 2 bytes (= 1 byte for bmControls + 1 byte for
> iClockSelector).  So those numbers appeared in the patch.
>
> Below is the revised patch.  Let me know if this works for you.
>

Looks perfect to me. Thank you for the patch and explanations!

Can we cc stable to get it backported?

>
> thanks,
>
> Takashi
>
> -- 8< --
> From: Takashi Iwai <tiwai@xxxxxxx>
> Subject: [PATCH] ALSA: usb-audio: Fix out of bounds reads when finding clock sources
>
> The current USB-audio driver code doesn't check bLength of each
> descriptor at traversing for clock descriptors.  That is, when a
> device provides a bogus descriptor with a shorter bLength, the driver
> might hit out-of-bounds reads.
>
> For addressing it, this patch adds sanity checks to the validator
> functions for the clock descriptor traversal.  When the descriptor
> length is shorter than expected, it's skipped in the loop.
>
> For the clock source and clock multiplier descriptors, we can just
> check bLength against the sizeof() of each descriptor type.
> OTOH, the clock selector descriptor of UAC2 and UAC3 has an array
> of bNrInPins elements and two more fields at its tail, hence those
> have to be checked in addition to the sizeof() check.
>
> Reported-by: Benoît Sevens <bsevens@xxxxxxxxxx>
> Link: https://lore.kernel.org/20241121140613.3651-1-bsevens@xxxxxxxxxx
> Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
> ---
>  sound/usb/clock.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/sound/usb/clock.c b/sound/usb/clock.c
> index 8f85200292f3..842ba5b801ea 100644
> --- 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,27 @@ 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 array (in bNrInPins size)
> +        * and two more fields (which sizes depend on the protocol)
> +        */
> +       if (proto == UAC_VERSION_3)
> +               return cs->v3.bLength >= sizeof(cs->v3) + cs->v3.bNrInPins +
> +                       4 /* bmControls */ + 2 /* wCSelectorDescrStr */;
> +       else
> +               return cs->v2.bLength >= sizeof(cs->v2) + cs->v2.bNrInPins +
> +                       1 /* bmControls */ + 1 /* iClockSelector */;
>  }
>
>  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;
>  }
>
> --
> 2.43.0
>