From owner-freebsd-usb@FreeBSD.ORG Fri Nov 6 08:52:32 2009 Return-Path: Delivered-To: freebsd-usb@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 5F4A0106566B; Fri, 6 Nov 2009 08:52:32 +0000 (UTC) (envelope-from hselasky@c2i.net) Received: from swip.net (mailfe16.tele2.se [212.247.155.225]) by mx1.freebsd.org (Postfix) with ESMTP id 89F878FC21; Fri, 6 Nov 2009 08:52:31 +0000 (UTC) X-Cloudmark-Score: 0.000000 [] X-Cloudmark-Analysis: v=1.0 c=1 a=JhkvhRibbtIA:10 a=MnI1ikcADjEx7bvsp0jZvQ==:17 a=6I5d2MoRAAAA:8 a=miH4J3ebS2kzY9m3ClQA:9 a=0Qhij3TW4Wc2f62AdjwA:7 a=IAq8naS0Nz3HJEQWETfPczCoFXUA:4 a=SV7veod9ZcQA:10 a=xJ1uwIrbEd-Rv1g9:21 a=pZJ9fp19uh5bu4rS:21 Received: from [188.126.201.140] (account mc467741@c2i.net HELO laptop.adsl.tele2.no) by mailfe16.swip.net (CommuniGate Pro SMTP 5.2.16) with ESMTPA id 577851993; Fri, 06 Nov 2009 09:52:29 +0100 From: Hans Petter Selasky To: freebsd-usb@freebsd.org Date: Fri, 6 Nov 2009 09:51:34 +0100 User-Agent: KMail/1.11.4 (FreeBSD/9.0-CURRENT; KDE/4.2.4; i386; ; ) References: <200911060026.nA60QEoB073159@www.freebsd.org> In-Reply-To: <200911060026.nA60QEoB073159@www.freebsd.org> X-Face: (%:6u[ldzJ`0qjD7sCkfdMmD*RxpOwEEQ+KWt[{J#x6ow~JO:,zwp.(t; @Aq :4:&nFCgDb8[3oIeTb^'",;u{5{}C9>"PuY\)!=#\u9SSM-nz8+SR~B\!qBv MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200911060951.34794.hselasky@c2i.net> Cc: freebsd-gnats-submit@freebsd.org, Robert Jenssen 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 List-Id: FreeBSD support for USB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 06 Nov 2009 08:52:32 -0000 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