Re: [PATCH] USB:Fix kernel NULL pointer when unbind UHCI form vfio-pci

[Alan Stern <stern@rowland.harvard.edu>]

On Wed, Jul 22, 2020 at 10:18:17PM -0600, Alex Williamson wrote:
> On Thu, 23 Jul 2020 02:59:55 +0000
> "Weitao Wang(BJ-RD)" <WeitaoWang@zhaoxin.com> wrote:
> 
> > On , Jul 22, 2020 at 02:44:14PM +0200, Alan wrote:
> > > On Wed, Jul 22, 2020 at 02:44:14PM +0200, Greg KH wrote:  
> > > > On Wed, Jul 22, 2020 at 07:57:48PM +0800, WeitaoWangoc wrote:  
> > > > > This bug is found in Zhaoxin platform, but it's a commom code bug.
> > > > > Fail sequence:
> > > > > step1: Unbind UHCI controller from native driver;
> > > > > step2: Bind UHCI controller to vfio-pci, which will put UHCI controller in one  
> > > vfio  
> > > > >        group's device list and set UHCI's dev->driver_data to struct  
> > > vfio-pci(for UHCI)  
> > > >
> > > > Hah, that works?  How do you do that properly?  What code does that?  
> > >
> > > Yeah, that can't possibly work.  The USB core expects that any host
> > > controller device (or at least, any PCI host controller device) has its
> > > driver_data set to point to a struct usb_hcd.  It doesn't expect a host
> > > controller to be bound to anything other than a host controller driver.
> > >
> > > Things could easily go very wrong here.  For example, suppose at this
> > > point the ehci-hcd driver just happens to bind to the EHCI controller.
> > > When this happens, the EHCI controller hardware takes over all the USB
> > > connections that were routed to the UHCI controller.  How will vfio-pci
> > > deal with that?  Pretty ungracefully, I imagine.
> 
> The issue I believe we're seeing here is not with vfio-pci trying to do
> anything with the device, the IOMMU grouping would prevent a user from
> opening any device within the group while other devices within the
> group are bound to host drivers.

You've lost me.  (A) What is IOMMU grouping?  (B) How does it prevent 
users from opening devices?  (C) What do users have to do with the 
problem anyway (USB host controllers and drivers have to do things on 
their own even without user intervention)?

>  So it should be fine if the EHCI
> device takes over the other ports, but it's not ok that ehci-hcd
> assumes the driver private data for any other UHCI/OHCI/EHCI device in
> the same slot is something that it's free to modify.  It really seems
> there should be some sort of companion device registration/opt-in
> rather than modifying unvalidated data.

Until now that hasn't been necessary, since nobody wanted to bind a 
different driver to these devices.

> > > The only way to make this work at all is to unbind both uhci-hcd and
> > > ehci-hcd first.  Then after both are finished you can safely bind
> > > vfio-pci to the EHCI controller and the UHCI controllers (in that
> > > order).
> > >  
> > I'm agree with you, unbind both uhci-hcd and ehci-hcd first then bind to
> > vfio-pci is a more reasonable sequence. Our experiments prove that this
> > sequence is indeed good as expected.
> > However, I did not find a formal document to prescribe this order.
> > Unfortunately, some application software such as virt-manager/qemu assign
> > UHCI/EHCI to guest OS has the same bind/unbind sequence as test “by hand”.
> > Do we need to consider compatibility with this application scenario?
> 
> Unbinding all functions first, before binding any to vfio-pci should
> indeed work, thanks to the for_each_companion() function at least
> testing for null private data before going further.  I'd still argue
> though that these hcd drivers are overstepping their authority by
> walking the PCI bus and assuming any device in the same slot, matching
> a set of class codes, is making use of a driver with a known data
> structure that they're allowed to modify.

Until recently that has been a valid assumption.

>  Even if we claim that the
> user needs to know what they're doing when they change driver binding,
> that's a pretty subtle interaction with no validation.  Thanks,

It's worse than that.  We're not just dealing with a software 
interaction issue -- the _hardware_ for these devices also interacts.  
That's the real reason why the driver for the device on one slot has to 
be aware of the driver for the device on a different slot.

Adding a mechanism for software registration or validation won't fix the 
hardware issues.  Relying on a protocol that requires all the devices to 
be unbound before any of them are bound to a new class of drivers, on 
the other hand, will fix the problem.

Alan Stern