Message-ID: <CA+5PVA43qT7=fOfgLN3wo8S22Gcq3E1u+RO4LfECgdCA1YQdbA@mail.gmail.com>
Date:	Tue, 1 Dec 2015 14:46:33 -0500
From:	Josh Boyer <jwboyer@...oraproject.org>
To:	Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Alan Stern <stern@...land.harvard.edu>,
	Felipe Balbi <balbi@...com>,
	Vladis Dronov <vdronov@...hat.com>,
	USB list <linux-usb@...r.kernel.org>,
	lkml <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] usb: interface: allow drivers declare number of endpoints
 they need

On Mon, Nov 30, 2015 at 7:47 PM, Dmitry Torokhov
<dmitry.torokhov@...il.com> wrote:
> On Mon, Nov 30, 2015 at 3:36 PM, Greg Kroah-Hartman
> <gregkh@...uxfoundation.org> 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
>>> <gregkh@...uxfoundation.org> 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 <stern@...land.harvard.edu>
>>> >> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@...il.com>
>>> >> ---
>>> >>
>>> >> 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists