Date:	Fri, 13 May 2016 06:50:25 +0000
From:	"Tian, Kevin" <kevin.tian@...el.com>
To:	Alex Williamson <alex.williamson@...hat.com>
CC:	Yongji Xie <xyjxie@...ux.vnet.ibm.com>,
	David Laight <David.Laight@...LAB.COM>,
	"kvm@...r.kernel.org" <kvm@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	"linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
	"iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
	"bhelgaas@...gle.com" <bhelgaas@...gle.com>,
	"aik@...abs.ru" <aik@...abs.ru>,
	"benh@...nel.crashing.org" <benh@...nel.crashing.org>,
	"paulus@...ba.org" <paulus@...ba.org>,
	"mpe@...erman.id.au" <mpe@...erman.id.au>,
	"joro@...tes.org" <joro@...tes.org>,
	"warrier@...ux.vnet.ibm.com" <warrier@...ux.vnet.ibm.com>,
	"zhong@...ux.vnet.ibm.com" <zhong@...ux.vnet.ibm.com>,
	"nikunj@...ux.vnet.ibm.com" <nikunj@...ux.vnet.ibm.com>,
	"eric.auger@...aro.org" <eric.auger@...aro.org>,
	"will.deacon@....com" <will.deacon@....com>,
	"gwshan@...ux.vnet.ibm.com" <gwshan@...ux.vnet.ibm.com>,
	"alistair@...ple.id.au" <alistair@...ple.id.au>,
	"ruscur@...sell.cc" <ruscur@...sell.cc>
Subject: RE: [PATCH 5/5] vfio-pci: Allow to mmap MSI-X table if interrupt
 remapping is supported

> From: Alex Williamson [mailto:alex.williamson@...hat.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.

> MSI-X vector table when untrusted userspace drivers are involved, we
> must always assume that a device can generate DMA writes that are
> indistinguishable from actual interrupt requests and if the platform
> does not provide interrupt isolation we should require the user to
> opt-in to an unsafe mode.
> 
> Simply denying direct writes to the vector table or preventing mapping
> of the vector table into the user address space does not provide any
> tangible form of protection.  Many devices make use of window registers
> that allow backdoors to arbitrary device registers.  Some drivers even
> use this as the primary means for configuring MSI-X, which makes them
> incompatible with device assignment without device specific quirks to
> enable virtualization of these paths.
> 
> If you have an objection to this patch, please show me how preventing
> direct CPU access to the MSI-X vector table provides any kind of
> security guarantee of the contents of the vector table and also prove
> to me that a device cannot spoof a DMA write that is indistinguishable
> from one associated with an actual interrupt, regardless of the
> contents of the MSI-X vector table.  Thanks,
> 

I'm not object to the whole patch series. As replied above, my point
is just that current condition of allowing mmap MSI-X in this patch is not 
accurate, but my argument on security manner is not correct. Thanks
for your elaboration to make it clear.

Thanks
Kevin

Powered by blists - more mailing lists