linux-kernel - Re: [PATCH] usb: storage: Add ums-cros-aoa driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAODwPW-+c6Ty_gqEFEaE0YhtutMR2tFnhEFOQre81uyM3mfMVA@mail.gmail.com>
Date:   Wed, 28 Aug 2019 20:26:15 -0700
From:   Julius Werner <jwerner@...omium.org>
To:     Dan Williams <dcbw@...hat.com>
Cc:     Alan Stern <stern@...land.harvard.edu>,
        Julius Werner <jwerner@...omium.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Kernel development list <linux-kernel@...r.kernel.org>,
        USB list <linux-usb@...r.kernel.org>,
        USB Storage list <usb-storage@...ts.one-eyed-alien.net>
Subject: Re: [PATCH] usb: storage: Add ums-cros-aoa driver

(Thanks for the reviews... I'll get back to the kernel code details
after double-checking if this can be done from userspace.)

> > Besides, what's wrong with binding to devices that weren't switched
> > into AOA mode?  Would that just provoke a bunch of unnecessary error
> > messages?

It's not about devices that aren't switched into AOA mode... it's
about devices that are switched into AOA mode for other reasons
(connecting to other Android apps). I don't think a kernel driver like
that exists today, but it could be added, or people could use libusb
to talk to an AOA device. AOA is just a general mechanism to talk to
an Android app for whatever you want, and the descriptors sent during
mode switch clarify what app it's talking to (and thereby what
protocol it is using... it could be mass storage or it could be
something entirely different). But a device switched into AOA mode for
whatever app will always use that same well-known VID/PID (18d1:2d00).
So if I just add that VID/PID to the IDs bound by the usb-storage
driver, it would also grab a device that was mode-switched by
userspace to talk to a completely different app. I need some way to
make sure it only grabs the intended device, and there's no good
identifier for that other than comparing the dev path to what you
originally mode switched.

> > > +     /*
> > > +      * Only interface 0 connects to the AOA app. Android devices that have
> > > +      * ADB enabled also export an interface 1. We don't want it.
> > > +      */
> > > +     if (us->pusb_intf->cur_altsetting->desc.bInterfaceNumber != 0)
> > > +             return -ENODEV;
> >
> > Do you really need this test?  What would go wrong if you don't do it?

Yes, otherwise two separate usb-storage instances bind to the two
interfaces. The second interface is meant for a special ADB debugging
protocol and will not respond at all to USB mass storage packets, so
eventually the first request to it times out and
usb_stor_invoke_transport() will do a port reset to recover. That also
kills the first interface asynchronously even though it was working
fine.

> > IMO the userspace approach would be better, unless you can provide a
> > really compelling argument for why it won't suffice.

Well... okay, let me think through that again. I just found the new_id
sysfs API that I wasn't aware of before, maybe I could leverage that
to bind this from userspace after doing the mode switch. But it looks
like that only operates on whole devices... is there any way to force
it to only bind one particular interface?