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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAODwPW8gTZ_2WEc9n=WJ2PEmQk2anTQYfwQ-898+kOq6wsjnZw@mail.gmail.com>
Date:   Thu, 29 Aug 2019 17:31:12 -0700
From:   Julius Werner <jwerner@...omium.org>
To:     Alan Stern <stern@...land.harvard.edu>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Julius Werner <jwerner@...omium.org>,
        Dan Williams <dcbw@...hat.com>,
        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

> USB drivers only bind to interfaces, are you saying that your device has
> multiple interfaces on it?

Yes, I have a case where the device has two interfaces which both have
interface class 0xff (although they do differ in subclass and
protocol). I only want the usb-storage driver to bind to one of them
(if it binds to the other it will eventually cause a timeout error
that resets the whole device).

I tried doing a userspace mode switch and using
/sys/bus/usb/drivers/usb-storage/new_id to bind the device now, which
*almost* works, but I can't prevent it from binding to both
interfaces. Unfortunately new_id can only take an interface class, not
a subclass or protocol.

> In fact, there already is a way to do this in the kernel: write to the
> sysfs "bind" file.  The difficulty is that you can't force a driver to
> bind to an interface if it doesn't believe it is compatible with the
> interface.  And if the driver believes it is compatible, it will
> automatically attempt to bind with all such interfaces regardless of
> their path.
>
> Perhaps what you need is a usb_device_id flag to indicate that the
> entry should never be used for automatic matches -- only for matches
> made by the user via the "bind" file.  Greg KH would probably be
> willing to accept a new USB_DEVICE_ID_MATCH_NO_AUTO flag, which
> could be included in your unusual_devs.h entries.

This is an interesting idea, but I don't quite see how it can work as
you described? When I write to 'bind', the driver core calls
driver_match_device(), which ends up calling usb_device_match()
(right?), which is the same path that it would call for automatic
matching. It still ends up in usb_match_one_id(), and if I check for
the NO_AUTO flag there it would abort just as if it was an auto-match
attempt. I see no way to pass the information that this is an
explicit, user-requested "bind" rather than an automatic match across
the bus->match() callback into the USB code. (I could change the
signature of the match() callback, but that would require changing
code for all device busses in Linux, which I assume is something we
wouldn't want to do? I could also add a flag to struct device to
communicate "this is currently trying to match for a user-initiated
bind", but that seems hacky and not really the right place to put
that.)

I could instead add a new sysfs node 'force_bind' to the driver core,
that would work like 'bind' except for skipping the
driver_match_device() call entirely and forcing a probe(). Do you
think that would be acceptable? Or is that too big of a hammer to make
available for all drivers in Linux? Maybe if I do the same thing but
only for usb drivers, or even only for the usb-storage driver
specifically, would that be acceptable?

If none of this works, I could also extend the new_id interface to
allow subclass/protocol matches instead. I don't like that as much
because it doesn't allow me to control the devpath of the device I'm
matching, but I think it would be enough for my use case (I can't make
the usb-storage driver bind all AOA devices at all times, but at the
times where I do want to use it for my one device, I don't expect any
other AOA devices to be connected). The problem with this is that the
order of arguments for new ID is already set in stone (vendor,
product, interface class, refVendor, refProduct), and I don't think I
can use the refVendor/refProduct for my case so I can't just append
more numbers behind that. I could maybe instead change it so that it
also accepts a key-value style line (like "idVendor=abcd
idProduct=efgh bInterfaceSubClass=ff"), while still being
backwards-compatible to the old format if you only give it numbers?
What do you think?

Thanks for your advice!