[PATCH v3 0/3] ALSA: usb-audio: Minor refactoring and UAC3 fix

[PATCH v3 0/3] ALSA: usb-audio: Minor refactoring and UAC3 fix

[Takashi Iwai]

Hi,

this is a v3 patchset, addressing the silly mistakes in validator
functions in the previous patchset.


Takashi

===

v1->v2:	Rename the helper function to be more specific
	Make validators to check sizes more strictly
v2->v3: Fix clock selector validator v2 and v3 as suggested by Ruslan

Takashi Iwai (3):
  ALSA: usb-audio: Refactor clock finder helpers
  ALSA: usb-audio: More strict sanity checks for clock parsers
  ALSA: usb-audio: Add sanity checks in UAC3 clock parsers

 sound/usb/clock.c | 128 +++++++++++++++++++++++-------------------------------
 1 file changed, 54 insertions(+), 74 deletions(-)

-- 
2.16.2

[PATCH v3 1/3] ALSA: usb-audio: Refactor clock finder helpers

[Takashi Iwai]

There are lots of open-coded functions to find a clock source,
selector and multiplier.  Now there are both v2 and v3, so six
variants.

This patch refactors the code to use a common helper for the main
loop, and define each validator function for each target.
There is no functional change.

Fixes: 9a2fe9b801f5 ("ALSA: usb: initial USB Audio Device Class 3.0 support")
Reviewed-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/usb/clock.c | 127 +++++++++++++++++++++++-------------------------------
 1 file changed, 53 insertions(+), 74 deletions(-)

diff --git a/sound/usb/clock.c b/sound/usb/clock.c
index ab39ccb974c6..27c2275a2505 100644
--- a/sound/usb/clock.c
+++ b/sound/usb/clock.c
@@ -35,105 +35,84 @@
 #include "clock.h"
 #include "quirks.h"
 
