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

[Takashi Iwai <tiwai@suse.de>]

On Fri, 22 Nov 2024 14:01:53 +0100,
Benoît Sevens wrote:
> 
> On Fri, 22 Nov 2024 at 13:09, Takashi Iwai <tiwai@suse.de> 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@suse.de> 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?

Sure, I'll add it when submitting properly.


thanks,

Takashi