From: Robin Murphy <robin.murphy@arm.com> To: Mika Westerberg <mika.westerberg@linux.intel.com> Cc: "michael.jamet@intel.com" <michael.jamet@intel.com>, "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "andreas.noever@gmail.com" <andreas.noever@gmail.com>, "iommu@lists.linux-foundation.org" <iommu@lists.linux-foundation.org>, "Limonciello, Mario" <Mario.Limonciello@amd.com>, "YehezkelShB@gmail.com" <YehezkelShB@gmail.com>, "hch@lst.de" <hch@lst.de> Subject: Re: [PATCH] thunderbolt: Stop using iommu_present() Date: Thu, 17 Mar 2022 13:42:56 +0000 [thread overview] Message-ID: <23f232a1-f511-d2fe-b1f8-5fd32b3a1a8f@arm.com> (raw) In-Reply-To: <YjLsfhUmhjOiy6G8@lahna> On 2022-03-17 08:08, Mika Westerberg wrote: > Hi Robin, > > On Wed, Mar 16, 2022 at 07:17:57PM +0000, Robin Murphy wrote: >> The feeling I'm getting from all this is that if we've got as far as >> iommu_dma_protection_show() then it's really too late to meaningfully >> mitigate bad firmware. > > Note, these are requirements from Microsoft in order for the system to > use the "Kernel DMA protection". Because of this, likelyhood of "bad > firmware" should be quite low since these systems ship with Windows > installed so they should get at least some soft of validation that this > actually works. > >> We should be able to detect missing >> untrusted/external-facing properties as early as nhi_probe(), and if we >> could go into "continue at your own risk" mode right then *before* anything >> else happens, it all becomes a lot easier to reason about. > > I think what we want is that the DMAR opt-in bit is set in the ACPI > tables and that we know the full IOMMU translation is happening for the > devices behind "external facing ports". If that's not the case the > iommu_dma_protection_show() should return 0 meaning the userspace can > ask the user whether the connected device is allowed to use DMA (e.g > PCIe is tunneled or not). Ah, if it's safe to just say "no protection" in the case that we don't know for sure, that's even better. Clearly I hadn't quite grasped that aspect of the usage model, thanks for the nudge! > We do check for the DMAR bit in the Intel IOMMU code and we also do > check that there actually are PCIe ports marked external facing but we > could issue warning there if that's not the case. Similarly if the user > explicitly disabled the IOMMU translation. This can be done inside a new > IOMMU API that does something like the below pseudo-code: > > #if IOMMU_ENABLED > bool iommu_dma_protected(struct device *dev) > { > if (dmar_platform_optin() /* or the AMD equivalent */) { > if (!iommu_present(...)) /* whatever is needed to check that the full translation is enabled */ > dev_warn(dev, "IOMMU protection disabled!"); > /* > * Look for the external facing ports. Should be at > * least 1 or issue warning. > */ > ... > > return true; > } > > return false; > } > #else > static inline bool iommu_dma_protected(struct device *dev) > { > return false; > } > #endif > > Then we can make iommu_dma_protection_show() to call this function. The problem that I've been trying to nail down here is that dmar_platform_optin() really doesn't mean much for us - I don't know how Windows' IOMMU drivers work, but there's every chance it's not the same way as ours. The only material effect that dmar_platform_optin() has for us is to prevent the user from disabling the IOMMU driver altogether, and thus ensure that iommu_present() is true. Whether or not we can actually trust the IOMMU driver to provide reliable protection depends entirely on whether it knows the PCIe ports are external-facing. If not, we can only *definitely* know what the IOMMU driver will do for a given endpoint once that endpoint has appeared behind the port and iommu_probe_device() has decided what its default domain should be, and as far as I now understand, that's not an option for Thunderbolt since it can only happen *after* the tunnel has been authorised and created. Much as I'm tempted to de-scope back to my IOMMU API cleanup and run away from the rest of the issue, I think I can crib enough from the existing code to attempt a reasonable complete fix, so let me give that a go... Thanks, Robin.
WARNING: multiple messages have this Message-ID (diff)
From: Robin Murphy <robin.murphy@arm.com> To: Mika Westerberg <mika.westerberg@linux.intel.com> Cc: "michael.jamet@intel.com" <michael.jamet@intel.com>, "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "YehezkelShB@gmail.com" <YehezkelShB@gmail.com>, "iommu@lists.linux-foundation.org" <iommu@lists.linux-foundation.org>, "Limonciello, Mario" <Mario.Limonciello@amd.com>, "andreas.noever@gmail.com" <andreas.noever@gmail.com>, "hch@lst.de" <hch@lst.de> Subject: Re: [PATCH] thunderbolt: Stop using iommu_present() Date: Thu, 17 Mar 2022 13:42:56 +0000 [thread overview] Message-ID: <23f232a1-f511-d2fe-b1f8-5fd32b3a1a8f@arm.com> (raw) In-Reply-To: <YjLsfhUmhjOiy6G8@lahna> On 2022-03-17 08:08, Mika Westerberg wrote: > Hi Robin, > > On Wed, Mar 16, 2022 at 07:17:57PM +0000, Robin Murphy wrote: >> The feeling I'm getting from all this is that if we've got as far as >> iommu_dma_protection_show() then it's really too late to meaningfully >> mitigate bad firmware. > > Note, these are requirements from Microsoft in order for the system to > use the "Kernel DMA protection". Because of this, likelyhood of "bad > firmware" should be quite low since these systems ship with Windows > installed so they should get at least some soft of validation that this > actually works. > >> We should be able to detect missing >> untrusted/external-facing properties as early as nhi_probe(), and if we >> could go into "continue at your own risk" mode right then *before* anything >> else happens, it all becomes a lot easier to reason about. > > I think what we want is that the DMAR opt-in bit is set in the ACPI > tables and that we know the full IOMMU translation is happening for the > devices behind "external facing ports". If that's not the case the > iommu_dma_protection_show() should return 0 meaning the userspace can > ask the user whether the connected device is allowed to use DMA (e.g > PCIe is tunneled or not). Ah, if it's safe to just say "no protection" in the case that we don't know for sure, that's even better. Clearly I hadn't quite grasped that aspect of the usage model, thanks for the nudge! > We do check for the DMAR bit in the Intel IOMMU code and we also do > check that there actually are PCIe ports marked external facing but we > could issue warning there if that's not the case. Similarly if the user > explicitly disabled the IOMMU translation. This can be done inside a new > IOMMU API that does something like the below pseudo-code: > > #if IOMMU_ENABLED > bool iommu_dma_protected(struct device *dev) > { > if (dmar_platform_optin() /* or the AMD equivalent */) { > if (!iommu_present(...)) /* whatever is needed to check that the full translation is enabled */ > dev_warn(dev, "IOMMU protection disabled!"); > /* > * Look for the external facing ports. Should be at > * least 1 or issue warning. > */ > ... > > return true; > } > > return false; > } > #else > static inline bool iommu_dma_protected(struct device *dev) > { > return false; > } > #endif > > Then we can make iommu_dma_protection_show() to call this function. The problem that I've been trying to nail down here is that dmar_platform_optin() really doesn't mean much for us - I don't know how Windows' IOMMU drivers work, but there's every chance it's not the same way as ours. The only material effect that dmar_platform_optin() has for us is to prevent the user from disabling the IOMMU driver altogether, and thus ensure that iommu_present() is true. Whether or not we can actually trust the IOMMU driver to provide reliable protection depends entirely on whether it knows the PCIe ports are external-facing. If not, we can only *definitely* know what the IOMMU driver will do for a given endpoint once that endpoint has appeared behind the port and iommu_probe_device() has decided what its default domain should be, and as far as I now understand, that's not an option for Thunderbolt since it can only happen *after* the tunnel has been authorised and created. Much as I'm tempted to de-scope back to my IOMMU API cleanup and run away from the rest of the issue, I think I can crib enough from the existing code to attempt a reasonable complete fix, so let me give that a go... Thanks, Robin. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
next prev parent reply other threads:[~2022-03-17 13:43 UTC|newest] Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-03-16 11:25 [PATCH] thunderbolt: Stop using iommu_present() Robin Murphy 2022-03-16 11:25 ` Robin Murphy 2022-03-16 12:45 ` Mika Westerberg 2022-03-16 12:45 ` Mika Westerberg 2022-03-16 14:49 ` Robin Murphy 2022-03-16 14:49 ` Robin Murphy 2022-03-16 17:18 ` Mika Westerberg 2022-03-16 17:18 ` Mika Westerberg 2022-03-16 17:24 ` Limonciello, Mario 2022-03-16 17:24 ` Limonciello, Mario via iommu 2022-03-16 17:37 ` Mika Westerberg 2022-03-16 17:37 ` Mika Westerberg 2022-03-16 17:49 ` Robin Murphy 2022-03-16 17:49 ` Robin Murphy 2022-03-16 17:53 ` Limonciello, Mario 2022-03-16 17:53 ` Limonciello, Mario via iommu 2022-03-16 18:08 ` Limonciello, Mario 2022-03-16 18:08 ` Limonciello, Mario via iommu 2022-03-16 18:22 ` Robin Murphy 2022-03-16 18:22 ` Robin Murphy 2022-03-16 18:34 ` Limonciello, Mario 2022-03-16 18:34 ` Limonciello, Mario via iommu 2022-03-16 19:17 ` Robin Murphy 2022-03-16 19:17 ` Robin Murphy 2022-03-16 19:25 ` Limonciello, Mario 2022-03-16 19:25 ` Limonciello, Mario via iommu 2022-03-17 8:08 ` Mika Westerberg 2022-03-17 8:08 ` Mika Westerberg 2022-03-17 13:42 ` Robin Murphy [this message] 2022-03-17 13:42 ` Robin Murphy 2022-03-17 14:21 ` Mika Westerberg 2022-03-17 14:21 ` Mika Westerberg 2022-03-17 6:30 ` Mika Westerberg 2022-03-17 6:30 ` Mika Westerberg 2022-03-16 14:49 ` Limonciello, Mario 2022-03-16 14:49 ` Limonciello, Mario via iommu
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=23f232a1-f511-d2fe-b1f8-5fd32b3a1a8f@arm.com \ --to=robin.murphy@arm.com \ --cc=Mario.Limonciello@amd.com \ --cc=YehezkelShB@gmail.com \ --cc=andreas.noever@gmail.com \ --cc=hch@lst.de \ --cc=iommu@lists.linux-foundation.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-usb@vger.kernel.org \ --cc=michael.jamet@intel.com \ --cc=mika.westerberg@linux.intel.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: 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.