re: kern/55919: Creative HP-BTW3 (BT-W3) USB audio device has another descriptor layout

[Ryo ONODERA <ryo%tetera.org@localhost>]

Hi,

matthew green <mrg%eterna.com.au@localhost> writes:

> +       while (offs < size) {
> +               desc = (const void *)(tbuf + offs);
>
> should this be "offs <= size - sizeof(*desc)"?  so that we
> don't attempt to read beyond the buffer for this part, and
> these should probably check the larger structure size before
> deciding it's valid:
>
> +                       asid = (const void *)desc;
> +                       offs += asid->bLength;
>
> +                       asf1d = (const void *)desc;
> +                       offs += asf1d->bLength;
>
> etc..
>
> rest LGTM.  would be nice to have tested on many usb audio
> devices :)

Thanks for your comment.
I have tried to fixed two issues.

How about this?

http://www.ryoon.net/~ryoon/sys-dev-usb-uaudio-3.diff

Index: sys/dev/usb/uaudio.c
===================================================================
RCS file: /cvsroot/src/sys/dev/usb/uaudio.c,v
retrieving revision 1.168
diff -u -r1.168 uaudio.c
--- sys/dev/usb/uaudio.c	10 Jan 2021 15:50:16 -0000	1.168
+++ sys/dev/usb/uaudio.c	15 Feb 2021 13:28:02 -0000
@@ -1532,52 +1532,73 @@
 		  int size, const usb_interface_descriptor_t *id)
 #define offs (*offsp)
 {
-	const struct usb_audio_streaming_interface_descriptor *asid;
-	const struct usb_audio_streaming_type1_descriptor *asf1d;
-	const usb_endpoint_descriptor_audio_t *ed;
-	const usb_endpoint_descriptor_audio_t *epdesc1;
-	const struct usb_audio_streaming_endpoint_descriptor *sed;
+	const struct usb_audio_streaming_interface_descriptor *asid = NULL;
+	const struct usb_audio_streaming_type1_descriptor *asf1d = NULL;
+	const usb_endpoint_descriptor_audio_t *ed = NULL;
+	const usb_endpoint_descriptor_audio_t *epdesc1 = NULL;
+	const struct usb_audio_streaming_endpoint_descriptor *sed = NULL;
+	const usb_descriptor_t *desc = NULL;
 	int format, chan __unused, prec, enc;
 	int dir, type, sync;
 	struct as_info ai;
 	const char *format_str __unused;
 
-	asid = (const void *)(tbuf + offs);
-	if (asid->bDescriptorType != UDESC_CS_INTERFACE ||
-	    asid->bDescriptorSubtype != AS_GENERAL)
-		return USBD_INVAL;
-	DPRINTF("asid: bTerminalLink=%d wFormatTag=%d\n",
-		 asid->bTerminalLink, UGETW(asid->wFormatTag));
-	offs += asid->bLength;
-	if (offs > size)
-		return USBD_INVAL;
+	while (offs <= size - sizeof(*desc)) {
+		desc = (const void *)(tbuf + offs);
 
-	asf1d = (const void *)(tbuf + offs);
-	if (asf1d->bDescriptorType != UDESC_CS_INTERFACE ||
-	    asf1d->bDescriptorSubtype != FORMAT_TYPE)
-		return USBD_INVAL;
-	offs += asf1d->bLength;
-	if (offs > size)
+		if (desc->bDescriptorType == UDESC_CS_INTERFACE &&
+		    desc->bDescriptorSubtype == AS_GENERAL) {
+			offs += desc->bLength;
+			if (offs > size)
+				return USBD_INVAL;
+			asid = (const void *)desc;
+		} else if (desc->bDescriptorType == UDESC_CS_INTERFACE &&
+			   desc->bDescriptorSubtype == FORMAT_TYPE) {
+			offs += desc->bLength;
+			if (offs > size)
+				return USBD_INVAL;
+			asf1d = (const void *)desc;
+		} else if (desc->bDescriptorType == UDESC_ENDPOINT) {
+			offs += desc->bLength;
+			if (offs > size)
+				return USBD_INVAL;
+			ed = (const void *)desc;
+		} else if (desc->bDescriptorType == UDESC_CS_ENDPOINT &&
+	    		   desc->bDescriptorSubtype == AS_GENERAL) {
+			offs += desc->bLength;
+			if (offs > size)
+				return USBD_INVAL;
+			sed = (const void *)desc;
+		} else if (ed != NULL && id->bNumEndpoints > 1 &&
+			   desc->bDescriptorType == UDESC_ENDPOINT) {
+			offs += desc->bLength;
+			if (offs > size)
+				return USBD_INVAL;
+			epdesc1 = (const void*)desc;
+		} else
+			break;
+	}
+
+	if (asid == NULL || asf1d == NULL || ed == NULL || sed == NULL ||
+	    (id->bNumEndpoints > 1 && epdesc1 == NULL))
 		return USBD_INVAL;
 
+	format = UGETW(asid->wFormatTag);
+	DPRINTF("asid: bTerminalLink=%d wFormatTag=%d\n",
+		 asid->bTerminalLink, format);
+
 	if (asf1d->bFormatType != FORMAT_TYPE_I) {
 		aprint_normal_dev(sc->sc_dev,
-		    "ignored setting with type %d format\n", UGETW(asid->wFormatTag));
+		    "ignored setting with type %d format\n", format);
 		return USBD_NORMAL_COMPLETION;
 	}
 
