iommu.lists.linux-foundation.org archive mirror
 help / color / mirror / Atom feed
From: Bjorn Helgaas <helgaas@kernel.org>
To: Lu Baolu <baolu.lu@linux.intel.com>
Cc: Stuart Yoder <stuyoder@gmail.com>,
	rafael@kernel.org, David Airlie <airlied@linux.ie>,
	linux-pci@vger.kernel.org,
	Thierry Reding <thierry.reding@gmail.com>,
	Diana Craciun <diana.craciun@oss.nxp.com>,
	Dmitry Osipenko <digetx@gmail.com>, Will Deacon <will@kernel.org>,
	Ashok Raj <ashok.raj@intel.com>,
	Jonathan Hunter <jonathanh@nvidia.com>,
	Christoph Hellwig <hch@infradead.org>,
	Jason Gunthorpe <jgg@nvidia.com>,
	Kevin Tian <kevin.tian@intel.com>,
	Chaitanya Kulkarni <kch@nvidia.com>,
	Alex Williamson <alex.williamson@redhat.com>,
	kvm@vger.kernel.org, Bjorn Helgaas <bhelgaas@google.com>,
	Dan Williams <dan.j.williams@intel.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Cornelia Huck <cohuck@redhat.com>,
	linux-kernel@vger.kernel.org, Li Yang <leoyang.li@nxp.com>,
	iommu@lists.linux-foundation.org,
	Jacob jun Pan <jacob.jun.pan@intel.com>,
	Daniel Vetter <daniel@ffwll.ch>,
	Robin Murphy <robin.murphy@arm.com>
Subject: Re: [PATCH v4 04/13] PCI: portdrv: Suppress kernel DMA ownership auto-claiming
Date: Wed, 29 Dec 2021 15:16:26 -0600	[thread overview]
Message-ID: <20211229211626.GA1701512@bhelgaas> (raw)
In-Reply-To: <20211217063708.1740334-5-baolu.lu@linux.intel.com>

On Fri, Dec 17, 2021 at 02:36:59PM +0800, Lu Baolu wrote:
> IOMMU grouping on PCI necessitates that if we lack isolation on a bridge
> then all of the downstream devices will be part of the same IOMMU group
> as the bridge. The existing vfio framework allows the portdrv driver to
> be bound to the bridge while its downstream devices are assigned to user
> space. The pci_dma_configure() marks the iommu_group as containing only
> devices with kernel drivers that manage DMA. Avoid this default behavior
> for the portdrv driver in order for compatibility with the current vfio
> policy.

A word about the isolation would be useful.  I think you're referring
to some specific ACS controls, probably P2P Request Redirect?

I guess this is just a wording issue, but I think it's actually the
*lack* of some ACS controls that forces us to put several devices in
the same IOMMU group, isn't it?  It's not that we start with "IOMMU
grouping" and that necessitates something else.

Maybe something like this?

  If a switch lacks ACS P2P Request Redirect (and possibly other
  controls?), a device below the switch can bypass the IOMMU and DMA
  directly to other devices below the switch, so all the downstream
  devices must be in the same IOMMU group as the switch itself.

> The commit 5f096b14d421b ("vfio: Whitelist PCI bridges") extended above
> policy to all kernel drivers of bridge class. This is not always safe.
> For example, The shpchp_core driver relies on the PCI MMIO access for the
> controller functionality. With its downstream devices assigned to the
> userspace, the MMIO might be changed through user initiated P2P accesses
> without any notification. This might break the kernel driver integrity
> and lead to some unpredictable consequences.
> 
> For any bridge driver, in order to avoiding default kernel DMA ownership
> claiming, we should consider:
> 
>  1) Does the bridge driver use DMA? Calling pci_set_master() or
>     a dma_map_* API is a sure indicate the driver is doing DMA
> 
>  2) If the bridge driver uses MMIO, is it tolerant to hostile
>     userspace also touching the same MMIO registers via P2P DMA
>     attacks?
> 
> Conservatively if the driver maps an MMIO region at all, we can say that
> it fails the test.

I'm not sure what all this explanation is telling me.  It says
something done by 5f096b14d421 is not always safe, but this patch
doesn't fix any of those unsafe things.

If it doesn't explain why we need this patch or how this patch works,
I don't think we need it in the commit log.

Maybe this is an explanation for why you didn't set
.suppress_auto_claim_dma_owner for shpc_driver?

Minor typos above:
  s/in order to avoiding default/before avoiding default/
  s/relies on the PCI MMIO access/relies on PCI MMIO access/
  s/For example, The/For example, the/
  s/is a sure indicate the/is a sure indication the/

> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Suggested-by: Kevin Tian <kevin.tian@intel.com>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/pci/pcie/portdrv_pci.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> index 35eca6277a96..c48a8734f9c4 100644
> --- a/drivers/pci/pcie/portdrv_pci.c
> +++ b/drivers/pci/pcie/portdrv_pci.c
> @@ -202,7 +202,10 @@ static struct pci_driver pcie_portdriver = {
>  
>  	.err_handler	= &pcie_portdrv_err_handler,
>  
> -	.driver.pm	= PCIE_PORTDRV_PM_OPS,
> +	.driver		= {
> +		.pm = PCIE_PORTDRV_PM_OPS,
> +		.suppress_auto_claim_dma_owner = true,
> +	},
>  };
>  
>  static int __init dmi_pcie_pme_disable_msi(const struct dmi_system_id *d)
> -- 
> 2.25.1
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

  reply	other threads:[~2021-12-29 21:16 UTC|newest]

