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

[Ruslan Bilovol]

On Fri, Apr 6, 2018 at 2:57 PM, Takashi Iwai <tiwai at suse.de> wrote:
> On Fri, 06 Apr 2018 01:41:51 +0200,
> Ruslan Bilovol wrote:
>>
>> Hi Takashi,
>>
>> On Thu, Apr 5, 2018 at 3:11 PM, 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 | 6 +++---
>> >  1 file changed, 3 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/sound/usb/clock.c b/sound/usb/clock.c
>> > index 27c2275a2505..cbf68ab01836 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;
>> >  }
>> >
>> >  static bool validate_clock_source_v3(void *p, int id)
>> > @@ -65,7 +65,7 @@ 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;
>> > +               cs->bLength == 5 + cs->bNrInPins;
>>
>> This one still has an issue, here we should check it next way:
>>                cs->bLength == 7 + cs->bNrInPins;
>>
>> This is because bLength is 7+bNrInPins as per UAC2 spec, not 5 :P
>
> Doh, I obviously overlooked the hidden members...
> The fixed version is below.
>
>
> Takashi
>
> -- 8< --
> From: Takashi Iwai <tiwai at suse.de>
> Subject: [PATCH v3] ALSA: usb-audio: More strict sanity checks for clock parsers
>
> 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.  The value in clock selector
> validator is corrected from 5 to 7 to count the two unlisted fields
> after baCSourceID[].

Looks good!

Reviewed-by: Ruslan Bilovol <ruslan.bilovol at gmail.com>

I also tested this patch partially (validate_clock_source_v2() func only)
and didn't get any issue in my case.

Thanks,
Ruslan

>
> Suggested-by: Ruslan Bilovol <ruslan.bilovol at gmail.com>
> Signed-off-by: Takashi Iwai <tiwai at suse.de>
> ---
>  sound/usb/clock.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/sound/usb/clock.c b/sound/usb/clock.c
> index 27c2275a2505..30cfd5b1bdfb 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;
>  }
>
>  static bool validate_clock_source_v3(void *p, int id)
> @@ -65,7 +65,7 @@ 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;
> +               cs->bLength == 7 + cs->bNrInPins;
>  }
>
>  static bool validate_clock_selector_v3(void *p, int id)
> @@ -77,7 +77,7 @@ static bool validate_clock_selector_v3(void *p, int id)
>  static bool validate_clock_multiplier_v2(void *p, int id)
>  {
>         struct uac_clock_multiplier_descriptor *cs = p;
> -       return cs->bLength >= sizeof(*cs) && cs->bClockID == id;
> +       return cs->bLength == sizeof(*cs) && cs->bClockID == id;
>  }
>
>  static bool validate_clock_multiplier_v3(void *p, int id)
> --
> 2.16.3
>