From owner-freebsd-usb@FreeBSD.ORG Fri Nov 6 09:00:10 2009 Return-Path: Delivered-To: freebsd-usb@hub.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 2E04D10656A6 for ; Fri, 6 Nov 2009 09:00:10 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by mx1.freebsd.org (Postfix) with ESMTP id 46E078FC2A for ; Fri, 6 Nov 2009 09:00:09 +0000 (UTC) Received: from freefall.freebsd.org (localhost [127.0.0.1]) by freefall.freebsd.org (8.14.3/8.14.3) with ESMTP id nA690867052243 for ; Fri, 6 Nov 2009 09:00:08 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.3/8.14.3/Submit) id nA6908Z0052242; Fri, 6 Nov 2009 09:00:08 GMT (envelope-from gnats) Date: Fri, 6 Nov 2009 09:00:08 GMT Message-Id: <200911060900.nA6908Z0052242@freefall.freebsd.org> To: freebsd-usb@FreeBSD.org From: Hans Petter Selasky Cc: Subject: Re: usb/140325: Missing/incorrect initialisation and memory leak in libusb10/libusb20 X-BeenThere: freebsd-usb@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: Hans Petter Selasky List-Id: FreeBSD support for USB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 06 Nov 2009 09:00:10 -0000 The following reply was made to PR usb/140325; it has been noted by GNATS. From: Hans Petter Selasky To: freebsd-usb@freebsd.org Cc: Robert Jenssen , freebsd-gnats-submit@freebsd.org Subject: Re: usb/140325: Missing/incorrect initialisation and memory leak in libusb10/libusb20 Date: Fri, 6 Nov 2009 09:51:34 +0100 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@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 > 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