| Message ID | 20180404053621.32539-3-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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)
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(-)