Re: [PATCH 5/5] vfio-pci: Allow to mmap MSI-X table if interrupt remapping is supported

[Alex Williamson <alex.williamson@redhat.com>]

On Fri, 13 May 2016 06:50:25 +0000
"Tian, Kevin" <kevin.tian@intel.com> wrote:

> > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > Sent: Friday, May 13, 2016 1:33 PM  
> > > >
> > > > As argued previously in this thread, there's nothing special about a
> > > > DMA write to memory versus a DMA write to a special address that
> > > > triggers an MSI vector.  If the device is DMA capable, which we assume
> > > > all are, it can be fooled into generating those DMA writes regardless
> > > > of whether we actively block access to the MSI-X vector table itself.  
> > >
> > > But with DMA remapping above can be blocked.  
> > 
> > How?  VT-d explicitly ignores DMA writes to 0xFEEx_xxxx, section 3.13:
> > 
> >   Write requests without PASID of DWORD length are treated as interrupt
> >   requests. Interrupt requests are not subjected to DMA remapping[...]
> >   Instead, remapping hardware can be enabled to subject such interrupt
> >   requests to interrupt remapping.  
> 
> Thanks for catching this!
> 
> >   
> > > > MSI-X vector table access w/o interrupt remapping is to avoid obvious
> > > > collisions if it were to be programmed directly, it doesn't actually
> > > > prevent an identical DMA transaction from being generated by other  
> > >
> > > Kernel can enable DMA remapping but disable IRQ remapping. In such
> > > case identical DMA transaction can be prevented.  
> > 
> > Not according to the VT-d spec as quoted above.  If so, how?  
> 
> So my argument on this is wrong. sorry.
> 
> >   
> > > Anyway my point is simple. Let's ignore how Linux kernel implements
> > > IRQ remapping on x86 (which may change time to time), and just
> > > focus on architectural possibility. Non-x86 platform may implement
> > > IRQ remapping completely separate from device side, then checking
> > > availability of IRQ remapping is enough to decide whether mmap
> > > MSI-X table is safe. x86 with VT-d can be configured to a mode
> > > requiring host control of both MSI-X entry and IRQ remapping hardware
> > > (without source id check). In such case it's insufficient to make
> > > decision simply based on IRQ remapping availability. We need a way
> > > to query from IRQ remapping module whether it's actually safe to
> > > mmap MSI-X.  
> > 
> > We're going in circles here.  This patch is attempting to remove
> > protection from the MSI-X vector table that is really nothing more than
> > security theater already.  That "protection" only actually prevents
> > casual misuse of the API which is really only a problem when the
> > platform offers no form of interrupt isolation, such as VT-d with DMA
> > remapping but not interrupt remapping.  Disabling source-id checking in
> > VT-d should be handled as the equivalent of disabling interrupt
> > remapping altogether as far as the IOMMU API is concerned.  That's
> > a trivial gap that should be fixed.  There is no such thing as a secure  
> 
> That is the main change I'm asking against original patch, which has:
> 
> +static void pci_check_msi_remapping(struct pci_dev *pdev,
> +					const struct iommu_ops *ops)
> +{
> +	struct pci_bus *bus = pdev->bus;
> +
> +	if (ops->capable(IOMMU_CAP_INTR_REMAP) &&
> +		!(bus->bus_flags & PCI_BUS_FLAGS_MSI_REMAP))
> +		bus->bus_flags |= PCI_BUS_FLAGS_MSI_REMAP;
> +}
> +
> 
> Above flag should be cleared when source-id checking is disabled on x86. 
> Yes, VFIO is part of OS but any assumption we made about other parts
> needs to be reflected accurately in the code.

I would say this is an independent bug which should be fixed simply as:

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index e1852e8..60d55c0 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -4948,7 +4948,7 @@ static bool intel_iommu_capable(enum iommu_cap cap)
        if (cap == IOMMU_CAP_CACHE_COHERENCY)
                return domain_update_iommu_snooping(NULL) == 1;
        if (cap == IOMMU_CAP_INTR_REMAP)
-               return irq_remapping_enabled == 1;
+               return irq_remapping_enabled == 1 && !disable_sourceid_checking;
 
        return false;
 }

I believe the intent of the IOMMU_CAP_INTR_REMAP flag is simply to
indicate interrupt isolation is provided through the IOMMU.  Nobody
cares about the interrupt remapping support beyond that.  If source-id
checking is disabled, the remainder of interrupt remapping is
irrelevant as far as this capability is concerned imho.  Thanks,

Alex