Re: [PATCH] option: Do not try to bind to ADB interfaces

[Johan Hovold]

On Mon, Jul 23, 2018 at 06:45:22PM +0200, Romain Izard wrote:
> 2018-07-23 16:08 GMT+02:00 Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>:
> > On Mon, Jul 23, 2018 at 04:02:20PM +0200, Romain Izard wrote:
> >> Some modems now use the Android Debug Bridge to provide a debugging
> >> interface, and some phones can also export serial ports managed by the
> >> "option" driver.
> >>
> >> The ADB daemon running in userspace tries to use USB interfaces with
> >> bDeviceClass=0xFF, bDeviceSubClass=0x42, bDeviceProtocol=1
> >>
> >> Prevent the option driver from binding to those interfaces, as they
> >> will not be serial ports.
> >>
> >> This can fix issues like:
> >> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=781256
> >>
> >> Signed-off-by: Romain Izard <romain.izard.pro@xxxxxxxxx>
> >> Cc: stable <stable@xxxxxxxxxxxxxxx>
> >> ---
> >>  drivers/usb/serial/option.c | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/drivers/usb/serial/option.c b/drivers/usb/serial/option.c
> >> index 664e61f16b6a..f98943a57ff0 100644
> >> --- a/drivers/usb/serial/option.c
> >> +++ b/drivers/usb/serial/option.c
> >> @@ -1987,6 +1987,12 @@ static int option_probe(struct usb_serial *serial,
> >>       if (iface_desc->bInterfaceClass == USB_CLASS_MASS_STORAGE)
> >>               return -ENODEV;
> >>
> >> +     /* Do not bind Android Debug Bridge interfaces */
> >> +     if (iface_desc->bInterfaceClass == USB_CLASS_VENDOR_SPEC &&
> >> +             iface_desc->bInterfaceSubClass == 0x42 &&
> >> +             iface_desc->bInterfaceProtocol == 1)
> >> +             return -ENODEV;
> >
> > Shouldn't you also check the vendor/product id as well?  Otherwise this
> > has the potential to match random devices that are not really adb
> > devices.
> 
> The only random devices are those that already match with the option driver,
> either with the whole device or the whole reserved class. It reduces the
> amount of potentially affected devices.
> 
> Among those, I do not expect any of them to use 0xff,0x42,0x01 for a
> serial port. But if it occurred, it would be necessary to revert this change as
> no userspace hack would allow to rebind the interface.

Yeah, but by that time we may have added (or enabled) devices that rely
on this general rule so we'd need to start adding exceptions to this
negative matching rule instead...

> As this is only an intuition, please discard this patch if you have any doubt
> about this.

Bjørn also suggested that we at least consider adding a rule like this a
few months ago (when I changed the blacklist implementation). I just
never got around to look into it.

It would allow for simpler device-id entries, at least when ADB is the
only blacklisted interface, and may enable ADB for some older entries.

On the other hand, interface class 0xff is indeed supposed to be vendor
specific as Lars and Greg pointed out, and with status quo we don't
cause any regressions. If ADB isn't currently available for some device
due to option binding to that interface, we'll just blacklist it as soon
we get a report.

So personally I'm not sure it's worth it, but I don't have a strong
opinion on the matter either.

Johan