[alsa-devel] [PATCH v2 2/3] ALSA: usb-audio: More strict sanity checks for clock parsers

[Takashi Iwai]

On Thu, 05 Apr 2018 13:16:09 +0200,
Ruslan Bilovol wrote:
> 
> Hi Takashi,
> 
> On Wed, Apr 4, 2018 at 8:36 AM, Takashi Iwai <tiwai at suse.de> wrote:
> > The sanity checks introduced for malformed descriptors loosely check
> > the given descriptor size, although the size greater than the defined
> > description is invalid.  It was due to a concern of any funky firmware
> > in the actual products.  But this doesn't look hitting, and any sane
> > products must have the defined descriptors.
> >
> > So in this patch, we make the validators more strict, allowing only
> > with the defined descriptor sizes.
> >
> > Suggested-by: Ruslan Bilovol <ruslan.bilovol at gmail.com>
> > Signed-off-by: Takashi Iwai <tiwai at suse.de>
> > ---
> >  sound/usb/clock.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/sound/usb/clock.c b/sound/usb/clock.c
> > index 27c2275a2505..5e533edfb092 100644
> > --- a/sound/usb/clock.c
> > +++ b/sound/usb/clock.c
> > @@ -52,7 +52,7 @@ static void *find_uac_clock_desc(struct usb_host_interface *iface, int id,
> >  static bool validate_clock_source_v2(void *p, int id)
> >  {
> >         struct uac_clock_source_descriptor *cs = p;
> > -       return cs->bLength >= sizeof(*cs) && cs->bClockID == id;
> > +       return cs->bLength == sizeof(*cs) && cs->bClockID == id;
> 
> This one looks good
> 
> >  }
> >
> >  static bool validate_clock_source_v3(void *p, int id)
> > @@ -64,8 +64,8 @@ static bool validate_clock_source_v3(void *p, int id)
> >  static bool validate_clock_selector_v2(void *p, int id)
> >  {
> >         struct uac_clock_selector_descriptor *cs = p;
> > -       return cs->bLength >= sizeof(*cs) && cs->bClockID == id &&
> > -               cs->bLength >= 5 + cs->bNrInPins;
> > +       return cs->bLength == sizeof(*cs) && cs->bClockID == id &&
> > +               cs->bLength == 5 + cs->bNrInPins;
> 
> This one is wrong. uac_clock_selector_descriptor is of variable size, so
> original first bLength check was correct, but last one can be more strict.
> However, as per UAC2 spec, bLength should be "7 + bNrInPins", so
> finally we shoud have here something like this:
> 
>        return cs->bLength >= sizeof(*cs) && cs->bClockID == id &&
>                cs->bLength == 7 + cs->bNrInPins;

Gah, of course!
Thanks for checking it.

Will send a v3 patchset.


Takashi