Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755388AbbLATrv (ORCPT ); Tue, 1 Dec 2015 14:47:51 -0500 Received: from mail-ob0-f178.google.com ([209.85.214.178]:35560 "EHLO mail-ob0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750931AbbLATrt (ORCPT ); Tue, 1 Dec 2015 14:47:49 -0500 MIME-Version: 1.0 In-Reply-To: References: <20151130211150.GA38770@dtor-ws> <20151130221825.GA6426@kroah.com> <20151130233653.GB8414@kroah.com> Date: Tue, 1 Dec 2015 11:47:48 -0800 Message-ID: Subject: Re: [PATCH] usb: interface: allow drivers declare number of endpoints they need From: Dmitry Torokhov To: Josh Boyer Cc: Greg Kroah-Hartman , Alan Stern , Felipe Balbi , Vladis Dronov , USB list , lkml Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6575 Lines: 151 On Tue, Dec 1, 2015 at 11:46 AM, Josh Boyer wrote: > On Mon, Nov 30, 2015 at 7:47 PM, 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. >> >> Let's consider this patch dropped. > > Since you're dropping this patch, are you going to take the patch > Vladis originally sent for the aiptek driver? I'm not objecting to > fixing this in a broader sense, but it might be good to get existing > fixes in before the whole rework is done. Yeah, I'd better. Thanks. -- Dmitry -- 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/