Re: CVS commit: src/sys/dev/usb

[Maxime Villard <max%m00nbsd.net@localhost>]

Can you add printfs in these two functions to dump 'bLength'?

I've reverted the change for now. I found these bugs a week ago while
manually crafting incorrect USB packets in Qemu's USB driver. It caused
memory corruptions in the NetBSD guest, which were detected by KASAN.
Fixing the length checks eliminated the corruptions, while still
allowing correctly formed USB devices to attach.


Le 06/07/2019 à 09:54, Thomas Klausner a écrit :
> It could be a coincidence, but with yesterday's kernel my
> non-malicious USB keyboard (Cherry G230) worked and today it doesn't.
>
>
> -uhidev0 at uhub5 port 1 configuration 1 interface 0
> -uhidev0: vendor 046a (0x46a) product 0023 (0x23), rev 2.00/2.20, addr 1, iclass 3/1
> -ukbd0 at uhidev0: 8 Variable keys, 6 Array codes
> -wskbd0 at ukbd0: console keyboard
> -uhidev1 at uhub5 port 1 configuration 1 interface 1
> -uhidev1: vendor 046a (0x46a) product 0023 (0x23), rev 2.00/2.20, addr 1, iclass 3/0
> -uhidev1: 2 report ids
> -uhid0 at uhidev1 reportid 1: input=2, output=0, feature=0
> -uhid1 at uhidev1 reportid 2: input=1, output=0, feature=0
> +uhub5: port 1, set config at addr 1 failed
> +uhub5: autoconfiguration error: device problem, disabling port 1
>
>   Thomas
>
> On Sat, Jul 06, 2019 at 05:05:54AM +0000, Maxime Villard wrote:
> > Module Name:	src
> > Committed By:	maxv
> > Date:		Sat Jul  6 05:05:53 UTC 2019
> >
> > Modified Files:
> > 	src/sys/dev/usb: usb_subr.c
> >
> > Log Message:
> > Fix two length checks, otherwise a malicious USB key plugged in the
> > system could trigger overflows, seen with KASAN.
> >
> >
> > To generate a diff of this commit:
> > cvs rdiff -u -r1.230 -r1.231 src/sys/dev/usb/usb_subr.c
> >
> > Please note that diffs are not public domain; they are subject to the
> > copyright notices on the relevant files.
>
> > Modified files:
> >
> > Index: src/sys/dev/usb/usb_subr.c
> > diff -u src/sys/dev/usb/usb_subr.c:1.230 src/sys/dev/usb/usb_subr.c:1.231
> > --- src/sys/dev/usb/usb_subr.c:1.230	Tue Feb 12 14:17:44 2019
> > +++ src/sys/dev/usb/usb_subr.c	Sat Jul  6 05:05:53 2019
> > @@ -1,4 +1,4 @@
> > -/*	$NetBSD: usb_subr.c,v 1.230 2019/02/12 14:17:44 rin Exp $	*/
> > +/*	$NetBSD: usb_subr.c,v 1.231 2019/07/06 05:05:53 maxv Exp $	*/
> >   /*	$FreeBSD: src/sys/dev/usb/usb_subr.c,v 1.18 1999/11/17 22:33:47 n_hibma Exp $	*/
> >   
> >   /*
> > @@ -32,7 +32,7 @@
> >    */
> >   
> >   #include <sys/cdefs.h>
> > -__KERNEL_RCSID(0, "$NetBSD: usb_subr.c,v 1.230 2019/02/12 14:17:44 rin Exp $");
> > +__KERNEL_RCSID(0, "$NetBSD: usb_subr.c,v 1.231 2019/07/06 05:05:53 maxv Exp $");
> >   
> >   #ifdef _KERNEL_OPT
> >   #include "opt_compat_netbsd.h"
> > @@ -366,8 +366,8 @@ usbd_find_idesc(usb_config_descriptor_t
> >   		    altidx, curaidx);
> >   		DPRINTFN(4, "len=%jd type=%jd", d->bLength, d->bDescriptorType,
> >   		    0, 0);
> > -		if (d->bLength == 0) /* bad descriptor */
> > -			break;
> > +		if (d->bLength < USB_INTERFACE_DESCRIPTOR_SIZE)
> > +			break; /* bad descriptor */
> >   		p += d->bLength;
> >   		if (p <= end && d->bDescriptorType == UDESC_INTERFACE) {
> >   			if (d->bInterfaceNumber != lastidx) {
> > @@ -402,8 +402,8 @@ usbd_find_edesc(usb_config_descriptor_t
> >   	curidx = -1;
> >   	for (p = (char *)d + d->bLength; p < end; ) {
> >   		e = (usb_endpoint_descriptor_t *)p;
> > -		if (e->bLength == 0) /* bad descriptor */
> > -			break;
> > +		if (e->bLength < USB_ENDPOINT_DESCRIPTOR_SIZE)
> > +			break; /* bad descriptor */
> >   		p += e->bLength;
> >   		if (p <= end && e->bDescriptorType == UDESC_INTERFACE)
> >   			return NULL;