From: "Tian, Kevin" <kevin.tian@intel.com>
To: Alex Williamson <alex.williamson@redhat.com>
Cc: Yongji Xie <xyjxie@linux.vnet.ibm.com>,
David Laight <David.Laight@ACULAB.COM>,
"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
"linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
"iommu@lists.linux-foundation.org"
<iommu@lists.linux-foundation.org>,
"bhelgaas@google.com" <bhelgaas@google.com>,
"aik@ozlabs.ru" <aik@ozlabs.ru>,
"benh@kernel.crashing.org" <benh@kernel.crashing.org>,
"paulus@samba.org" <paulus@samba.org>,
"mpe@ellerman.id.au" <mpe@ellerman.id.au>,
"joro@8bytes.org" <joro@8bytes.org>,
"warrier@linux.vnet.ibm.com" <warrier@linux.vnet.ibm.com>,
"zhong@linux.vnet.ibm.com" <zhong@linux.vnet.ibm.com>,
"nikunj@linux.vnet.ibm.com" <nikunj@linux.vnet.ibm.com>,
"eric.auger@linaro.org" <eric.auger@linaro.org>,
"will.deacon@arm.com" <will.deacon@arm.com>,
"gwshan@linux.vnet.ibm.com" <gwshan@linux.vnet.ibm.com>,
"alistair@popple.id.au" <alistair@popple.id.au>,
"ruscur@russell.cc" <ruscur@russell.cc>
Subject: RE: [PATCH 5/5] vfio-pci: Allow to mmap MSI-X table if interrupt remapping is supported
Date: Fri, 13 May 2016 06:50:25 +0000 [thread overview]
Message-ID: <AADFC41AFE54684AB9EE6CBC0274A5D15F854287@SHSMSX101.ccr.corp.intel.com> (raw)
In-Reply-To: <20160512233246.347b8b3c@t450s.home>
> 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.
> 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
next prev parent reply other threads:[~2016-05-13 6:50 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-04-27 12:43 [PATCH 0/5] vfio-pci: Add support for mmapping MSI-X table Yongji Xie
2016-04-27 12:43 ` [PATCH 1/5] PCI: Add a new PCI_BUS_FLAGS_MSI_REMAP flag Yongji Xie
2016-05-24 20:55 ` Bjorn Helgaas
2016-05-25 5:46 ` Yongji Xie
2016-04-27 12:43 ` [PATCH 2/5] iommu: Set PCI_BUS_FLAGS_MSI_REMAP if IOMMU have capability of IRQ remapping Yongji Xie
2016-05-24 21:11 ` Bjorn Helgaas
2016-05-25 5:54 ` Yongji Xie
[not found] ` <201605250554.u4P5sRqv014439@mx0a-001b2d01.pphosted.com>
2016-05-26 3:48 ` Bjorn Helgaas
2016-04-27 12:43 ` [PATCH 3/5] PCI: Set PCI_BUS_FLAGS_MSI_REMAP if MSI controller supports " Yongji Xie
2016-05-24 21:04 ` Bjorn Helgaas
2016-05-25 5:48 ` Yongji Xie
2016-04-27 12:43 ` [PATCH 4/5] pci-ioda: Set PCI_BUS_FLAGS_MSI_REMAP for IODA host bridge Yongji Xie
2016-05-06 6:34 ` Alexey Kardashevskiy
2016-04-27 12:43 ` [PATCH 5/5] vfio-pci: Allow to mmap MSI-X table if interrupt remapping is supported Yongji Xie
2016-05-03 5:34 ` Tian, Kevin
2016-05-03 6:08 ` Yongji Xie
2016-05-03 6:22 ` Tian, Kevin
2016-05-03 7:34 ` Yongji Xie
2016-05-05 9:36 ` Tian, Kevin
2016-05-05 9:54 ` David Laight
2016-05-05 11:42 ` Yongji Xie
2016-05-05 12:15 ` Tian, Kevin
2016-05-05 13:28 ` Yongji Xie
2016-05-05 15:05 ` Alex Williamson
2016-05-06 6:35 ` Alexey Kardashevskiy
2016-05-06 16:54 ` Alex Williamson
2016-05-11 6:29 ` Tian, Kevin
2016-05-11 15:53 ` Alex Williamson
2016-05-12 1:19 ` Tian, Kevin
2016-05-12 2:20 ` Alex Williamson
2016-05-12 4:53 ` Tian, Kevin
2016-05-12 17:47 ` Alex Williamson
2016-05-13 2:33 ` Tian, Kevin
2016-05-13 5:32 ` Alex Williamson
2016-05-13 6:50 ` Tian, Kevin [this message]
2016-05-13 16:42 ` Alex Williamson
2016-05-13 9:16 ` David Laight
2016-05-13 2:36 ` Tian, Kevin
2016-05-05 11:44 ` Yongji Xie
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=AADFC41AFE54684AB9EE6CBC0274A5D15F854287@SHSMSX101.ccr.corp.intel.com \
--to=kevin.tian@intel.com \
--cc=David.Laight@ACULAB.COM \
--cc=aik@ozlabs.ru \
--cc=alex.williamson@redhat.com \
--cc=alistair@popple.id.au \
--cc=benh@kernel.crashing.org \
--cc=bhelgaas@google.com \
--cc=eric.auger@linaro.org \
--cc=gwshan@linux.vnet.ibm.com \
--cc=iommu@lists.linux-foundation.org \
--cc=joro@8bytes.org \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=mpe@ellerman.id.au \
--cc=nikunj@linux.vnet.ibm.com \
--cc=paulus@samba.org \
--cc=ruscur@russell.cc \
--cc=warrier@linux.vnet.ibm.com \
--cc=will.deacon@arm.com \
--cc=xyjxie@linux.vnet.ibm.com \
--cc=zhong@linux.vnet.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).