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

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

[Romain Izard]

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;
+
 	/*
 	 * Don't bind reserved interfaces (like network ones) which often have
 	 * the same class/subclass/protocol as the serial interfaces.  Look at
-- 
2.17.1

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

[Greg Kroah-Hartman]

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

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

[Lars Melin]

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

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

[Romain Izard]

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,
-- 
Romain Izard

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

[Romain Izard]

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,
-- 
Romain Izard

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@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

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

[Bjørn Mork]

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

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

[Johan Hovold]

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