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

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

The following reply was made to PR kern/55919; it has been noted by GNATS.

From: Ryo ONODERA <ryo%tetera.org@localhost>
To: matthew green <mrg%eterna.com.au@localhost>, gnats-bugs%netbsd.org@localhost
Cc: kern-bug-people%netbsd.org@localhost, gnats-admin%netbsd.org@localhost, netbsd-bugs%netbsd.org@localhost
Subject: re: kern/55919: Creative HP-BTW3 (BT-W3) USB audio device has
 another descriptor layout
Date: Mon, 15 Feb 2021 22:30:46 +0900

 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