Re: [PATCH] usb: interface: allow drivers declare number of endpoints they need

[Dmitry Torokhov]

On Tue, Dec 1, 2015 at 11:46 AM, Josh Boyer <jwboyer@xxxxxxxxxxxxxxxxx> wrote:
> On Mon, Nov 30, 2015 at 7:47 PM, Dmitry Torokhov
> <dmitry.torokhov@xxxxxxxxx> wrote:
>> On Mon, Nov 30, 2015 at 3:36 PM, Greg Kroah-Hartman
>> <gregkh@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx>
>>>> >> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
>>>> >> ---
>>>> >>
>>>> >> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/