Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755495AbbLABJN (ORCPT ); Mon, 30 Nov 2015 20:09:13 -0500 Received: from mail.linuxfoundation.org ([140.211.169.12]:37721 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755299AbbLABJK (ORCPT ); Mon, 30 Nov 2015 20:09:10 -0500 Date: Mon, 30 Nov 2015 17:09:09 -0800 From: Greg Kroah-Hartman To: Dmitry Torokhov Cc: Alan Stern , Felipe Balbi , Vladis Dronov , USB list , lkml Subject: Re: [PATCH] usb: interface: allow drivers declare number of endpoints they need Message-ID: <20151201010909.GA13673@kroah.com> References: <20151130211150.GA38770@dtor-ws> <20151130221825.GA6426@kroah.com> <20151130233653.GB8414@kroah.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6408 Lines: 144 On Mon, Nov 30, 2015 at 04:47:18PM -0800, Dmitry Torokhov wrote: > On Mon, Nov 30, 2015 at 3:36 PM, Greg Kroah-Hartman > wrote: > > On Mon, Nov 30, 2015 at 02:56:09PM -0800, Dmitry Torokhov wrote: > >> On Mon, Nov 30, 2015 at 2:18 PM, Greg Kroah-Hartman > >> wrote: > >> > On Mon, Nov 30, 2015 at 01:11:50PM -0800, Dmitry Torokhov wrote: > >> >> USB interface drivers need to check number of endpoints before trying to > >> >> access/use them. Quite a few drivers only use the default setting > >> >> (altsetting 0), so let's allow them to declare number of endpoints in > >> >> altsetting 0 they require to operate and have USB core check it for us > >> >> instead of having every driver implement check manually. > >> >> > >> >> For compatibility, if driver does not specify number of endpoints (i.e. > >> >> number of endpoints is left at 0) we bypass the check in USB core and > >> >> expect the driver perform necessary checks on its own. > >> >> > >> >> Acked-by: Alan Stern > >> >> Signed-off-by: Dmitry Torokhov > >> >> --- > >> >> > >> >> Greg, if the patch is reasonable I wonder if I can take it through my > >> >> tree, as I have a few drivers that do not check number of endpoints > >> >> properly and will crash the kernel when specially crafted device is > >> >> plugged in, as reported by Vladis Dronov. > >> >> > >> >> drivers/usb/core/driver.c | 9 +++++++++ > >> >> include/linux/usb.h | 7 +++++++ > >> >> 2 files changed, 16 insertions(+) > >> >> > >> >> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c > >> >> index 6b5063e..d9f680d 100644 > >> >> --- a/drivers/usb/core/driver.c > >> >> +++ b/drivers/usb/core/driver.c > >> >> @@ -306,6 +306,15 @@ static int usb_probe_interface(struct device *dev) > >> >> > >> >> dev_dbg(dev, "%s - got id\n", __func__); > >> >> > >> >> + if (driver->num_endpoints && > >> >> + intf->altsetting[0].desc.bNumEndpoints < driver->num_endpoints) { > >> >> + > >> > > >> > Empty line :( > >> > > >> >> + dev_err(dev, "Not enough endpoints %d (want %d)\n", > >> >> + intf->altsetting[0].desc.bNumEndpoints, > >> >> + driver->num_endpoints); > >> > > >> > What can a user do with this? > >> > >> Report on the lists or throw such device into a bin. > >> > >> > > >> >> + return -EINVAL; > >> >> + } > >> >> + > >> >> error = usb_autoresume_device(udev); > >> >> if (error) > >> >> return error; > >> >> diff --git a/include/linux/usb.h b/include/linux/usb.h > >> >> index 447fe29..93f8dfc 100644 > >> >> --- a/include/linux/usb.h > >> >> +++ b/include/linux/usb.h > >> >> @@ -1051,6 +1051,11 @@ struct usbdrv_wrap { > >> >> * @id_table: USB drivers use ID table to support hotplugging. > >> >> * Export this with MODULE_DEVICE_TABLE(usb,...). This must be set > >> >> * or your driver's probe function will never get called. > >> >> + * @num_endpoints: Number of endpoints that should be present in default > >> >> + * setting (altsetting 0) the driver needs to operate properly. > >> >> + * The probe will be aborted if actual number of endpoints is less > >> >> + * than what the driver specified here. 0 means no check should be > >> >> + * performed. > >> > > >> > I don't understand, a driver can do whatever it wants with the endpoints > >> > of the interface, why do we need to check/know this ahead of time? What > >> > is crashing without this? > >> > >> The kernel because some drivers do not verify that > >> intf->altsetting[0].desc.bNumEndpoints >= 1 before referencing > >> intf->altsetting[0].endpoints[0]. > > > > The USB core does that? Or just a driver, and if it's just a driver, we > > should fix that in the driver itself as there are lots of other > > validation checks the drivers should be doing becides just this one > > about endpoints, sizes, and directions that we can't catch in the core. > > > >> > It's up to the driver to check this, if it cares about it. > >> > >> Instead of duplicating the check in almost every driver is it more > >> efficient to allow USB core check it for them (if driver requests it > >> to do so). > > > > ok, fair enough, but it's just one of many things they should be > > checking, this doesn't provide all that much "protection". > > > >> > How many > >> > drivers do you have that is going to care? > >> > >> I saw at least 3 that did not check, that's from cursory glance. Plus > >> we have many that do check explicitly. > >> > >> > Why is this suddenly a new > >> > thing that we haven't run into in the past 15+ years? > >> > >> We are less trusting now. Before we/some of the drivers believed that > >> if device has VID/PID that they recognize the rest of descriptors will > >> have the data we expect, but we can't rely on this anymore. > > > > There's lots of things we can't "rely on", and we have never been able > > to rely on, but this is going to require we touch every USB driver to > > make those changes, this one change isn't going to really do all that > > much to help out with that. > > > > Every USB driver _should_ be having a loop over all endpoints to find > > what they need/expect, and if it isn't there, then it needs to abort. > > Just checking the number of endpoints isn't ok, so I really think this > > isn't going to help all that much in the end... > > OK, fair enough. Maybe what is missing is something like: > > ep = usb_locate_endpoint(altsetting, type, direction); > if (!ep) { > ... > return -EINVAL; > } > > that would loop through endpoints so that drivers do not have to > open-code the loop and we indeed need to fix the drivers that blindly > grab endpoints at fixed offsets and expect them to be there and have > correct types. Yes, that would work for one single type of endpoint, but lots of drivers need/have 2 of the same type/direction, so what would this function do then? Error out? Hm, that might work, and it would reduce a bunch of common code, care to make up a patch for that? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/