-static struct uac_clock_source_descriptor *
-	snd_usb_find_clock_source(struct usb_host_interface *ctrl_iface,
-				  int clock_id)
+static void *find_uac_clock_desc(struct usb_host_interface *iface, int id,
+				 bool (*validator)(void *, int), u8 type)
 {
-	struct uac_clock_source_descriptor *cs = NULL;
+	void *cs = NULL;
 
-	while ((cs = snd_usb_find_csint_desc(ctrl_iface->extra,
-					     ctrl_iface->extralen,
-					     cs, UAC2_CLOCK_SOURCE))) {
-		if (cs->bLength >= sizeof(*cs) && cs->bClockID == clock_id)
+	while ((cs = snd_usb_find_csint_desc(iface->extra, iface->extralen,
+					     cs, type))) {
+		if (validator(cs, id))
 			return cs;
 	}
 
 	return NULL;
 }
 
-static struct uac3_clock_source_descriptor *
-	snd_usb_find_clock_source_v3(struct usb_host_interface *ctrl_iface,
-				  int clock_id)
+static bool validate_clock_source_v2(void *p, int id)
 {
-	struct uac3_clock_source_descriptor *cs = NULL;
-
-	while ((cs = snd_usb_find_csint_desc(ctrl_iface->extra,
-					     ctrl_iface->extralen,
-					     cs, UAC3_CLOCK_SOURCE))) {
-		if (cs->bClockID == clock_id)
-			return cs;
-	}
-
-	return NULL;
+	struct uac_clock_source_descriptor *cs = p;
+	return cs->bLength >= sizeof(*cs) && cs->bClockID == id;
 }
 
-static struct uac_clock_selector_descriptor *
-	snd_usb_find_clock_selector(struct usb_host_interface *ctrl_iface,
-				    int clock_id)
+static bool validate_clock_source_v3(void *p, int id)
 {
-	struct uac_clock_selector_descriptor *cs = NULL;
-
-	while ((cs = snd_usb_find_csint_desc(ctrl_iface->extra,
-					     ctrl_iface->extralen,
-					     cs, UAC2_CLOCK_SELECTOR))) {
-		if (cs->bLength >= sizeof(*cs) && cs->bClockID == clock_id) {
-			if (cs->bLength < 5 + cs->bNrInPins)
-				return NULL;
-			return cs;
-		}
-	}
-
-	return NULL;
+	struct uac3_clock_source_descriptor *cs = p;
+	return cs->bClockID == id;
 }
 
-static struct uac3_clock_selector_descriptor *
-	snd_usb_find_clock_selector_v3(struct usb_host_interface *ctrl_iface,
-				    int clock_id)
+static bool validate_clock_selector_v2(void *p, int id)
 {
-	struct uac3_clock_selector_descriptor *cs = NULL;
-
-	while ((cs = snd_usb_find_csint_desc(ctrl_iface->extra,
-					     ctrl_iface->extralen,
-					     cs, UAC3_CLOCK_SELECTOR))) {
-		if (cs->bClockID == clock_id)
-			return cs;
-	}
-
-	return NULL;
+	struct uac_clock_selector_descriptor *cs = p;
+	return cs->bLength >= sizeof(*cs) && cs->bClockID == id &&
+		cs->bLength >= 5 + cs->bNrInPins;
 }
 
-static struct uac_clock_multiplier_descriptor *
-	snd_usb_find_clock_multiplier(struct usb_host_interface *ctrl_iface,
-				      int clock_id)
+static bool validate_clock_selector_v3(void *p, int id)
 {
-	struct uac_clock_multiplier_descriptor *cs = NULL;
-
-	while ((cs = snd_usb_find_csint_desc(ctrl_iface->extra,
-					     ctrl_iface->extralen,
-					     cs, UAC2_CLOCK_MULTIPLIER))) {
-		if (cs->bLength >= sizeof(*cs) && cs->bClockID == clock_id)
-			return cs;
-	}
-
-	return NULL;
+	struct uac3_clock_selector_descriptor *cs = p;
+	return cs->bClockID == id;
 }
 
-static struct uac3_clock_multiplier_descriptor *
-	snd_usb_find_clock_multiplier_v3(struct usb_host_interface *ctrl_iface,
-				      int clock_id)
+static bool validate_clock_multiplier_v2(void *p, int id)
 {
-	struct uac3_clock_multiplier_descriptor *cs = NULL;
+	struct uac_clock_multiplier_descriptor *cs = p;
+	return cs->bLength >= sizeof(*cs) && cs->bClockID == id;
+}
 
-	while ((cs = snd_usb_find_csint_desc(ctrl_iface->extra,
-					     ctrl_iface->extralen,
-					     cs, UAC3_CLOCK_MULTIPLIER))) {
-		if (cs->bClockID == clock_id)
-			return cs;
-	}
+static bool validate_clock_multiplier_v3(void *p, int id)
+{
+	struct uac3_clock_multiplier_descriptor *cs = p;
+	return cs->bClockID == id;
+}
 
-	return NULL;
+#define DEFINE_FIND_HELPER(name, obj, validator, type)		\
+static obj *name(struct usb_host_interface *iface, int id)	\
+{								\
+	return find_uac_clock_desc(iface, id, validator, type);	\
 }
 
+DEFINE_FIND_HELPER(snd_usb_find_clock_source,
+		   struct uac_clock_source_descriptor,
+		   validate_clock_source_v2, UAC2_CLOCK_SOURCE);
+DEFINE_FIND_HELPER(snd_usb_find_clock_source_v3,
+		   struct uac3_clock_source_descriptor,
+		   validate_clock_source_v3, UAC3_CLOCK_SOURCE);
+
+DEFINE_FIND_HELPER(snd_usb_find_clock_selector,
+		   struct uac_clock_selector_descriptor,
+		   validate_clock_selector_v2, UAC2_CLOCK_SELECTOR);
+DEFINE_FIND_HELPER(snd_usb_find_clock_selector_v3,
+		   struct uac3_clock_selector_descriptor,
+		   validate_clock_selector_v3, UAC3_CLOCK_SELECTOR);
+
+DEFINE_FIND_HELPER(snd_usb_find_clock_multiplier,
+		   struct uac_clock_multiplier_descriptor,
+		   validate_clock_multiplier_v2, UAC2_CLOCK_MULTIPLIER);
+DEFINE_FIND_HELPER(snd_usb_find_clock_multiplier_v3,
+		   struct uac3_clock_multiplier_descriptor,
+		   validate_clock_multiplier_v3, UAC3_CLOCK_MULTIPLIER);
+
 static int uac_clock_selector_get_val(struct snd_usb_audio *chip, int selector_id)
 {
 	unsigned char buf;
-- 
2.16.2

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

[Takashi Iwai]

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@gmail.com>
Signed-off-by: Takashi Iwai <tiwai@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;
 }
 
 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.2

[PATCH v3 3/3] ALSA: usb-audio: Add sanity checks in UAC3 clock parsers

[Takashi Iwai]

The UAC3 clock parser codes lack of the sanity checks for malformed
descriptors like UAC2 parser does.  Without it, the driver may lead to
a potential crash.

Fixes: 9a2fe9b801f5 ("ALSA: usb: initial USB Audio Device Class 3.0 support")
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/usb/clock.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/sound/usb/clock.c b/sound/usb/clock.c
index cbf68ab01836..29b7c85635e3 100644
--- a/sound/usb/clock.c
+++ b/sound/usb/clock.c
@@ -58,7 +58,7 @@ static bool validate_clock_source_v2(void *p, int id)
 static bool validate_clock_source_v3(void *p, int id)
 {
 	struct uac3_clock_source_descriptor *cs = p;
-	return cs->bClockID == id;
+	return cs->bLength == sizeof(*cs) && cs->bClockID == id;
 }
 
 static bool validate_clock_selector_v2(void *p, int id)
@@ -71,7 +71,8 @@ static bool validate_clock_selector_v2(void *p, int id)
 static bool validate_clock_selector_v3(void *p, int id)
 {
 	struct uac3_clock_selector_descriptor *cs = p;
-	return cs->bClockID == id;
+	return cs->bLength >= sizeof(*cs) && cs->bClockID == id &&
+		cs->bLength == 11 + cs->bNrInPins;
 }
 
 static bool validate_clock_multiplier_v2(void *p, int id)
@@ -83,7 +84,7 @@ static bool validate_clock_multiplier_v2(void *p, int id)
 static bool validate_clock_multiplier_v3(void *p, int id)
 {
 	struct uac3_clock_multiplier_descriptor *cs = p;
-	return cs->bClockID == id;
+	return cs->bLength == sizeof(*cs) && cs->bClockID == id;
 }
 
 #define DEFINE_FIND_HELPER(name, obj, validator, type)		\
-- 
2.16.2

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

[Ruslan Bilovol]

Hi Takashi,

On Thu, Apr 5, 2018 at 3:11 PM, Takashi Iwai <tiwai@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@gmail.com>
> Signed-off-by: Takashi Iwai <tiwai@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

Thanks,
Ruslan

>  }
>
>  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.2
>

