[alsa-devel] [PATCH v2 0/3] ALSA: usb-audio: Minor refactoring and UAC3 fix
Hi,
this is a v2 patchset (now with a shiny cover letter!) for refactoring and hardening UAC2 and UAC3 clock parsers.
BTW, it'll be great if anyone can double-check that this rewrite doesn't break things, especially with a UAC3 device.
thanks,
Takashi
===
v1->v2: Rename the helper function to be more specific Make validators to check sizes more strictly
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(-)
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;
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 | 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; }
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; }
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)
Hi Takashi,
On Wed, Apr 4, 2018 at 8:36 AM, 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 | 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;
}
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;
This one looks good too
Thanks, Ruslan
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@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 | 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
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 5e533edfb092..177c6017e22c 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 == 5 + 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) \
Hi Takashi,
On Wed, Apr 4, 2018 at 8:36 AM, 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.
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 5e533edfb092..177c6017e22c 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 == 5 + cs->bNrInPins;
Same comments as for UAC2 patch, but in this case bLength should be "11 + bNrInPins", so finally it should looks like:
return cs->bLength >= sizeof(*cs) && cs->bClockID == id && cs->bLength == 11 + cs->bNrInPins;
Thanks, Ruslan
}
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
participants (2)
-
Ruslan Bilovol -
Takashi Iwai