From patchwork Fri May 13 16:42:36 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alex Williamson X-Patchwork-Id: 9092521 X-Patchwork-Delegate: bhelgaas@google.com Return-Path: X-Original-To: patchwork-linux-pci@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id C6C019F372 for ; Fri, 13 May 2016 16:43:08 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 66F3320263 for ; Fri, 13 May 2016 16:43:06 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 3F07D2025A for ; Fri, 13 May 2016 16:43:05 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752256AbcEMQmq (ORCPT ); Fri, 13 May 2016 12:42:46 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46109 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751739AbcEMQmp (ORCPT ); Fri, 13 May 2016 12:42:45 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 6E43D486B0; Fri, 13 May 2016 16:42:42 +0000 (UTC) Received: from t450s.home (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u4DGgbsY032637; Fri, 13 May 2016 12:42:37 -0400 Date: Fri, 13 May 2016 10:42:36 -0600 From: Alex Williamson To: "Tian, Kevin" 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 Message-ID: <20160513104236.438a800a@t450s.home> In-Reply-To: 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> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Fri, 13 May 2016 16:42:44 +0000 (UTC) Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org X-Spam-Status: No, score=-6.4 required=5.0 tests=BAYES_00,FSL_HELO_HOME, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Fri, 13 May 2016 06:50:25 +0000 "Tian, Kevin" 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: 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 --- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html 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; }