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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Mon, Nov 30, 2015 at 04:47:18PM -0800, Dmitry Torokhov wrote:
> On Mon, Nov 30, 2015 at 3:36 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.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@linuxfoundation.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@rowland.harvard.edu>
> >> >> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.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.

Yes, that would work for one single type of endpoint, but lots of
drivers need/have 2 of the same type/direction, so what would this
function do then?  Error out?  Hm, that might work, and it would reduce
a bunch of common code, care to make up a patch for that?

thanks,

greg k-h