On Fri, 13 May 2016 06:50:25 +0000
"Tian, Kevin" [off-list ref] wrote:

quoted

From: Alex Williamson [mailto:alex.williamson@redhat.com]
Sent: Friday, May 13, 2016 1:33 PM  

quoted

quoted

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!

quoted

quoted

quoted

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.

quoted

quoted

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