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

[Dmitry Torokhov]

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.

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/