From: Robin Murphy <robin.murphy@arm.com> To: Jason Gunthorpe <jgg@nvidia.com> Cc: Alex Williamson <alex.williamson@redhat.com>, Kevin Tian <kevin.tian@intel.com>, Chaitanya Kulkarni <kch@nvidia.com>, Ashok Raj <ashok.raj@intel.com>, kvm@vger.kernel.org, rafael@kernel.org, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Cornelia Huck <cohuck@redhat.com>, linux-kernel@vger.kernel.org, Christoph Hellwig <hch@infradead.org>, iommu@lists.linux-foundation.org, Jacob jun Pan <jacob.jun.pan@intel.com>, linux-pci@vger.kernel.org, Bjorn Helgaas <bhelgaas@google.com>, Will Deacon <will@kernel.org>, Diana Craciun <diana.craciun@oss.nxp.com> Subject: Re: [PATCH 03/11] PCI: pci_stub: Suppress kernel DMA ownership auto-claiming Date: Mon, 15 Nov 2021 17:54:42 +0000 [thread overview] Message-ID: <e9db18d3-dea3-187a-d58a-31a913d95211@arm.com> (raw) In-Reply-To: <20211115161756.GP2105516@nvidia.com> On 2021-11-15 16:17, Jason Gunthorpe wrote: > On Mon, Nov 15, 2021 at 03:14:49PM +0000, Robin Murphy wrote: > >>> If userspace has control of device A and can cause A to issue DMA to >>> arbitary DMA addresses then there are certain PCI topologies where A >>> can now issue peer to peer DMA and manipulate the MMMIO registers in >>> device B. >>> >>> A kernel driver on device B is thus subjected to concurrent >>> manipulation of the device registers from userspace. >>> >>> So, a 'safe' kernel driver is one that can tolerate this, and an >>> 'unsafe' driver is one where userspace can break kernel integrity. >> >> You mean in the case where the kernel driver is trying to use device B in a >> purely PIO mode, such that userspace might potentially be able to interfere >> with data being transferred in and out of the kernel? > > s/PIO/MMIO, but yes basically. And not just data trasnfer but > userspace can interfere with the device state as well. Sure, but unexpected changes in device state could happen for any number of reasons - uncorrected ECC error, surprise removal, etc. - so if that can affect "kernel integrity" I'm considering it an independent problem. >> Perhaps it's not so clear to put that under a notion of "DMA >> ownership", since device B's DMA is irrelevant and it's really much >> more equivalent to /dev/mem access or mmaping BARs to userspace >> while a driver is bound. > > It is DMA ownership because device A's DMA is what is relevant > here. device A's DMA compromises device B. So device A asserts it has > USER ownership for DMA. > > Any device in a group with USER ownership is incompatible with a > kernel driver. I can see the argument from that angle, but you can equally look at it another way and say that a device with kernel ownership is incompatible with a kernel driver, if userspace can call write() on "/sys/devices/B/resource0" such that device A's kernel driver DMAs all over it. Maybe that particular example lands firmly under "just don't do that", but I'd like to figure out where exactly we should draw the line between "DMA" and "ability to mess with a device". >>> The second issue is DMA - because there is only one iommu_domain >>> underlying many devices if we give that iommu_domain to userspace it >>> means the kernel DMA API on other devices no longer works. >> >> Actually, the DMA API itself via iommu-dma will "work" just fine in the >> sense that it will still successfully perform all its operations in the >> unattached default domain, it's just that if the driver then programs the >> device to access the returned DMA address, the device is likely to get a >> nasty surprise. > > A DMA API that returns an dma_ddr_t that does not result in data > transfer to the specified buffers is not working, in my book - it > breaks the API contract. > >>> So no kernel driver doing DMA can work at all, under any PCI topology, >>> if userspace owns the IO page table. >> >> This isn't really about userspace at all - it's true of any case where a >> kernel driver wants to attach a grouped device to its own unmanaged >> domain. > > This is true for the dma api issue in isolation. No, it's definitely a general IOMMU-API-level thing; you could just as well have two drivers both trying to attach to their own unmanaged domains without DMA API involvement. What I think it boils down to is that if multiple devices in a group are bound (or want to bind) to different drivers, we want to enforce some kind of consensus between those drivers over IOMMU API usage. > I think if we have a user someday it would make sense to add another > API DMA_OWNER_DRIVER_DOMAIN that captures how the dma API doesn't work > but DMA MMIO attacks are not possible. > >> The fact that the VFIO kernel driver uses its unmanaged domains to map user >> pages upon user requests is merely a VFIO detail, and VFIO happens to be the >> only common case where unmanaged domains and non-singleton groups intersect. >> I'd say that, logically, if you want to put policy on mutual driver/usage >> compatibility anywhere it should be in iommu_attach_group(). > > It would make sense for iommu_attach_group() to require that the > DMA_OWNERSHIP is USER or DRIVER_DOMAIN. > > That has a nice symmetry with iommu_attach_device() already requiring > that the group has a single device. For a driver to use these APIs it > must ensure security, one way or another. iommu_attach_device() is supposed to be deprecated and eventually going away; I wouldn't look at it too much. > That is a good idea, but requires understanding what tegra is > doing. Maybe tegra is that DMA_OWNER_DRIVER_DOMAIN user? > > I wouldn't want to see iommu_attach_group() change the DMA_OWNERSHIP, > I think ownership is cleaner as a dedicated API. Adding a file * and > probably the enum to iommu_attach_group() feels weird. Indeed I wasn't imagining it changing any ownership, just preventing a group from being attached to a non-default domain if it contains devices bound to different incompatible drivers. Basically just taking the existing check that VFIO tries to enforce and formalising it into the core API. It's not too far off what we already have around changing the default domain type, so there seems to be room for it to all fit together quite nicely. There would still need to be separate enforcement elsewhere to prevent new drivers binding *after* a group *has* been attached to an unmanaged domain, but again it can still be in those simplest terms. Tying it in to userspace and FDs just muddies the water far more than necessary. Robin. > We need the dedicated API for the dma_configure op, and keeping > ownership split from the current domain makes more sense with the > design in the iommfd RFC. > > Thanks, > Jason > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
WARNING: multiple messages have this Message-ID (diff)
From: Robin Murphy <robin.murphy@arm.com> To: Jason Gunthorpe <jgg@nvidia.com> Cc: Christoph Hellwig <hch@infradead.org>, Kevin Tian <kevin.tian@intel.com>, Chaitanya Kulkarni <kch@nvidia.com>, Ashok Raj <ashok.raj@intel.com>, kvm@vger.kernel.org, rafael@kernel.org, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Cornelia Huck <cohuck@redhat.com>, Will Deacon <will@kernel.org>, linux-kernel@vger.kernel.org, iommu@lists.linux-foundation.org, Alex Williamson <alex.williamson@redhat.com>, Jacob jun Pan <jacob.jun.pan@intel.com>, linux-pci@vger.kernel.org, Bjorn Helgaas <bhelgaas@google.com>, Diana Craciun <diana.craciun@oss.nxp.com> Subject: Re: [PATCH 03/11] PCI: pci_stub: Suppress kernel DMA ownership auto-claiming Date: Mon, 15 Nov 2021 17:54:42 +0000 [thread overview] Message-ID: <e9db18d3-dea3-187a-d58a-31a913d95211@arm.com> (raw) In-Reply-To: <20211115161756.GP2105516@nvidia.com> On 2021-11-15 16:17, Jason Gunthorpe wrote: > On Mon, Nov 15, 2021 at 03:14:49PM +0000, Robin Murphy wrote: > >>> If userspace has control of device A and can cause A to issue DMA to >>> arbitary DMA addresses then there are certain PCI topologies where A >>> can now issue peer to peer DMA and manipulate the MMMIO registers in >>> device B. >>> >>> A kernel driver on device B is thus subjected to concurrent >>> manipulation of the device registers from userspace. >>> >>> So, a 'safe' kernel driver is one that can tolerate this, and an >>> 'unsafe' driver is one where userspace can break kernel integrity. >> >> You mean in the case where the kernel driver is trying to use device B in a >> purely PIO mode, such that userspace might potentially be able to interfere >> with data being transferred in and out of the kernel? > > s/PIO/MMIO, but yes basically. And not just data trasnfer but > userspace can interfere with the device state as well. Sure, but unexpected changes in device state could happen for any number of reasons - uncorrected ECC error, surprise removal, etc. - so if that can affect "kernel integrity" I'm considering it an independent problem. >> Perhaps it's not so clear to put that under a notion of "DMA >> ownership", since device B's DMA is irrelevant and it's really much >> more equivalent to /dev/mem access or mmaping BARs to userspace >> while a driver is bound. > > It is DMA ownership because device A's DMA is what is relevant > here. device A's DMA compromises device B. So device A asserts it has > USER ownership for DMA. > > Any device in a group with USER ownership is incompatible with a > kernel driver. I can see the argument from that angle, but you can equally look at it another way and say that a device with kernel ownership is incompatible with a kernel driver, if userspace can call write() on "/sys/devices/B/resource0" such that device A's kernel driver DMAs all over it. Maybe that particular example lands firmly under "just don't do that", but I'd like to figure out where exactly we should draw the line between "DMA" and "ability to mess with a device". >>> The second issue is DMA - because there is only one iommu_domain >>> underlying many devices if we give that iommu_domain to userspace it >>> means the kernel DMA API on other devices no longer works. >> >> Actually, the DMA API itself via iommu-dma will "work" just fine in the >> sense that it will still successfully perform all its operations in the >> unattached default domain, it's just that if the driver then programs the >> device to access the returned DMA address, the device is likely to get a >> nasty surprise. > > A DMA API that returns an dma_ddr_t that does not result in data > transfer to the specified buffers is not working, in my book - it > breaks the API contract. > >>> So no kernel driver doing DMA can work at all, under any PCI topology, >>> if userspace owns the IO page table. >> >> This isn't really about userspace at all - it's true of any case where a >> kernel driver wants to attach a grouped device to its own unmanaged >> domain. > > This is true for the dma api issue in isolation. No, it's definitely a general IOMMU-API-level thing; you could just as well have two drivers both trying to attach to their own unmanaged domains without DMA API involvement. What I think it boils down to is that if multiple devices in a group are bound (or want to bind) to different drivers, we want to enforce some kind of consensus between those drivers over IOMMU API usage. > I think if we have a user someday it would make sense to add another > API DMA_OWNER_DRIVER_DOMAIN that captures how the dma API doesn't work > but DMA MMIO attacks are not possible. > >> The fact that the VFIO kernel driver uses its unmanaged domains to map user >> pages upon user requests is merely a VFIO detail, and VFIO happens to be the >> only common case where unmanaged domains and non-singleton groups intersect. >> I'd say that, logically, if you want to put policy on mutual driver/usage >> compatibility anywhere it should be in iommu_attach_group(). > > It would make sense for iommu_attach_group() to require that the > DMA_OWNERSHIP is USER or DRIVER_DOMAIN. > > That has a nice symmetry with iommu_attach_device() already requiring > that the group has a single device. For a driver to use these APIs it > must ensure security, one way or another. iommu_attach_device() is supposed to be deprecated and eventually going away; I wouldn't look at it too much. > That is a good idea, but requires understanding what tegra is > doing. Maybe tegra is that DMA_OWNER_DRIVER_DOMAIN user? > > I wouldn't want to see iommu_attach_group() change the DMA_OWNERSHIP, > I think ownership is cleaner as a dedicated API. Adding a file * and > probably the enum to iommu_attach_group() feels weird. Indeed I wasn't imagining it changing any ownership, just preventing a group from being attached to a non-default domain if it contains devices bound to different incompatible drivers. Basically just taking the existing check that VFIO tries to enforce and formalising it into the core API. It's not too far off what we already have around changing the default domain type, so there seems to be room for it to all fit together quite nicely. There would still need to be separate enforcement elsewhere to prevent new drivers binding *after* a group *has* been attached to an unmanaged domain, but again it can still be in those simplest terms. Tying it in to userspace and FDs just muddies the water far more than necessary. Robin. > We need the dedicated API for the dma_configure op, and keeping > ownership split from the current domain makes more sense with the > design in the iommfd RFC. > > Thanks, > Jason >
next prev parent reply other threads:[~2021-11-15 17:54 UTC|newest] Thread overview: 120+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-11-15 2:05 [PATCH 00/11] Fix BUG_ON in vfio_iommu_group_notifier() Lu Baolu 2021-11-15 2:05 ` Lu Baolu 2021-11-15 2:05 ` [PATCH 01/11] iommu: Add device dma ownership set/release interfaces Lu Baolu 2021-11-15 2:05 ` Lu Baolu 2021-11-15 13:14 ` Christoph Hellwig 2021-11-15 13:14 ` Christoph Hellwig 2021-11-16 1:57 ` Lu Baolu 2021-11-16 1:57 ` Lu Baolu 2021-11-16 13:46 ` Jason Gunthorpe 2021-11-16 13:46 ` Jason Gunthorpe via iommu 2021-11-17 5:22 ` Lu Baolu 2021-11-17 5:22 ` Lu Baolu 2021-11-17 13:35 ` Jason Gunthorpe 2021-11-17 13:35 ` Jason Gunthorpe via iommu 2021-11-18 1:12 ` Lu Baolu 2021-11-18 1:12 ` Lu Baolu 2021-11-18 14:10 ` Jason Gunthorpe 2021-11-18 14:10 ` Jason Gunthorpe via iommu 2021-11-18 2:39 ` Tian, Kevin 2021-11-18 2:39 ` Tian, Kevin 2021-11-18 13:33 ` Jason Gunthorpe 2021-11-18 13:33 ` Jason Gunthorpe via iommu 2021-11-19 5:44 ` Tian, Kevin 2021-11-19 5:44 ` Tian, Kevin 2021-11-19 11:14 ` Lu Baolu 2021-11-19 11:14 ` Lu Baolu 2021-11-19 15:06 ` Jörg Rödel 2021-11-19 15:06 ` Jörg Rödel 2021-11-19 15:43 ` Jason Gunthorpe 2021-11-19 15:43 ` Jason Gunthorpe via iommu 2021-11-20 11:16 ` Lu Baolu 2021-11-20 11:16 ` Lu Baolu 2021-11-19 12:56 ` Jason Gunthorpe 2021-11-19 12:56 ` Jason Gunthorpe via iommu 2021-11-15 20:38 ` Bjorn Helgaas 2021-11-15 20:38 ` Bjorn Helgaas 2021-11-16 1:52 ` Lu Baolu 2021-11-16 1:52 ` Lu Baolu 2021-11-15 2:05 ` [PATCH 02/11] driver core: Set DMA ownership during driver bind/unbind Lu Baolu 2021-11-15 2:05 ` Lu Baolu 2021-11-15 6:59 ` Greg Kroah-Hartman 2021-11-15 6:59 ` Greg Kroah-Hartman 2021-11-15 13:20 ` Christoph Hellwig 2021-11-15 13:20 ` Christoph Hellwig 2021-11-15 13:38 ` Jason Gunthorpe via iommu 2021-11-15 13:38 ` Jason Gunthorpe 2021-11-15 13:19 ` Christoph Hellwig 2021-11-15 13:19 ` Christoph Hellwig 2021-11-15 13:24 ` Jason Gunthorpe 2021-11-15 13:24 ` Jason Gunthorpe via iommu 2021-11-15 15:37 ` Robin Murphy 2021-11-15 15:37 ` Robin Murphy 2021-11-15 15:56 ` Jason Gunthorpe 2021-11-15 15:56 ` Jason Gunthorpe via iommu 2021-11-15 18:15 ` Christoph Hellwig 2021-11-15 18:15 ` Christoph Hellwig 2021-11-15 18:35 ` Robin Murphy 2021-11-15 18:35 ` Robin Murphy 2021-11-15 19:39 ` Jason Gunthorpe via iommu 2021-11-15 19:39 ` Jason Gunthorpe 2021-11-15 2:05 ` [PATCH 03/11] PCI: pci_stub: Suppress kernel DMA ownership auto-claiming Lu Baolu 2021-11-15 2:05 ` Lu Baolu 2021-11-15 13:21 ` Christoph Hellwig 2021-11-15 13:21 ` Christoph Hellwig 2021-11-15 13:31 ` Jason Gunthorpe via iommu 2021-11-15 13:31 ` Jason Gunthorpe 2021-11-15 15:14 ` Robin Murphy 2021-11-15 15:14 ` Robin Murphy 2021-11-15 16:17 ` Jason Gunthorpe 2021-11-15 16:17 ` Jason Gunthorpe via iommu 2021-11-15 17:54 ` Robin Murphy [this message] 2021-11-15 17:54 ` Robin Murphy 2021-11-15 18:19 ` Christoph Hellwig 2021-11-15 18:19 ` Christoph Hellwig 2021-11-15 18:44 ` Robin Murphy 2021-11-15 18:44 ` Robin Murphy 2021-11-15 19:22 ` Jason Gunthorpe via iommu 2021-11-15 19:22 ` Jason Gunthorpe 2021-11-15 20:58 ` Robin Murphy 2021-11-15 20:58 ` Robin Murphy 2021-11-15 21:19 ` Jason Gunthorpe via iommu 2021-11-15 21:19 ` Jason Gunthorpe 2021-11-15 20:48 ` Bjorn Helgaas 2021-11-15 20:48 ` Bjorn Helgaas 2021-11-15 22:17 ` Bjorn Helgaas 2021-11-15 22:17 ` Bjorn Helgaas 2021-11-16 6:05 ` Lu Baolu 2021-11-16 6:05 ` Lu Baolu 2021-11-15 2:05 ` [PATCH 04/11] PCI: portdrv: " Lu Baolu 2021-11-15 2:05 ` Lu Baolu 2021-11-15 20:44 ` Bjorn Helgaas 2021-11-15 20:44 ` Bjorn Helgaas 2021-11-16 7:24 ` Lu Baolu 2021-11-16 7:24 ` Lu Baolu 2021-11-16 20:22 ` Bjorn Helgaas 2021-11-16 20:22 ` Bjorn Helgaas 2021-11-16 20:48 ` Jason Gunthorpe 2021-11-16 20:48 ` Jason Gunthorpe via iommu 2021-11-15 2:05 ` [PATCH 05/11] iommu: Add security context management for assigned devices Lu Baolu 2021-11-15 2:05 ` Lu Baolu 2021-11-15 13:22 ` Christoph Hellwig 2021-11-15 13:22 ` Christoph Hellwig 2021-11-16 7:25 ` Lu Baolu 2021-11-16 7:25 ` Lu Baolu 2021-11-15 2:05 ` [PATCH 06/11] iommu: Expose group variants of dma ownership interfaces Lu Baolu 2021-11-15 2:05 ` Lu Baolu 2021-11-15 13:27 ` Christoph Hellwig 2021-11-15 13:27 ` Christoph Hellwig 2021-11-16 9:42 ` Lu Baolu 2021-11-16 9:42 ` Lu Baolu 2021-11-15 2:05 ` [PATCH 07/11] vfio: Use DMA_OWNER_USER to declaim passthrough devices Lu Baolu 2021-11-15 2:05 ` Lu Baolu 2021-11-15 2:05 ` [PATCH 08/11] vfio: Remove use of vfio_group_viable() Lu Baolu 2021-11-15 2:05 ` Lu Baolu 2021-11-15 2:05 ` [PATCH 09/11] vfio: Delete the unbound_list Lu Baolu 2021-11-15 2:05 ` Lu Baolu 2021-11-15 2:05 ` [PATCH 10/11] vfio: Remove iommu group notifier Lu Baolu 2021-11-15 2:05 ` Lu Baolu 2021-11-15 2:05 ` [PATCH 11/11] iommu: Remove iommu group changes notifier Lu Baolu 2021-11-15 2:05 ` 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=e9db18d3-dea3-187a-d58a-31a913d95211@arm.com \ --to=robin.murphy@arm.com \ --cc=alex.williamson@redhat.com \ --cc=ashok.raj@intel.com \ --cc=bhelgaas@google.com \ --cc=cohuck@redhat.com \ --cc=diana.craciun@oss.nxp.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=kch@nvidia.com \ --cc=kevin.tian@intel.com \ --cc=kvm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pci@vger.kernel.org \ --cc=rafael@kernel.org \ --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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.