Thread overview: 47+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-17  6:36 [PATCH v4 00/13] Fix BUG_ON in vfio_iommu_group_notifier() Lu Baolu
2021-12-17  6:36 ` [PATCH v4 01/13] iommu: Add device dma ownership set/release interfaces Lu Baolu
2021-12-17  6:36 ` [PATCH v4 02/13] driver core: Set DMA ownership during driver bind/unbind Lu Baolu
2021-12-22 12:47   ` Greg Kroah-Hartman
2021-12-22 17:52     ` Jason Gunthorpe via iommu
2021-12-23  2:08     ` Lu Baolu
2021-12-23  3:02     ` Lu Baolu
2021-12-23  7:13       ` Greg Kroah-Hartman
2021-12-23  7:23         ` Lu Baolu
2021-12-31  0:36           ` Jason Gunthorpe via iommu
2021-12-17  6:36 ` [PATCH v4 03/13] PCI: pci_stub: Suppress kernel DMA ownership auto-claiming Lu Baolu
2021-12-29 20:42   ` Bjorn Helgaas
2021-12-30  5:34     ` Lu Baolu
2021-12-30 22:24       ` Bjorn Helgaas
2021-12-31  0:40         ` Jason Gunthorpe via iommu
2021-12-31  1:10           ` Lu Baolu
2021-12-31  1:58             ` Lu Baolu
2022-01-03 19:53             ` Jason Gunthorpe via iommu
2022-01-04  1:54               ` Lu Baolu
2021-12-31  1:06         ` Lu Baolu
2021-12-17  6:36 ` [PATCH v4 04/13] PCI: portdrv: " Lu Baolu
2021-12-29 21:16   ` Bjorn Helgaas [this message]
2021-12-30  5:49     ` Lu Baolu
2021-12-17  6:37 ` [PATCH v4 05/13] iommu: Add security context management for assigned devices Lu Baolu
2021-12-17  6:37 ` [PATCH v4 06/13] iommu: Expose group variants of dma ownership interfaces Lu Baolu
2021-12-17  6:37 ` [PATCH v4 07/13] iommu: Add iommu_at[de]tach_device_shared() for multi-device groups Lu Baolu
2021-12-21 16:50   ` Robin Murphy
2021-12-21 18:46     ` Jason Gunthorpe via iommu
2021-12-22  4:22       ` Lu Baolu
2021-12-22  4:25         ` Lu Baolu
2021-12-22 20:26       ` Robin Murphy
2021-12-23  0:57         ` Jason Gunthorpe via iommu
2021-12-23  5:53           ` Lu Baolu
2021-12-23 14:03             ` Jason Gunthorpe via iommu
2021-12-24  1:30               ` Lu Baolu
2021-12-24  2:50                 ` Jason Gunthorpe via iommu
2021-12-24  6:44                   ` Lu Baolu
2022-01-04  1:53                   ` Lu Baolu
2021-12-24  3:19         ` Lu Baolu
2021-12-24 14:24           ` Jason Gunthorpe via iommu
2021-12-17  6:37 ` [PATCH v4 08/13] vfio: Set DMA USER ownership for VFIO devices Lu Baolu
2021-12-17  6:37 ` [PATCH v4 09/13] vfio: Remove use of vfio_group_viable() Lu Baolu
2021-12-17  6:37 ` [PATCH v4 10/13] vfio: Delete the unbound_list Lu Baolu
2021-12-17  6:37 ` [PATCH v4 11/13] vfio: Remove iommu group notifier Lu Baolu
2021-12-17  6:37 ` [PATCH v4 12/13] iommu: Remove iommu group changes notifier Lu Baolu
2021-12-17  6:37 ` [PATCH v4 13/13] drm/tegra: Use the iommu dma_owner mechanism Lu Baolu
2022-01-04  5:23 ` [PATCH v4 00/13] Fix BUG_ON in vfio_iommu_group_notifier() Lu Baolu

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=20211229211626.GA1701512@bhelgaas \
    --to=helgaas@kernel.org \
    --cc=airlied@linux.ie \
    --cc=alex.williamson@redhat.com \
    --cc=ashok.raj@intel.com \
    --cc=baolu.lu@linux.intel.com \
    --cc=bhelgaas@google.com \
    --cc=cohuck@redhat.com \
    --cc=dan.j.williams@intel.com \
    --cc=daniel@ffwll.ch \
    --cc=diana.craciun@oss.nxp.com \
    --cc=digetx@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=hch@infradead.org \
    --cc=iommu@lists.linux-foundation.org \
    --cc=jacob.jun.pan@intel.com \
    --cc=jgg@nvidia.com \
    --cc=jonathanh@nvidia.com \
    --cc=kch@nvidia.com \
    --cc=kevin.tian@intel.com \
    --cc=kvm@vger.kernel.org \
    --cc=leoyang.li@nxp.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=rafael@kernel.org \
    --cc=robin.murphy@arm.com \
    --cc=stuyoder@gmail.com \
    --cc=thierry.reding@gmail.com \
    --cc=will@kernel.org \
    /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).