Date:   Thu, 23 Jul 2020 10:17:35 -0600
From:   Alex Williamson <alex.williamson@...hat.com>
To:     Alan Stern <stern@...land.harvard.edu>
Cc:     "Weitao Wang\(BJ-RD\)" <WeitaoWang@...oxin.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        WeitaoWang-oc <WeitaoWang-oc@...oxin.com>,
        "mathias.nyman@...ux.intel.com" <mathias.nyman@...ux.intel.com>,
        "ulf.hansson@...aro.org" <ulf.hansson@...aro.org>,
        "vkoul@...nel.org" <vkoul@...nel.org>,
        "hslester96@...il.com" <hslester96@...il.com>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Carsten_Schmid@...tor.com" <Carsten_Schmid@...tor.com>,
        "efremov@...ux.com" <efremov@...ux.com>,
        "Tony W. Wang\(XA-RD\)" <TonyWWang@...oxin.com>,
        "Cobe Chen\(BJ-RD\)" <CobeChen@...oxin.com>,
        "Tim Guo\(BJ-RD\)" <TimGuo@...oxin.com>,
        "wwt8723@....com" <wwt8723@....com>
Subject: Re: [PATCH] USB:Fix kernel NULL pointer when unbind UHCI form
 vfio-pci

On Thu, 23 Jul 2020 11:38:21 -0400
Alan Stern <stern@...land.harvard.edu> wrote:

> 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@...oxin.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)?

The alternate driver in question here is vfio-pci, which allows IOMMU
protected userspace driver access to a device.  A primary use case of
vfio is to assign PCI devices to a VM, where QEMU is the userspace
driver.  An IOMMU group is a set of one or more devices that are
considered to be DMA isolated from other groups of devices.  DMA
isolation includes, for instance, the potential for peer-to-peer
between devices which cannot be managed by the IOMMU.  DMA between PCIe
functions within a multifunction slot are generally considered to be
non-isolated from one another unless they implement PCIe Access Control
Services (ACS) to indicate isolation.  I've never seen a USB controller
implement ACS, nor should they due to the interactions between
functions, therefore all the USB functions within a slot will be
grouped together.

The vfio framework will not allow users to access groups for which some
of the devices within the group are bound to active host drivers,
therefore in this scenario where we have one USB function bound to a
host driver and the other bound to vfio-pci, the user would not be able
to access the device and the vfio-pci usage of the device is
essentially nothing more than a stub driver until driver binding of the
other devices within the group changes.  IOW, vfio-pci is not trying to
make use of the device with this split binding, the report here is only
a fault seen in the process of moving all of the functions to vfio-pci,
which would then make the devices accessible to the user.

> >  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.

The IOMMU grouping restriction does solve the hardware issue, so long
as one driver doesn't blindly assume the driver private data for
another device and modify it.  I do agree that your solution would
work, requiring all devices are unbound before any can be bound, but it
also seems difficult to manage.  The issue is largely unique to USB
AFAIK.  On the other hand, drivers coordinating with each other to
register their _private_ data as share-able within a set of drivers
seems like a much more direct and explicit interaction between the
drivers.  Thanks,

Alex

Powered by blists - more mailing lists