Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755265AbbLATqg (ORCPT ); Tue, 1 Dec 2015 14:46:36 -0500 Received: from mail-vk0-f41.google.com ([209.85.213.41]:33433 "EHLO mail-vk0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751837AbbLATqf (ORCPT ); Tue, 1 Dec 2015 14:46:35 -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 14:46:33 -0500 X-Google-Sender-Auth: tHll0KXND-0jR_h6zMG5rwtP6NE Message-ID: Subject: Re: [PATCH] usb: interface: allow drivers declare number of endpoints they need From: Josh Boyer To: Dmitry Torokhov 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: 6318 Lines: 145 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. josh -- 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/