Re: Bug caused by 53965c79c2db (USB: Fix device driver race)

[Bastien Nocera <hadess@hadess.net>]

On Wed, 2020-10-21 at 00:18 -0400, M. Vefa Bicakci wrote:
> On 20/10/2020 11.28, Alan Stern wrote:
> > On Tue, Oct 20, 2020 at 08:03:23AM -0400, M. Vefa Bicakci wrote:
> > > On 19/10/2020 13.40, Alan Stern wrote:
> > > > On Mon, Oct 19, 2020 at 09:36:00AM +0000, Pany wrote:
> [Snipped by Vefa]
> > > 
> > > ... and Pany's system has multiple USB devices manufactured by
> > > Apple
> > > (including the SD card reader), with the vendor code 0x05ac,
> > > which is
> > > the value included by the id_table of the apple-mfi-fastcharge
> > > driver:
> > > 
> > > Sep 29 15:22:48 fedora.local kernel: usb 2-3: new SuperSpeed Gen
> > > 1 USB device number 2 using xhci_hcd
> > > Sep 29 15:22:48 fedora.local kernel: usb 2-3: New USB device
> > > found, idVendor=05ac, idProduct=8406, bcdDevice= 8.20
> > > Sep 29 15:22:48 fedora.local kernel: usb 2-3: New USB device
> > > strings: Mfr=3, Product=4, SerialNumber=5
> > > Sep 29 15:22:48 fedora.local kernel: usb 2-3: Product: Card
> > > Reader
> > > Sep 29 15:22:48 fedora.local kernel: usb 2-3: Manufacturer: Apple
> > > ...
> > > Sep 29 15:22:48 fedora.local kernel: usb 1-5: new full-speed USB
> > > device number 3 using xhci_hcd
> > > Sep 29 15:22:48 fedora.local kernel: usb 1-5: New USB device
> > > found, idVendor=05ac, idProduct=0273, bcdDevice= 6.22
> > > Sep 29 15:22:48 fedora.local kernel: usb 1-5: New USB device
> > > strings: Mfr=1, Product=2, SerialNumber=3
> > > Sep 29 15:22:48 fedora.local kernel: usb 1-5: Product: Apple
> > > Internal Keyboard / Trackpad
> > > Sep 29 15:22:48 fedora.local kernel: usb 1-5: Manufacturer: Apple
> > > Inc.
> > > ...
> > > 
> > > One way to fix this issue would be to implement a match function
> > > in the
> > > apple-mfi-fastcharge driver, possibly instead of an id_table, so
> > > that it
> > > does not match devices that it cannot bind to. This may require
> > > other
> > > changes in the USB core that I cannot fathom at the moment.
> > 
> > How about matching on the vendor ID and the product ID?  That would
> > be
> > an easy addition to the ID table.  Do the fastcharge devices have a
> > well
> > known product ID?
> > 
> > Alan Stern
> 
> Hello Alan,
> 
> Thank you for the feedback! Judging from the driver's code, it looks
> like
> the product identifiers are known, but there unfortunately appear to
> be
> 256 possible product identifiers (i.e., 0x00->0xFF):
> 
> 
>   23 /* The product ID is defined as starting with 0x12nn, as per the
>   24  * "Choosing an Apple Device USB Configuration" section in
>   25  * release R9 (2012) of the "MFi Accessory Hardware
> Specification"
>   26  *
>   27  * To distinguish an Apple device, a USB host can check the
> device
>   28  * descriptor of attached USB devices for the following fields:
>   29  * - Vendor ID: 0x05AC
>   30  * - Product ID: 0x12nn
>   31  *
>   32  * Those checks will be done in .match() and .probe().
>   33  */
> ...
> 166 static int mfi_fc_probe(struct usb_device *udev)
> 167 {
> ...
> 171
> 172         idProduct = le16_to_cpu(udev->descriptor.idProduct);
> 173         /* See comment above mfi_fc_id_table[] */
> 174         if (idProduct < 0x1200 || idProduct > 0x12ff) {
> 175                 return -ENODEV;
> 176         }
> 
> A quick look at drivers/usb/core/driver.c::usb_match_device indicates
> that it is not yet possible to specify ranges in ID tables of USB
> drivers, so I think that we would need to replace the ID table with
> a match function. Interestingly enough, the comment block quoted
> above mentions the use of a match function as well.

I have no idea why there isn't a match function. Patch v1 had a huge
table:
https://marc.info/?l=linux-usb&m=157062863431186&w=2
and v2 already didn't had that comment, but no .match function:
https://marc.info/?l=linux-usb&m=157114990905421&w=2

I'll prepare a patch that adds a match function. I'll let you (Vefa)
look at which of your patches need backporting though, as I'm really
quite a bit lost in the different patch sets and branches :/