-	ed = (const void *)(tbuf + offs);
-	if (ed->bDescriptorType != UDESC_ENDPOINT)
-		return USBD_INVAL;
 	DPRINTF("endpoint[0] bLength=%d bDescriptorType=%d "
 		 "bEndpointAddress=%d bmAttributes=%#x wMaxPacketSize=%d "
 		 "bInterval=%d bRefresh=%d bSynchAddress=%d\n",
 		 ed->bLength, ed->bDescriptorType, ed->bEndpointAddress,
 		 ed->bmAttributes, UGETW(ed->wMaxPacketSize),
 		 ed->bInterval, ed->bRefresh, ed->bSynchAddress);
-	offs += ed->bLength;
-	if (offs > size)
-		return USBD_INVAL;
 	if (UE_GET_XFERTYPE(ed->bmAttributes) != UE_ISOCHRONOUS)
 		return USBD_INVAL;
 
@@ -1606,14 +1627,7 @@
 #endif
 	}
 
-	sed = (const void *)(tbuf + offs);
-	if (sed->bDescriptorType != UDESC_CS_ENDPOINT ||
-	    sed->bDescriptorSubtype != AS_GENERAL)
-		return USBD_INVAL;
 	DPRINTF(" streadming_endpoint: offset=%d bLength=%d\n", offs, sed->bLength);
-	offs += sed->bLength;
-	if (offs > size)
-		return USBD_INVAL;
 
 #ifdef UAUDIO_MULTIPLE_ENDPOINTS
 	if (sync && id->bNumEndpoints <= 1) {
@@ -1627,11 +1641,7 @@
 		    "non sync-pipe endpoint but multiple endpoints\n");
 		return USBD_INVAL;
 	}
-	epdesc1 = NULL;
 	if (id->bNumEndpoints > 1) {
-		epdesc1 = (const void*)(tbuf + offs);
-		if (epdesc1->bDescriptorType != UDESC_ENDPOINT)
-			return USBD_INVAL;
 		DPRINTF("endpoint[1] bLength=%d "
 			 "bDescriptorType=%d bEndpointAddress=%d "
 			 "bmAttributes=%#x wMaxPacketSize=%d bInterval=%d "
@@ -1640,9 +1650,6 @@
 			 epdesc1->bEndpointAddress, epdesc1->bmAttributes,
 			 UGETW(epdesc1->wMaxPacketSize), epdesc1->bInterval,
 			 epdesc1->bRefresh, epdesc1->bSynchAddress);
-		offs += epdesc1->bLength;
-		if (offs > size)
-			return USBD_INVAL;
 		if (epdesc1->bSynchAddress != 0) {
 			aprint_error_dev(sc->sc_dev,
 			    "invalid endpoint: bSynchAddress=0\n");
@@ -1665,7 +1672,6 @@
 		/* UE_GET_ADDR(epdesc1->bEndpointAddress), and epdesc1->bRefresh */
 	}
 
-	format = UGETW(asid->wFormatTag);
 	chan = asf1d->bNrChannels;
 	prec = asf1d->bBitResolution;
 	if (prec != 8 && prec != 16 && prec != 24) {


> .mrg.

-- 
Ryo ONODERA // ryo%tetera.org@localhost
PGP fingerprint = 82A2 DC91 76E0 A10A 8ABB  FD1B F404 27FA C7D1 15F3