usb/140325: Missing/incorrect initialisation and memory leak in libusb10/libusb20

[Hans Petter Selasky]

On Friday 06 November 2009 01:26:14 Robert Jenssen wrote:
> >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 at 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 at 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:
>

I'm working on these issues.

--HPS