From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751437AbcEMGuc (ORCPT ); Fri, 13 May 2016 02:50:32 -0400 Received: from mga04.intel.com ([192.55.52.120]:34334 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750918AbcEMGua convert rfc822-to-8bit (ORCPT ); Fri, 13 May 2016 02:50:30 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,612,1455004800"; d="scan'208";a="102479258" From: "Tian, Kevin" To: Alex Williamson CC: Yongji Xie , David Laight , "kvm@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "linux-pci@vger.kernel.org" , "linuxppc-dev@lists.ozlabs.org" , "iommu@lists.linux-foundation.org" , "bhelgaas@google.com" , "aik@ozlabs.ru" , "benh@kernel.crashing.org" , "paulus@samba.org" , "mpe@ellerman.id.au" , "joro@8bytes.org" , "warrier@linux.vnet.ibm.com" , "zhong@linux.vnet.ibm.com" , "nikunj@linux.vnet.ibm.com" , "eric.auger@linaro.org" , "will.deacon@arm.com" , "gwshan@linux.vnet.ibm.com" , "alistair@popple.id.au" , "ruscur@russell.cc" Subject: RE: [PATCH 5/5] vfio-pci: Allow to mmap MSI-X table if interrupt remapping is supported Thread-Topic: [PATCH 5/5] vfio-pci: Allow to mmap MSI-X table if interrupt remapping is supported Thread-Index: AQHRoIMDw+bjSjW8wkusiJ7fWPCsmJ+muJEA//+EsACAAIcFkP//kPaAgAPMIVCAACQUuoAAByVg//+rVIABKrYjoAAEuVWAACQVMAD//46SAP//UvaQgAGwAoD//u4KoIAB1vwA//9ql4A= Date: Fri, 13 May 2016 06:50:25 +0000 Message-ID: References: <1461761010-5452-1-git-send-email-xyjxie@linux.vnet.ibm.com> <063D6719AE5E284EB5DD2968C1650D6D5F4B52B5@AcuExch.aculab.com> <4be013bc-e81b-84c5-06d3-e1b3f46b3227@linux.vnet.ibm.com> <20160505090513.56886c12@t450s.home> <20160511095331.18436241@t450s.home> <20160511202042.77593861@t450s.home> <20160512114735.7ec61bd3@t450s.home> <20160512233246.347b8b3c@t450s.home> In-Reply-To: <20160512233246.347b8b3c@t450s.home> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_IC x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMTY2ODFhNzktODYyOS00YWQ3LWI1ZjAtYWFmOTA2ODNhOWY0IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6Iko1aGFVanFMSVFRalRHTXZsTXhqVkJzd2JQMkpwRmkxR1ZGUHlybXBVSEE9In0= x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > 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