Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 6 Nov 2009 00:26:14 GMT
From:      Robert Jenssen <robertjenssen@hotmail.com>
To:        freebsd-gnats-submit@FreeBSD.org
Subject:   usb/140325: Missing/incorrect initialisation and memory leak in libusb10/libusb20
Message-ID:  <200911060026.nA60QEoB073159@www.freebsd.org>
Resent-Message-ID: <200911060030.nA60U8mc078359@freefall.freebsd.org>

next in thread | raw e-mail | index | archive | help

>Number:         140325
>Category:       usb
>Synopsis:       Missing/incorrect initialisation and memory leak in libusb10/libusb20
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    freebsd-usb
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Fri Nov 06 00:30:07 UTC 2009
>Closed-Date:
>Last-Modified:
>Originator:     Robert Jenssen
>Release:        8.0RC2
>Organization:
>Environment:
FreeBSD kraken 8.0-RC2 FreeBSD 8.0-RC2 #0: Fri Nov  6 02:43:24 EST 2009     root@kraken:/usr/obj/usr/src/sys/KRAKEN  i386
>Description:
I was getting some weird values for usb configuration descriptor extra length. Valgrind is a wonderful tool recently ported to FreeBSD by stas@FreeBSD.org. Using valgrind I found the following problems (fixed in the attached patch):

1. In libusb10_desc.c, libusb_get_config_descriptor(), at line 162:
	pconfd->interface = (libusb_interface *) (pconfd +
	    sizeof(libusb_config_descriptor));
should be:
	pconfd->interface = (libusb_interface *) (pconfd + 1);
This problem causes illegal writes past the end of pconfd.

2. In libusb20_ugen20.c , ugen20_get_config_desc_full(), cdesc and ptr are not initialised. This problem causes branches on uninitialised values.

3. In libusb20.c, libusb20_be_free(), pbe is not free'd. This problem causes a minor memory leak.


>How-To-Repeat:
Compile the following test, link with a debug version of libusb.a and run valgrind.

#include <libusb.h>
int main(void) {
  libusb_context *context;
  struct libusb_device **devs;
  struct libusb_config_descriptor *config;

  libusb_init(&context);
  libusb_get_device_list(context, &devs);
  libusb_get_active_config_descriptor(devs[0], &config);
  libusb_free_config_descriptor(config);
  libusb_free_device_list(devs, 1);
  libusb_exit(context);
  return 0;
}

>Fix:
Apply the attached patch in /usr/src/lib/libusb


Patch attached with submission follows:

*** libusb10_desc.c	2009-11-06 10:35:00.000000000 +1100
--- libusb10_desc.c.orig	2009-08-03 18:13:06.000000000 +1000
***************
*** 116,133 ****
  	nep = 0;
  	nextra = pconf->extra.len;
  
- #define NEXTRA_ALIGN_TO(n) (nextra=((nextra+n)/n)*n)
  	for (i = 0; i < nif; i++) {
  
  		pinf = pconf->interface + i;
  		nextra += pinf->extra.len;
-     NEXTRA_ALIGN_TO(16);
  		nep += pinf->num_endpoints;
  		k = pinf->num_endpoints;
  		pend = pinf->endpoints;
  		while (k--) {
  			nextra += pend->extra.len;
-       NEXTRA_ALIGN_TO(16);
  			pend++;
  		}
  
--- 116,130 ----
***************
*** 136,148 ****
  		pinf = pinf->altsetting;
  		while (j--) {
  			nextra += pinf->extra.len;
-       NEXTRA_ALIGN_TO(16);
  			nep += pinf->num_endpoints;
  			k = pinf->num_endpoints;
  			pend = pinf->endpoints;
  			while (k--) {
  				nextra += pend->extra.len;
-         NEXTRA_ALIGN_TO(16);
  				pend++;
  			}
  			pinf++;
--- 133,143 ----
***************
*** 155,163 ****
  	    (nalt * sizeof(libusb_interface_descriptor)) +
  	    (nep * sizeof(libusb_endpoint_descriptor));
  
-   /* Align nextra */
-   NEXTRA_ALIGN_TO(16);
- 
  	pconfd = malloc(nextra);
  
  	if (pconfd == NULL) {
--- 150,155 ----
***************
*** 167,173 ****
  	/* make sure memory is clean */
  	memset(pconfd, 0, nextra);
  
! 	pconfd->interface = (libusb_interface *) (pconfd + 1);
  
  	ifd = (libusb_interface_descriptor *) (pconfd->interface + nif);
  	endd = (libusb_endpoint_descriptor *) (ifd + nalt);
--- 159,166 ----
  	/* make sure memory is clean */
  	memset(pconfd, 0, nextra);
  
! 	pconfd->interface = (libusb_interface *) (pconfd +
! 	    sizeof(libusb_config_descriptor));
  
  	ifd = (libusb_interface_descriptor *) (pconfd->interface + nif);
  	endd = (libusb_endpoint_descriptor *) (ifd + nalt);
***************
*** 194,200 ****
  
  	for (i = 0; i < nif; i++) {
  
- 		pconfd->interface[i].altsetting = 0;
  		pconfd->interface[i].altsetting = ifd;
  		ifd->endpoint = endd;
  		endd += pconf->interface[i].num_endpoints;
--- 187,192 ----
*** libusb20.c	2009-11-06 10:35:00.000000000 +1100
--- libusb20.c.orig	2009-08-03 18:13:06.000000000 +1000
***************
*** 1093,1100 ****
  	if (pbe->methods->exit_backend) {
  		pbe->methods->exit_backend(pbe);
  	}
-   /* free backend */
-   free(pbe);
  	return;
  }
  
--- 1093,1098 ----
*** libusb20_desc.c	2009-11-06 10:35:00.000000000 +1100
--- libusb20_desc.c.orig	2009-08-03 18:13:06.000000000 +1000
***************
*** 118,124 ****
  	if (lub_config == NULL) {
  		return (NULL);		/* out of memory */
  	}
-   memset(lub_config, 0, size);
  	lub_interface = (void *)(lub_config + 1);
  	lub_alt_interface = (void *)(lub_interface + niface_no_alt);
  	lub_endpoint = (void *)(lub_interface + niface);
--- 118,123 ----
*** libusb20_ugen20.c	2009-11-06 10:35:00.000000000 +1100
--- libusb20_ugen20.c.orig	2009-10-23 23:02:01.000000000 +1100
***************
*** 449,455 ****
  	uint16_t len;
  	int error;
  
- 	memset(&cdesc, 0, sizeof(cdesc));
  	memset(&gen_desc, 0, sizeof(gen_desc));
  
  	gen_desc.ugd_data = &cdesc;
--- 449,454 ----
***************
*** 469,475 ****
  	if (!ptr) {
  		return (LIBUSB20_ERROR_NO_MEM);
  	}
-   memset(ptr, 0, len);
  	gen_desc.ugd_data = ptr;
  	gen_desc.ugd_maxlen = len;
  
--- 468,473 ----


>Release-Note:
>Audit-Trail:
>Unformatted:



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200911060026.nA60QEoB073159>