Bug 80773

Summary:	"usbd_get_string()" could have taken a length parameter
Product:	Base System	Reporter:	Hans Petter Selasky <hselasky>
Component:	usb	Assignee:	Warner Losh <imp>
Status:	Closed FIXED
Severity:	Affects Only Me
Priority:	Normal
Version:	6.0-CURRENT
Hardware:	Any
OS:	Any

Description Hans Petter Selasky 2005-05-08 14:50:01 UTC


Fix: 

example + extra comments:

/* Use "usbd_get_string_any()" instead of
 * "usbd_get_string_desc()", when the language id is not known. The
 * maximum length of the string, "len", includes the terminating zero.
 * "usbd_get_string_any()" will always write a terminating zero to "buf",
 * also on error.
 */
usbd_status
usbd_get_string(struct usbd_device *udev, int si, char *buf, int len)
{

...

 if(len == 0)
 {
  return (USBD_NORMAL_COMPLETION);
 }

 buf[0] = 0;

 /* subtract the terminating zero */
 len--;

...

 for(i = 0; (i < n) && len; i++, len--)
 {

...
}

Comment 1 iedowse freebsd_committer

2006-02-25 04:10:27 UTC

State Changed
From-To: open->feedback


Can you provide a patch against -CURRENT to address this?

Comment 2 Mark Linimon freebsd_committer

2007-06-30 07:40:37 UTC

State Changed
From-To: feedback->suspended

No response from submitter; it still seems like this problem should be fixed.

Comment 3 M. Warner Losh 2007-06-30 16:08:18 UTC

----Next_Part(Sat_Jun_30_09_08_18_2007_041)--
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit

Please find enclosed a patch for this.  I'm sitting on the fence as to
whether or not to commit it, since it is an api/abi change.

Warner

----Next_Part(Sat_Jun_30_09_08_18_2007_041)--
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline; filename=patch

Index: if_cdce.c
===================================================================
RCS file: /home/ncvs/src/sys/dev/usb/if_cdce.c,v
retrieving revision 1.24
diff -u -r1.24 if_cdce.c
--- if_cdce.c	23 Jun 2007 06:47:43 -0000	1.24
+++ if_cdce.c	30 Jun 2007 14:28:41 -0000
@@ -280,7 +280,8 @@
 
 	ue = (const usb_cdc_ethernet_descriptor_t *)usb_find_desc(dev,
 	    UDESC_INTERFACE, UDESCSUB_CDC_ENF);
-	if (!ue || usbd_get_string(dev, ue->iMacAddress, eaddr_str)) {
+	if (!ue || usbd_get_string(dev, ue->iMacAddress, eaddr_str,
+	    sizeof(eaddr_str))) {
 		/* Fake MAC address */
 		device_printf(sc->cdce_dev, "faking MAC address\n");
 		eaddr[0]= 0x2a;
Index: uhub.c
===================================================================
RCS file: /home/ncvs/src/sys/dev/usb/uhub.c,v
retrieving revision 1.81
diff -u -r1.81 uhub.c
--- uhub.c	29 Jun 2007 20:34:42 -0000	1.81
+++ uhub.c	30 Jun 2007 14:28:41 -0000
@@ -655,7 +655,8 @@
 
 found_dev:
 	/* XXX can sleep */
-	(void)usbd_get_string(dev, dev->ddesc.iSerialNumber, &serial[0]);
+	(void)usbd_get_string(dev, dev->ddesc.iSerialNumber, serial,
+	    sizeof(serial));
 	if (dev->ifacenums == NULL) {
 		snprintf(buf, buflen, "vendor=0x%04x product=0x%04x "
 		    "devclass=0x%02x devsubclass=0x%02x "
Index: usb_subr.c
===================================================================
RCS file: /home/ncvs/src/sys/dev/usb/usb_subr.c,v
retrieving revision 1.94
diff -u -r1.94 usb_subr.c
--- usb_subr.c	20 Jun 2007 05:10:54 -0000	1.94
+++ usb_subr.c	30 Jun 2007 14:28:42 -0000
@@ -213,12 +213,14 @@
 	}
 
 	if (usedev) {
-		if (usbd_get_string(dev, udd->iManufacturer, v))
+		if (usbd_get_string(dev, udd->iManufacturer, v,
+		    USB_MAX_STRING_LEN))
 			vendor = NULL;
 		else
 			vendor = v;
 		usbd_trim_spaces(vendor);
-		if (usbd_get_string(dev, udd->iProduct, p))
+		if (usbd_get_string(dev, udd->iProduct, p,
+		    USB_MAX_STRING_LEN))
 			product = NULL;
 		else
 			product = p;
Index: usbdi.c
===================================================================
RCS file: /home/ncvs/src/sys/dev/usb/usbdi.c,v
retrieving revision 1.102
diff -u -r1.102 usbdi.c
--- usbdi.c	20 Jun 2007 05:10:54 -0000	1.102
+++ usbdi.c	30 Jun 2007 14:28:42 -0000
@@ -1310,7 +1310,7 @@
 }
 
 usbd_status
-usbd_get_string(usbd_device_handle dev, int si, char *buf)
+usbd_get_string(usbd_device_handle dev, int si, char *buf, size_t len)
 {
 	int swap = dev->quirks->uq_flags & UQ_SWAP_UNICODE;
 	usb_string_descriptor_t us;
@@ -1321,6 +1321,8 @@
 	int size;
 
 	buf[0] = '\0';
+	if (len == 0)
+		return (USBD_NORMAL_COMPLETION);
 	if (si == 0)
 		return (USBD_INVAL);
 	if (dev->quirks->uq_flags & UQ_NO_STRINGS)
@@ -1342,7 +1344,7 @@
 		return (err);
 	s = buf;
 	n = size / 2 - 1;
-	for (i = 0; i < n; i++) {
+	for (i = 0; i < n && i < len - 1; i++) {
 		c = UGETW(us.bString[i]);
 		/* Convert from Unicode, handle buggy strings. */
 		if ((c & 0xff00) == 0)
Index: usbdi.h
===================================================================
RCS file: /home/ncvs/src/sys/dev/usb/usbdi.h,v
retrieving revision 1.62
diff -u -r1.62 usbdi.h
--- usbdi.h	12 Jun 2007 19:40:20 -0000	1.62
+++ usbdi.h	30 Jun 2007 14:28:42 -0000
@@ -173,7 +173,8 @@
 
 int usbd_ratecheck(struct timeval *last);
 
-usbd_status usbd_get_string(usbd_device_handle dev, int si, char *buf);
+usbd_status usbd_get_string(usbd_device_handle dev, int si, char *buf,
+    size_t len);
 
 /* An iterator for descriptors. */
 typedef struct {

----Next_Part(Sat_Jun_30_09_08_18_2007_041)----

Comment 4 dfilter service freebsd_committer

2007-06-30 21:18:51 UTC

imp         2007-06-30 20:18:44 UTC

  FreeBSD src repository

  Modified files:
    sys/dev/usb          if_cdce.c uhub.c usb_subr.c usbdi.c 
                         usbdi.h 
  Log:
  Fix two more PRs:
  (1) Add size parameter to usbd_get_string()
  (2) Properly limit speed when a full speed hub is plugged into a high
      speed hub.
  
  Submitted by: Hans Petter Selasky
  PR: 80773, 79725
  Approved by: re@ (kensmith)
  
  Revision  Changes    Path
  1.25      +2 -1      src/sys/dev/usb/if_cdce.c
  1.82      +2 -1      src/sys/dev/usb/uhub.c
  1.95      +18 -2     src/sys/dev/usb/usb_subr.c
  1.103     +4 -2      src/sys/dev/usb/usbdi.c
  1.63      +2 -1      src/sys/dev/usb/usbdi.h
_______________________________________________
cvs-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/cvs-all
To unsubscribe, send any mail to "cvs-all-unsubscribe@freebsd.org"

Comment 5 Warner Losh freebsd_committer

2007-06-30 21:22:04 UTC

State Changed
From-To: suspended->patched

Committed.

Comment 6 Alfred Perlstein freebsd_committer

2007-07-01 02:53:03 UTC

no reason not to supply a usbd_get_stringn function which when given
'-1' trusts the device?

* M. Warner Losh <imp@bsdimp.com> [070630 08:08] wrote:
> Please find enclosed a patch for this.  I'm sitting on the fence as to
> whether or not to commit it, since it is an api/abi change.
> 
> Warner

> Index: if_cdce.c
> ===================================================================
> RCS file: /home/ncvs/src/sys/dev/usb/if_cdce.c,v
> retrieving revision 1.24
> diff -u -r1.24 if_cdce.c
> --- if_cdce.c	23 Jun 2007 06:47:43 -0000	1.24
> +++ if_cdce.c	30 Jun 2007 14:28:41 -0000
> @@ -280,7 +280,8 @@
>  
>  	ue = (const usb_cdc_ethernet_descriptor_t *)usb_find_desc(dev,
>  	    UDESC_INTERFACE, UDESCSUB_CDC_ENF);
> -	if (!ue || usbd_get_string(dev, ue->iMacAddress, eaddr_str)) {
> +	if (!ue || usbd_get_string(dev, ue->iMacAddress, eaddr_str,
> +	    sizeof(eaddr_str))) {
>  		/* Fake MAC address */
>  		device_printf(sc->cdce_dev, "faking MAC address\n");
>  		eaddr[0]= 0x2a;
> Index: uhub.c
> ===================================================================
> RCS file: /home/ncvs/src/sys/dev/usb/uhub.c,v
> retrieving revision 1.81
> diff -u -r1.81 uhub.c
> --- uhub.c	29 Jun 2007 20:34:42 -0000	1.81
> +++ uhub.c	30 Jun 2007 14:28:41 -0000
> @@ -655,7 +655,8 @@
>  
>  found_dev:
>  	/* XXX can sleep */
> -	(void)usbd_get_string(dev, dev->ddesc.iSerialNumber, &serial[0]);
> +	(void)usbd_get_string(dev, dev->ddesc.iSerialNumber, serial,
> +	    sizeof(serial));
>  	if (dev->ifacenums == NULL) {
>  		snprintf(buf, buflen, "vendor=0x%04x product=0x%04x "
>  		    "devclass=0x%02x devsubclass=0x%02x "
> Index: usb_subr.c
> ===================================================================
> RCS file: /home/ncvs/src/sys/dev/usb/usb_subr.c,v
> retrieving revision 1.94
> diff -u -r1.94 usb_subr.c
> --- usb_subr.c	20 Jun 2007 05:10:54 -0000	1.94
> +++ usb_subr.c	30 Jun 2007 14:28:42 -0000
> @@ -213,12 +213,14 @@
>  	}
>  
>  	if (usedev) {
> -		if (usbd_get_string(dev, udd->iManufacturer, v))
> +		if (usbd_get_string(dev, udd->iManufacturer, v,
> +		    USB_MAX_STRING_LEN))
>  			vendor = NULL;
>  		else
>  			vendor = v;
>  		usbd_trim_spaces(vendor);
> -		if (usbd_get_string(dev, udd->iProduct, p))
> +		if (usbd_get_string(dev, udd->iProduct, p,
> +		    USB_MAX_STRING_LEN))
>  			product = NULL;
>  		else
>  			product = p;
> Index: usbdi.c
> ===================================================================
> RCS file: /home/ncvs/src/sys/dev/usb/usbdi.c,v
> retrieving revision 1.102
> diff -u -r1.102 usbdi.c
> --- usbdi.c	20 Jun 2007 05:10:54 -0000	1.102
> +++ usbdi.c	30 Jun 2007 14:28:42 -0000
> @@ -1310,7 +1310,7 @@
>  }
>  
>  usbd_status
> -usbd_get_string(usbd_device_handle dev, int si, char *buf)
> +usbd_get_string(usbd_device_handle dev, int si, char *buf, size_t len)
>  {
>  	int swap = dev->quirks->uq_flags & UQ_SWAP_UNICODE;
>  	usb_string_descriptor_t us;
> @@ -1321,6 +1321,8 @@
>  	int size;
>  
>  	buf[0] = '\0';
> +	if (len == 0)
> +		return (USBD_NORMAL_COMPLETION);
>  	if (si == 0)
>  		return (USBD_INVAL);
>  	if (dev->quirks->uq_flags & UQ_NO_STRINGS)
> @@ -1342,7 +1344,7 @@
>  		return (err);
>  	s = buf;
>  	n = size / 2 - 1;
> -	for (i = 0; i < n; i++) {
> +	for (i = 0; i < n && i < len - 1; i++) {
>  		c = UGETW(us.bString[i]);
>  		/* Convert from Unicode, handle buggy strings. */
>  		if ((c & 0xff00) == 0)
> Index: usbdi.h
> ===================================================================
> RCS file: /home/ncvs/src/sys/dev/usb/usbdi.h,v
> retrieving revision 1.62
> diff -u -r1.62 usbdi.h
> --- usbdi.h	12 Jun 2007 19:40:20 -0000	1.62
> +++ usbdi.h	30 Jun 2007 14:28:42 -0000
> @@ -173,7 +173,8 @@
>  
>  int usbd_ratecheck(struct timeval *last);
>  
> -usbd_status usbd_get_string(usbd_device_handle dev, int si, char *buf);
> +usbd_status usbd_get_string(usbd_device_handle dev, int si, char *buf,
> +    size_t len);
>  
>  /* An iterator for descriptors. */
>  typedef struct {

> _______________________________________________
> freebsd-usb@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-usb
> To unsubscribe, send any mail to "freebsd-usb-unsubscribe@freebsd.org"


-- 
- Alfred Perlstein

Comment 7 Hans Petter Selasky 2007-07-02 07:53:30 UTC

On Saturday 30 June 2007 17:08, M. Warner Losh wrote:
> Please find enclosed a patch for this.  I'm sitting on the fence as to
> whether or not to commit it, since it is an api/abi change.
>
> Warner

It is not used that many places and I think they do it like this on NetBSD, if 
I don't recall wrong.

--HPS

Comment 8 M. Warner Losh 2007-07-02 14:46:28 UTC

In message: <200707020853.30905.hselasky@c2i.net>
            Hans Petter Selasky <hselasky@c2i.net> writes:
: On Saturday 30 June 2007 17:08, M. Warner Losh wrote:
: > Please find enclosed a patch for this.  I'm sitting on the fence as to
: > whether or not to commit it, since it is an api/abi change.
: >
: > Warner
: 
: It is not used that many places and I think they do it like this on NetBSD, if 
: I don't recall wrong.

I got off the fence, and committed it.

Warner

Comment 9 Mark Linimon freebsd_committer

2007-07-06 09:22:13 UTC

Responsible Changed
From-To: freebsd-usb->imp

imp committed the patch.

Comment 10 Warner Losh freebsd_committer

2009-04-13 16:49:27 UTC

State Changed
From-To: patched->closed

These have been in a release...