Re: [PATCH v3 3/3] ALSA: usb-audio: Add sanity checks in UAC3 clock parsers

[Ruslan Bilovol]

On Thu, Apr 5, 2018 at 3:11 PM, Takashi Iwai <tiwai@suse.de> wrote:
> The UAC3 clock parser codes lack of the sanity checks for malformed
> descriptors like UAC2 parser does.  Without it, the driver may lead to
> a potential crash.

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

And also I tested this patch along with patch #1 and don't see any issue,
so feel free to add to both:

Tested-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>

Thanks,
Ruslan

>
> Fixes: 9a2fe9b801f5 ("ALSA: usb: initial USB Audio Device Class 3.0 support")
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  sound/usb/clock.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/sound/usb/clock.c b/sound/usb/clock.c
> index cbf68ab01836..29b7c85635e3 100644
> --- a/sound/usb/clock.c
> +++ b/sound/usb/clock.c
> @@ -58,7 +58,7 @@ static bool validate_clock_source_v2(void *p, int id)
>  static bool validate_clock_source_v3(void *p, int id)
>  {
>         struct uac3_clock_source_descriptor *cs = p;
> -       return cs->bClockID == id;
> +       return cs->bLength == sizeof(*cs) && cs->bClockID == id;
>  }
>
>  static bool validate_clock_selector_v2(void *p, int id)
> @@ -71,7 +71,8 @@ static bool validate_clock_selector_v2(void *p, int id)
>  static bool validate_clock_selector_v3(void *p, int id)
>  {
>         struct uac3_clock_selector_descriptor *cs = p;
> -       return cs->bClockID == id;
> +       return cs->bLength >= sizeof(*cs) && cs->bClockID == id &&
> +               cs->bLength == 11 + cs->bNrInPins;
>  }
>
>  static bool validate_clock_multiplier_v2(void *p, int id)
> @@ -83,7 +84,7 @@ static bool validate_clock_multiplier_v2(void *p, int id)
>  static bool validate_clock_multiplier_v3(void *p, int id)
>  {
>         struct uac3_clock_multiplier_descriptor *cs = p;
> -       return cs->bClockID == id;
> +       return cs->bLength == sizeof(*cs) && cs->bClockID == id;
>  }
>
>  #define DEFINE_FIND_HELPER(name, obj, validator, type)         \
> --
> 2.16.2
>

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

[Ruslan Bilovol]

On Fri, Apr 6, 2018 at 2:41 AM, Ruslan Bilovol <ruslan.bilovol@gmail.com> wrote:
> Hi Takashi,
>
> On Thu, Apr 5, 2018 at 3:11 PM, Takashi Iwai <tiwai@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@gmail.com>
>> Signed-off-by: Takashi Iwai <tiwai@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;

Also I tested scenario which uses only this function (validate_clock_source_v2)
and it works fine to me.

>>  }
>>
>>  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
>
> Thanks,
> Ruslan
>
>>  }
>>
>>  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.2
>>



-- 
Best regards,
Ruslan Bilovol

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

[Takashi Iwai]

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@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@gmail.com>
> > Signed-off-by: Takashi Iwai <tiwai@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@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[].

Suggested-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>
Signed-off-by: Takashi Iwai <tiwai@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

Re: [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@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@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@gmail.com>
>> > Signed-off-by: Takashi Iwai <tiwai@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@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@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@gmail.com>
> Signed-off-by: Takashi Iwai <tiwai@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
>

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

[Takashi Iwai]

On Sat, 07 Apr 2018 02:55:23 +0200,
Ruslan Bilovol wrote:
> 
> On Fri, Apr 6, 2018 at 2:57 PM, Takashi Iwai <tiwai@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@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@gmail.com>
> >> > Signed-off-by: Takashi Iwai <tiwai@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@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@gmail.com>
> 
> I also tested this patch partially (validate_clock_source_v2() func only)
> and didn't get any issue in my case.

Great, I merged these three patches now .


thanks,

Takashi