Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 27 Aug 2018 15:28:15 +0200
From:   Johan Hovold <johan@kernel.org>
To:     Romain Izard <romain.izard.pro@gmail.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Johan Hovold <johan@kernel.org>, linux-usb@vger.kernel.org,
        LKML <linux-kernel@vger.kernel.org>,
        stable <stable@vger.kernel.org>,
        =?iso-8859-1?Q?Bj=F8rn?= Mork <bjorn@mork.no>,
        Lars Melin <larsm17@gmail.com>
Subject: Re: [PATCH] option: Do not try to bind to ADB interfaces
Message-ID: <20180827132815.GD14967@localhost>
References: <20180723140220.7166-1-romain.izard.pro@gmail.com>
 <20180723140801.GA4835@kroah.com>
 <CAGkQfmPbuK7-f=9GcvQPH6mkHXwsaGpjqv40DLgFUJmbOCpbgQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAGkQfmPbuK7-f=9GcvQPH6mkHXwsaGpjqv40DLgFUJmbOCpbgQ@mail.gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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