Date: Sat, 7 Nov 2009 01:10:03 GMT From: Hans Petter Selasky <hselasky@c2i.net> To: freebsd-usb@FreeBSD.org Subject: Re: usb/140325: Missing/incorrect initialisation and memory leak in libusb10/libusb20 Message-ID: <200911070110.nA71A3cG064305@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR usb/140325; it has been noted by GNATS.
From: Hans Petter Selasky <hselasky@c2i.net>
To: freebsd-usb@freebsd.org
Cc: Robert Jenssen <robertjenssen@hotmail.com>,
freebsd-gnats-submit@freebsd.org
Subject: Re: usb/140325: Missing/incorrect initialisation and memory leak in libusb10/libusb20
Date: Sat, 7 Nov 2009 02:05:43 +0100
On Friday 06 November 2009 09:51:34 Hans Petter Selasky wrote:
> 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 <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
First set of patches has been commited to USB P4 with some modifications.
Please verify:
http://p4web.freebsd.org/chv.cgi?CH=170304
--HPS
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200911070110.nA71A3cG064305>