| Message ID | 20180723140220.7166-1-romain.izard.pro@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | option: Do not try to bind to ADB interfaces | expand |
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@gmail.com> > Cc: stable <stable@vger.kernel.org> > --- > 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. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7/23/2018 21:02, 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. You are assuming that an interface with these attributes are always a ADB interface - that is wrong. Vendor specific class (0xff) is not standardized to be something specific. > This can fix issues like: > https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=781256 > You are trying to solve a 4++ years old bug report where it was assumed that the option driver was the culprit. The device in question, a Qualcomm modem with vid/pid 05c6:9025 has never been included in option. rgds Lars -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2018-07-23 17:15 GMT+02:00 Lars Melin <larsm17@gmail.com>: > On 7/23/2018 21:02, 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. > > > You are assuming that an interface with these attributes are always a > ADB interface - that is wrong. Vendor specific class (0xff) is not > standardized to be something specific. Yes. And the option driver binds to all the vendor-specific interfaces for many devices, assuming all the vendor-specific interfaces are serial ports, unless they are blacklisted. > >> This can fix issues like: >> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=781256 >> > > You are trying to solve a 4++ years old bug report where it was assumed that > the option driver was the culprit. The device in question, a Qualcomm modem > with vid/pid 05c6:9025 has never been included in option. While I'm not going to investigate why the 3.16 kernel did it, in the bug report the logs indicated that 05c6:9025 was bound to the option driver. But I did not try to solve this issue directly, I reported it as it led me to the solution for a similar problem that I encountered last week. A vendor for an SDK based on a modem was using an old Ubuntu 16.04, and everything was working correctly. But on my 18.04, and on an up-to-date 16.04 LTS, it was impossible to use ADB with the vendor's SDK. As ADB has many other failure modes, it took a lot of time to see the source of the problem. The option driver in the old Ubuntu did not know about the modem's vid/pid, so the driver was not bound by default and the ADB interface was free. But the modem was added in the stable branch of the kernel, so the driver was greedily binding to all interfaces that were not blacklisted. As a result, it was not possible to use ADB as long as the interface was bound to the option driver. One of the reasons for the problem, of course, was the reuse by the modem manufacturer of the same vid/pid for the standard version of the modem and for the SDK version. I see my change as a useful heuristic. ADB is a quite common protocol, using one of the 65536 vendor subclass/protocol combinations. As you can see in https://github.com/apkudo/adbusbini more than 3000 vendors have been spotted using it in the wild, including more than half of those mentioned in the option driver. And on the other hand, I don't expect any existing modem to use 0xff,0x42,0x01 for its serial port interface, and a USB facedancer would not gain anything by doing so. Best regards,
2018-07-23 16:08 GMT+02:00 Greg Kroah-Hartman <gregkh@linuxfoundation.org>: > 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@gmail.com> >> Cc: stable <stable@vger.kernel.org> >> --- >> 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. As this is only an intuition, please discard this patch if you have any doubt about this. Best regards,
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@linuxfoundation.org>: > > 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@gmail.com> > >> Cc: stable <stable@vger.kernel.org> > >> --- > >> 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
Johan Hovold <johan@kernel.org> writes: > 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. +1 The adb userspace application is also free to unbind any conflicting driver, so I don't think blacklisting is strictly necessary. Except to prevent any confusion caused by bogus ttyUSBx devices. Bjørn
On Mon, Aug 27, 2018 at 06:15:52PM +0200, Bjørn Mork wrote: > Johan Hovold <johan@kernel.org> writes: > > > 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. > > +1 > > The adb userspace application is also free to unbind any conflicting > driver, so I don't think blacklisting is strictly necessary. Except to > prevent any confusion caused by bogus ttyUSBx devices. Right. Let's leave things as they are then. Thanks, Johan
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; + /* * Don't bind reserved interfaces (like network ones) which often have * the same class/subclass/protocol as the serial interfaces. Look at
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@gmail.com> Cc: stable <stable@vger.kernel.org> --- drivers/usb/serial/option.c | 6 ++++++ 1 file changed, 6 insertions(+)