Re: [PATCH] x86/msi: Validate the guest-identified PCI devices in pci_prepare_msix()

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Wed, Jan 22, 2014 at 12:08:42PM +0000, Jan Beulich wrote:
> >>> On 22.01.14 at 11:28, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> > On 22/01/14 09:49, Jan Beulich wrote:
> >>>>> On 22.01.14 at 05:31, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> >>> See attached (and relevant part inlined).
> >>> ...
> >>> (XEN) [2014-01-22 12:27:07] Xen call trace:
> >>> (XEN) [2014-01-22 12:27:07]    [<ffff82d0801683a2>] 
> > msix_capability_init+0x1dc/0x603
> >>> (XEN) [2014-01-22 12:27:07]    [<ffff82d080168987>] pci_enable_msi+0x1be/0x4d7
> >>> (XEN) [2014-01-22 12:27:07]    [<ffff82d08016c65c>] map_domain_pirq+0x222/0x5ad
> >>> (XEN) [2014-01-22 12:27:07]    [<ffff82d08017f104>] physdev_map_pirq+0x507/0x5d1
> >>> (XEN) [2014-01-22 12:27:07]    [<ffff82d08017f814>] do_physdev_op+0x646/0x119e
> >>> (XEN) [2014-01-22 12:27:07]    [<ffff82d08022231b>] syscall_enter+0xeb/0x145
> >>> (XEN) [2014-01-22 12:27:07] 
> >>> (XEN) [2014-01-22 12:27:07] Pagetable walk from 0000000000000004:
> >> Considering the similarity, this is surely another incarnation of
> >> the same issue. Which gets me to ask first of all - is the device
> >> being acted upon an MSI-X capable one? If not, why is the call
> >> being made? If so (and Xen thinks differently) that's what
> >> needs fixing.
> >>
> >> On that basis I'm also going to ignore your patch for the first
> >> problem, Andrew: It's either incomplete or unnecessary or
> >> fixing the wrong thing.
> > 
> > I am going to go with incomplete - it is certainly not unnecessary.  The
> > PCI device parameters to pci_prepare_msix() are completely guest
> > controlled; There is no validation of the SBDF at all.
> 
> "Fixing the wrong thing" presumably, after taking a closer look at
> Konrad's second crash: The device in question really appears to
> be MSI-X capable, yet alloc_pdev() didn't recognize it as such. I
> wonder whether the capability gets displayed/hidden dynamically
> based on some other enabling the driver may be doing on the
> device. In which case we'd need to allocate the structure on
> demand.

The device in question (02:00.1) is an SR-IOV 82576:

02:00.0 Ethernet controller: Intel Corporation 82576 Gigabit Network Connection (rev 01)
02:00.1 Ethernet controller: Intel Corporation 82576 Gigabit Network Connection (rev 01)

-bash-4.1# lspci -s 02:00.1 -v | more
02:00.1 Ethernet controller: Intel Corporation 82576 Gigabit Network Connection (rev 01)
        Subsystem: Intel Corporation Gigabit ET Dual Port Server Adapter
        Flags: fast devsel, IRQ 18
        Memory at f1400000 (32-bit, non-prefetchable) [disabled] [size=128K]
        Memory at f0800000 (32-bit, non-prefetchable) [disabled] [size=4M]
        I/O ports at d000 [disabled] [size=32]
        Memory at f1440000 (32-bit, non-prefetchable) [disabled] [size=16K]
        Expansion ROM at f0400000 [disabled] [size=4M]
        Capabilities: [40] Power Management version 3
        Capabilities: [50] MSI: Enable- Count=1/1 Maskable+ 64bit+
        Capabilities: [70] MSI-X: Enable- Count=10 Masked-
        Capabilities: [a0] Express Endpoint, MSI 00
        Capabilities: [100] Advanced Error Reporting
        Capabilities: [140] Device Serial Number 00-1b-21-ff-ff-45-d9-ac
        Capabilities: [150] Alternative Routing-ID Interpretation (ARI)
        Capabilities: [160] Single Root I/O Virtualization (SR-IOV)
        Kernel driver in use: pciback
        Kernel modules: igb