thunderbolt: Make iommu_dma_protection more accurate
Subsystems (4)
Robin Murphy
reviewerv2 cover thunderbolt: Make iommu_dma_protection more accurate
OK, here's chapter 3 in the story of "but I really just want to remove iommu_present()", which is the second go at this approach on the Thunderbolt end, but now improving the IOMMU end as well since the subtlety of why that still matters has been clarified. Previous thread here:
https://lore.kernel.org/linux-iommu/[email protected]/T/
Thanks,
Robin.
Robin Murphy (2):
iommu: Add capability for pre-boot DMA protection
thunderbolt: Make iommu_dma_protection more accurate
drivers/iommu/intel/iommu.c | 2 ++ drivers/thunderbolt/domain.c | 12 +++-------- drivers/thunderbolt/nhi.c | 41 ++++++++++++++++++++++++++++++++++++ include/linux/iommu.h | 7 ++++++ include/linux/thunderbolt.h | 2 ++ 5 files changed, 55 insertions(+), 9 deletions(-)
Signature · 1 line
2.28.0.dirty
Robin Murphy
reviewerPATCH v2 1/2 iommu: Add capability for pre-boot DMA protection
VT-d's dmar_platform_optin() actually represents a combination of properties fairly well standardised by Microsoft as "Pre-boot DMA Protection" and "Kernel DMA Protection"[1]. As such, we can provide interested consumers with an abstracted capability rather than driver-specific interfaces that won't scale. We name it for the former aspect since that's what external callers are most likely to be interested in; the latter is for the IOMMU layer to handle itself.
Also use this as an opportunity to draw a line in the sand and add a new interface so as not to introduce any more callers of iommu_capable() which I also want to get rid of. For now it's a quick'n'dirty wrapper function, but will evolve to subsume the internal interface in future.
---
v2: New patch
drivers/iommu/intel/iommu.c | 2 ++ include/linux/iommu.h | 7 +++++++ 2 files changed, 9 insertions(+)
drivers/iommu/intel/iommu.c +2 -0
@@ -4817,6 +4817,8 @@ static bool intel_iommu_capable(enum iommu_cap cap)return domain_update_iommu_snooping (NULL );if (cap ==IOMMU_CAP_INTR_REMAP )return irq_remapping_enabled ==1 ; +if (cap ==IOMMU_CAP_PRE_BOOT_PROTECTION ) +return dmar_platform_optin ();return false; }
include/linux/iommu.h +7 -0
@@ -107,6 +107,8 @@ enum iommu_cap {transactions */ IOMMU_CAP_INTR_REMAP ,/* IOMMU supports interrupt isolation */ IOMMU_CAP_NOEXEC ,/* IOMMU_NOEXEC flag */ + IOMMU_CAP_PRE_BOOT_PROTECTION, /* Firmware says it used the IOMMU for + DMA protection and we should too */ };/* These are the possible reserved region types */
@@ -1042,6 +1044,11 @@ static inline size_t iommu_map_sgtable(struct iommu_domain *domain,return iommu_map_sg (domain, iova, sgt->sgl, sgt->orig_nents, prot); } +static inline bool dev_iommu_capable (struct device *dev,enum iommu_cap cap) +{ +return device_iommu_mapped (dev) &&iommu_capable (dev->bus, cap); +} +#ifdef CONFIG_IOMMU_DEBUGFS extern struct dentry *iommu_debugfs_dir;void iommu_debugfs_setup (void );
Signature · 6 lines
2.28.0.dirty _______________________________________________ iommu mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 1/2] iommu: Add capability for pre-boot DMA protection
Quoted 13 lines · Robin Murphy
VT-d's
dmar_platform_optin()actually represents a combination of properties fairly well standardised by Microsoft as "Pre-boot DMA Protection" and "Kernel DMA Protection"[1]. As such, we can provide interested consumers with an abstracted capability rather than driver-specific interfaces that won't scale. We name it for the former aspect since that's what external callers are most likely to be interested in; the latter is for the IOMMU layer to handle itself.Also use this as an opportunity to draw a line in the sand and add a new interface so as not to introduce any more callers of
iommu_capable()which I also want to get rid of. For now it's a quick'n'dirty wrapper function, but will evolve to subsume the internal interface in future.
Quoted 4 lines
Suggested-by Christoph Hellwig <[email protected]>Signed-off-by Robin Murphy <[email protected]>
I can't really think of a way in which I suggested this, but it does looks like a good interface:
Robin Murphy
reviewerRe: [PATCH v2 1/2] iommu: Add capability for pre-boot DMA protection
Quoted 14 lines · Christoph Hellwig
Quoted 13 lines · Robin Murphy
VT-d's
dmar_platform_optin()actually represents a combination of properties fairly well standardised by Microsoft as "Pre-boot DMA Protection" and "Kernel DMA Protection"[1]. As such, we can provide interested consumers with an abstracted capability rather than driver-specific interfaces that won't scale. We name it for the former aspect since that's what external callers are most likely to be interested in; the latter is for the IOMMU layer to handle itself.Also use this as an opportunity to draw a line in the sand and add a new interface so as not to introduce any more callers of
iommu_capable()which I also want to get rid of. For now it's a quick'n'dirty wrapper function, but will evolve to subsume the internal interface in future.
Quoted 7 lines
Quoted 4 lines
Suggested-by Christoph Hellwig <[email protected]>Signed-off-by Robin Murphy <[email protected]>I can't really think of a way in which I suggested this, but it does looks like a good interface:
Well, you were the first to say it should be abstracted[1], and since my initial thought that it could be hidden completely didn't pan out, I felt I should give you credit for being right all along :)
Quoted 1 line
Reviewed-by Christoph Hellwig <[email protected]>
Thanks!
Robin.
[1] https://lore.kernel.org/linux-iommu/YjDDUUeZ%[email protected]/
Robin Murphy
reviewerPATCH v2 2/2 thunderbolt: Make iommu_dma_protection more accurate
In reply to Robin MurphyBetween me trying to get rid of iommu_present() and Mario wanting to support the AMD equivalent of DMAR_PLATFORM_OPT_IN, scrutiny has shown that the iommu_dma_protection attribute is being far too optimistic. Even if an IOMMU might be present for some PCI segment in the system, that doesn't necessarily mean it provides translation for the device(s) we care about. Furthermore, all that DMAR_PLATFORM_OPT_IN really does is tell us that memory was protected before the kernel was loaded, and prevent the user from disabling the intel-iommu driver entirely. While that lets us assume kernel integrity, what matters for actual runtime DMA protection is whether we trust individual devices, based on the "external facing" property that we expect firmware to describe for Thunderbolt ports.
It's proven challenging to determine the appropriate ports accurately given the variety of possible topologies, so while still not getting a perfect answer, by putting enough faith in firmware we can at least get a good bit closer. If we can see that any device near a Thunderbolt NHI has all the requisites for Kernel DMA Protection, chances are that it *is* a relevant port, but moreover that implies that firmware is playing the game overall, so we'll use that to assume that all Thunderbolt ports should be correctly marked and thus will end up fully protected.
---
v2: Give up trying to look for specific devices, just look for evidence
that firmware cares at all.
drivers/thunderbolt/domain.c | 12 +++-------- drivers/thunderbolt/nhi.c | 41 ++++++++++++++++++++++++++++++++++++ include/linux/thunderbolt.h | 2 ++ 3 files changed, 46 insertions(+), 9 deletions(-)
drivers/thunderbolt/domain.c +3 -9
@@ -7,9 +7,7 @@*/ #include <linux/device.h> -#include <linux/dmar.h> #include <linux/idr.h> -#include <linux/iommu.h> #include <linux/module.h> #include <linux/pm_runtime.h> #include <linux/slab.h>
@@ -257,13 +255,9 @@ static ssize_t iommu_dma_protection_show(struct device *dev,struct device_attribute *attr, char *buf) { -/* -* Kernel DMA protection is a feature where Thunderbolt security is -* handled natively using IOMMU. It is enabled when IOMMU is -* enabled and ACPI DMAR table has DMAR_PLATFORM_OPT_IN set. -*/ -return sprintf (buf,"%d\n" , -iommu_present (&pci_bus_type) &&dmar_platform_optin ()); + struct tb *tb = container_of(dev, struct tb, dev); + +return sysfs_emit (buf,"%d\n" , tb->nhi->iommu_dma_protection); }static DEVICE_ATTR_RO (iommu_dma_protection);
drivers/thunderbolt/nhi.c +41 -0
59 lines not on this page · raw message
include/linux/thunderbolt.h +2 -0
@@ -465,6 +465,7 @@ static inline struct tb_xdomain *tb_service_parent(struct tb_service *svc)* @msix_ida: Used to allocate MSI-X vectors for rings * @going_away: The host controller device is about to disappear so when * this flag is set, avoid touching the hardware anymore. +* @iommu_dma_protection: An IOMMU will isolate external-facing ports. * @interrupt_work: Work scheduled to handle ring interrupt when no * MSI-X is used. * @hop_count: Number of rings (end point hops) supported by NHI.
@@ -479,6 +480,7 @@ struct tb_nhi {struct tb_ring **rx_rings;struct ida msix_ida;bool going_away; +bool iommu_dma_protection;struct work_struct interrupt_work;u32 hop_count;unsigned long quirks;
Signature · 6 lines
2.28.0.dirty _______________________________________________ iommu mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/iommu
[Public]
Quoted 14 lines
Between me trying to get rid of
iommu_present()and Mario wanting to support the AMD equivalent ofDMAR_PLATFORM_OPT_IN, scrutiny has shown
that theiommu_dma_protectionattribute is being far too optimistic. Even if an IOMMU might be present for some PCI segment in the system, that doesn't necessarily mean it provides translation for the device(s) we care about. Furthermore, all thatDMAR_PLATFORM_OPT_INreally does is tell us that memory was protected before the kernel was loaded, and prevent the user from disabling the intel-iommu driver entirely. While that lets us assume kernel integrity, what matters for actual runtime DMA protection is whether we trust individual devices, based on the "external facing" property that we expect firmware to describe for Thunderbolt ports.
Quoted 9 lines
It's proven challenging to determine the appropriate ports accurately given the variety of possible topologies, so while still not getting a perfect answer, by putting enough faith in firmware we can at least get a good bit closer. If we can see that any device near a Thunderbolt NHI has all the requisites for Kernel DMA Protection, chances are that it *is* a relevant port, but moreover that implies that firmware is playing the game overall, so we'll use that to assume that all Thunderbolt ports should be correctly marked and thus will end up fully protected.
This approach looks generally good to me. I do worry a little bit about older systems that didn't set ExternalFacingPort in the FW but were previously setting iommu_dma_protection, but I think that those could be treated on a quirk basis to set PCI IDs for those root ports as external facing if/when they come up.
I'll send up a follow up patch that adds the AMD ACPI table check. If it looks good can roll it into your series for v3, or if this series goes as is for v2 it can come on its own.
Quoted 6 lines
CC Mario Limonciello <[email protected]>Signed-off-by Robin Murphy <[email protected]>---
v2: Give up trying to look for specific devices, just look for evidence
that firmware cares at all.
I still do think you could know exactly which devices to use if you're in SW CM mode, but I guess the consensus is to not bifurcate the way this can be checked.
Quoted 131 lines
drivers/thunderbolt/domain.c | 12 +++-------- drivers/thunderbolt/nhi.c | 41 ++++++++++++++++++++++++++++++++++++ include/linux/thunderbolt.h | 2 ++ 3 files changed, 46 insertions(+), 9 deletions(-)drivers/thunderbolt/domain.c +3
-9@@ -7,9 +7,7 @@*/ #include <linux/device.h> -#include <linux/dmar.h> #include <linux/idr.h> -#include <linux/iommu.h> #include <linux/module.h> #include <linux/pm_runtime.h> #include <linux/slab.h> @@ -257,13 +255,9 @@ static ssize_t iommu_dma_protection_show(structdevice *dev, struct device_attribute *attr, char *buf) { -/* -* Kernel DMA protection is a feature where Thunderbolt security is -* handled natively using IOMMU. It is enabled when IOMMU is -* enabled and ACPI DMAR table has DMAR_PLATFORM_OPT_IN set. -*/ -return sprintf (buf,"%d\n" , -iommu_present (&pci_bus_type) && dmar_platform_optin()); + struct tb *tb = container_of(dev, struct tb, dev); + +return sysfs_emit (buf,"%d\n" , tb->nhi->iommu_dma_protection); }static
DEVICE_ATTR_RO(iommu_dma_protection);drivers/thunderbolt/nhi.c +40
-0@@ -14,6 +14,7 @@#include <linux/errno.h> #include <linux/pci.h> #include <linux/interrupt.h> +#include <linux/iommu.h> #include <linux/module.h> #include <linux/delay.h> #include <linux/property.h> @@ -1102,6 +1103,45 @@ static void nhi_check_quirks(struct tb_nhi *nhi) nhi->quirks |=QUIRK_AUTO_CLEAR_INT ; } +static int nhi_check_iommu_pdev (struct pci_dev *pdev,void *data) +{ +if (!pdev->untrusted || + !dev_iommu_capable (&pdev->dev,IOMMU_CAP_PRE_BOOT_PROTECTION )) +return 0 ; + *(bool *)data = true; +return 1 ;/* Stop walking */ +} + +static void nhi_check_iommu (struct tb_nhi *nhi) +{ +struct pci_bus *bus = nhi->pdev->bus; +bool port_ok = false; + +/* +* Ideally what we'd do here is grab every PCI device that +* represents a tunnelling adapter for this NHI and check their +* status directly, but unfortunately USB4 seems to make it +* obnoxiously difficult to reliably make any correlation. +* +* So for now we'll have to bodge it... Hoping that the system +* is at least sane enough that an adapter is in the same PCI +* segment as its NHI, if we can find *something* on that segment +* which meets the requirements for Kernel DMA Protection, we'll +* take that to imply that firmware is aware and has (hopefully) +* done the right thing in general. We need to know that the PCI +* layer has seen the ExternalFacingPort property and propagated +* it to the "untrusted" flag that the IOMMU layer will then +* enforce, but also that the IOMMU driver itself can be trusted +* not to have been subverted by a pre-boot DMA attack. +*/ +while (bus->parent) + bus = bus->parent; + +pci_walk_bus (bus, nhi_check_iommu_pdev, &port_ok); + + nhi->iommu_dma_protection = port_ok; +} +static int nhi_init_msi (struct tb_nhi *nhi) {drivers/thunderbolt/nhi.c +1
-0struct pci_dev *pdev = nhi->pdev;@@ -1219,6 +1259,7 @@ static int nhi_probe(struct pci_dev *pdev, const struct pci_device_id *id)return -ENOMEM ;nhi_check_quirks (nhi); +nhi_check_iommu (nhi); res =nhi_init_msi (nhi);drivers/thunderbolt/nhi.c +0
-0if (res) {include/linux/thunderbolt.h +1
-0@@ -465,6 +465,7 @@ static inline struct tb_xdomain *tb_service_parent(struct tb_service *svc) * @msix_ida: Used to allocate MSI-X vectors for rings * @going_away: The host controller device is about to disappear so when * this flag is set, avoid touching the hardware anymore. + * @iommu_dma_protection: An IOMMU will isolate external-facing ports. * @interrupt_work: Work scheduled to handle ring interrupt when no * MSI-X is used.* @hop_count: Number of rings (end point hops) supported by NHI.
include/linux/thunderbolt.h +1
-1@@ -479,6 +480,7 @@ struct tb_nhi {struct tb_ring **rx_rings;struct ida msix_ida;bool going_away; +bool iommu_dma_protection;struct work_struct interrupt_work;u32 hop_count;unsigned long quirks; --2.28.0.dirty
Hi Mario,
Quoted 26 lines · Mario
[Public]
Quoted 14 lines
Between me trying to get rid of
iommu_present()and Mario wanting to support the AMD equivalent ofDMAR_PLATFORM_OPT_IN, scrutiny has shown
that theiommu_dma_protectionattribute is being far too optimistic. Even if an IOMMU might be present for some PCI segment in the system, that doesn't necessarily mean it provides translation for the device(s) we care about. Furthermore, all thatDMAR_PLATFORM_OPT_INreally does is tell us that memory was protected before the kernel was loaded, and prevent the user from disabling the intel-iommu driver entirely. While that lets us assume kernel integrity, what matters for actual runtime DMA protection is whether we trust individual devices, based on the "external facing" property that we expect firmware to describe for Thunderbolt ports.Quoted 9 lines
It's proven challenging to determine the appropriate ports accurately given the variety of possible topologies, so while still not getting a perfect answer, by putting enough faith in firmware we can at least get a good bit closer. If we can see that any device near a Thunderbolt NHI has all the requisites for Kernel DMA Protection, chances are that it *is* a relevant port, but moreover that implies that firmware is playing the game overall, so we'll use that to assume that all Thunderbolt ports should be correctly marked and thus will end up fully protected.
Quoted 5 lines
This approach looks generally good to me. I do worry a little bit about older systems that didn't set ExternalFacingPort in the FW but were previously setting
iommu_dma_protection, but I think that those could be treated on a quirk basis to set PCI IDs for those root ports as external facing if/when they come up.
There are no such systems out there AFAICT.
Quoted 8 lines
I'll send up a follow up patch that adds the AMD ACPI table check. If it looks good can roll it into your series for v3, or if this series goes as is for v2 it can come on its own.
Quoted 4 lines
CC Mario Limonciello <[email protected]>Signed-off-by Robin Murphy <[email protected]>---
Quoted 6 lines
Quoted 2 lines
v2: Give up trying to look for specific devices, just look for evidence
that firmware cares at all.I still do think you could know exactly which devices to use if you're in SW CM mode, but I guess the consensus is to not bifurcate the way this can be checked.
Indeed.
The patch looks good to me now. I will give it a try on a couple of systems later today or tomorrow and let you guys know how it went. I don't expect any problems but let's see.
Thanks a lot Robin for working on this :)
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Robin Murphy
reviewerQuoted 29 lines · On 2022-03-21 10:58, [email protected] wrote:
Hi Mario,
Quoted 26 lines · Mario
[Public]
Quoted 23 lines
Between me trying to get rid of
iommu_present()and Mario wanting to support the AMD equivalent ofDMAR_PLATFORM_OPT_IN, scrutiny has shown
that theiommu_dma_protectionattribute is being far too optimistic. Even if an IOMMU might be present for some PCI segment in the system, that doesn't necessarily mean it provides translation for the device(s) we care about. Furthermore, all thatDMAR_PLATFORM_OPT_INreally does is tell us that memory was protected before the kernel was loaded, and prevent the user from disabling the intel-iommu driver entirely. While that lets us assume kernel integrity, what matters for actual runtime DMA protection is whether we trust individual devices, based on the "external facing" property that we expect firmware to describe for Thunderbolt ports.It's proven challenging to determine the appropriate ports accurately given the variety of possible topologies, so while still not getting a perfect answer, by putting enough faith in firmware we can at least get a good bit closer. If we can see that any device near a Thunderbolt NHI has all the requisites for Kernel DMA Protection, chances are that it *is* a relevant port, but moreover that implies that firmware is playing the game overall, so we'll use that to assume that all Thunderbolt ports should be correctly marked and thus will end up fully protected.
Quoted 7 lines
Quoted 5 lines
This approach looks generally good to me. I do worry a little bit about older systems that didn't set ExternalFacingPort in the FW but were previously setting
iommu_dma_protection, but I think that those could be treated on a quirk basis to set PCI IDs for those root ports as external facing if/when they come up.There are no such systems out there AFAICT.
And even if there are, as above they've never actually been fully protected and still won't be, so it's arguably a good thing for them to stop thinking so.
Quoted 15 lines
Quoted 8 lines
I'll send up a follow up patch that adds the AMD ACPI table check. If it looks good can roll it into your series for v3, or if this series goes as is for v2 it can come on its own.
Quoted 4 lines
CC Mario Limonciello <[email protected]>Signed-off-by Robin Murphy <[email protected]>---
Quoted 6 lines
Quoted 2 lines
v2: Give up trying to look for specific devices, just look for evidence
that firmware cares at all.I still do think you could know exactly which devices to use if you're in SW CM mode, but I guess the consensus is to not bifurcate the way this can be checked.
Quoted 7 lines
Indeed.
The patch looks good to me now. I will give it a try on a couple of systems later today or tomorrow and let you guys know how it went. I don't expect any problems but let's see.
Thanks a lot Robin for working on this :)
Heh, let's just hope the other half-dozen or so subsystems I need to touch for this IOMMU cleanup aren't all quite as involved as this turned out to be :)
Cheers,
Robin.
[Public]
Quoted 51 lines
-----Original Message-----
From: Robin Murphy <[email protected]>
Sent: Monday, March 21, 2022 06:12
To: [email protected]; Limonciello, Mario <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected] foundation.org; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v2 2/2] thunderbolt: Makeiommu_dma_protectionmore accurateQuoted 10 lines · On 2022-03-21 10:58, [email protected] wrote:
Hi Mario,
Quoted 7 lines · Mario
[Public]
Quoted 5 lines
Between me trying to get rid of
iommu_present()and Mario wanting to support the AMD equivalent ofDMAR_PLATFORM_OPT_IN, scrutiny has shown
that theiommu_dma_protectionattribute is being far too optimistic. Even if an IOMMU might be present for some PCI segment in thesystem,
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
that doesn't necessarily mean it provides translation for the device(s) we care about. Furthermore, all that
DMAR_PLATFORM_OPT_INreallydoes
Quoted 17 lines
Quoted 17 lines
Quoted 16 lines
is tell us that memory was protected before the kernel was loaded, and prevent the user from disabling the intel-iommu driver entirely. While that lets us assume kernel integrity, what matters for actual runtime DMA protection is whether we trust individual devices, based on the "external facing" property that we expect firmware to describe for Thunderbolt ports.
It's proven challenging to determine the appropriate ports accurately given the variety of possible topologies, so while still not getting a perfect answer, by putting enough faith in firmware we can at least get a good bit closer. If we can see that any device near a Thunderbolt NHI has all the requisites for Kernel DMA Protection, chances are that it *is* a relevant port, but moreover that implies that firmware is playing the game overall, so we'll use that to assume that all Thunderbolt ports should be correctly marked and thus will end up fully protected.
Quoted 1 line
Quoted 1 line
This approach looks generally good to me. I do worry a little bit about
older
Quoted 1 line
Quoted 1 line
systems that didn't set ExternalFacingPort in the FW but were previously
setting
Quoted 1 line
Quoted 1 line
iommu_dma_protection, but I think that those could be treated on aquirk
Quoted 1 line
Quoted 1 line
basis to set PCI IDs for those root ports as external facing if/when they
Quoted 8 lines
come
Quoted 3 lines
Quoted 1 line
up.
There are no such systems out there AFAICT.
And even if there are, as above they've never actually been fully protected and still won't be, so it's arguably a good thing for them to stop thinking so.
I was meaning that if this situation comes up that we could tag the PCI IDs for those root ports as ExternalFacing in drivers/pci/quirks.c so that the protection "is" enacted for them even if it was missing from the firmware.
Quoted 22 lines
Quoted 22 lines
Quoted 9 lines
I'll send up a follow up patch that adds the AMD ACPI table check. If it looks good can roll it into your series for v3, or if this series goes as is for v2 it can come on its own.
Quoted 4 lines
CC Mario Limonciello <[email protected]>Signed-off-by Robin Murphy <[email protected]>---
Quoted 6 lines
Quoted 2 lines
v2: Give up trying to look for specific devices, just look for evidence
that firmware cares at all.I still do think you could know exactly which devices to use if you're in SW CM mode, but I guess the consensus is to not bifurcate the way this can be checked.
Indeed.
The patch looks good to me now. I will give it a try on a couple of systems later today or tomorrow and let you guys know how it went. I don't expect any problems but let's see.
Quoted 5 lines
Quoted 1 line
Thanks a lot Robin for working on this :)
Heh, let's just hope the other half-dozen or so subsystems I need to touch for this IOMMU cleanup aren't all quite as involved as this turned out to be :)
Thanks a lot for this effort!
Quoted 3 lines
Cheers,
Robin.
Quoted 22 lines · Robin Murphy
Between me trying to get rid of
iommu_present()and Mario wanting to support the AMD equivalent ofDMAR_PLATFORM_OPT_IN, scrutiny has shown that theiommu_dma_protectionattribute is being far too optimistic. Even if an IOMMU might be present for some PCI segment in the system, that doesn't necessarily mean it provides translation for the device(s) we care about. Furthermore, all thatDMAR_PLATFORM_OPT_INreally does is tell us that memory was protected before the kernel was loaded, and prevent the user from disabling the intel-iommu driver entirely. While that lets us assume kernel integrity, what matters for actual runtime DMA protection is whether we trust individual devices, based on the "external facing" property that we expect firmware to describe for Thunderbolt ports.It's proven challenging to determine the appropriate ports accurately given the variety of possible topologies, so while still not getting a perfect answer, by putting enough faith in firmware we can at least get a good bit closer. If we can see that any device near a Thunderbolt NHI has all the requisites for Kernel DMA Protection, chances are that it *is* a relevant port, but moreover that implies that firmware is playing the game overall, so we'll use that to assume that all Thunderbolt ports should be correctly marked and thus will end up fully protected.
Quoted 2 lines
CC Mario Limonciello <[email protected]>Signed-off-by Robin Murphy <[email protected]>
Looks sensible to me:
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Hi Robin,
I tried this now on two Intel systems. One with integrated Thunderbolt and one with discrete. There was a small issue, see below but once fixed it worked as expected :)
Quoted 78 lines · Robin Murphy
Between me trying to get rid of
iommu_present()and Mario wanting to support the AMD equivalent ofDMAR_PLATFORM_OPT_IN, scrutiny has shown that theiommu_dma_protectionattribute is being far too optimistic. Even if an IOMMU might be present for some PCI segment in the system, that doesn't necessarily mean it provides translation for the device(s) we care about. Furthermore, all thatDMAR_PLATFORM_OPT_INreally does is tell us that memory was protected before the kernel was loaded, and prevent the user from disabling the intel-iommu driver entirely. While that lets us assume kernel integrity, what matters for actual runtime DMA protection is whether we trust individual devices, based on the "external facing" property that we expect firmware to describe for Thunderbolt ports.It's proven challenging to determine the appropriate ports accurately given the variety of possible topologies, so while still not getting a perfect answer, by putting enough faith in firmware we can at least get a good bit closer. If we can see that any device near a Thunderbolt NHI has all the requisites for Kernel DMA Protection, chances are that it *is* a relevant port, but moreover that implies that firmware is playing the game overall, so we'll use that to assume that all Thunderbolt ports should be correctly marked and thus will end up fully protected.
CC Mario Limonciello <[email protected]>Signed-off-by Robin Murphy <[email protected]>---
v2: Give up trying to look for specific devices, just look for evidence
that firmware cares at all.drivers/thunderbolt/domain.c | 12 +++-------- drivers/thunderbolt/nhi.c | 41 ++++++++++++++++++++++++++++++++++++ include/linux/thunderbolt.h | 2 ++ 3 files changed, 46 insertions(+), 9 deletions(-)drivers/thunderbolt/domain.c +3
-9@@ -7,9 +7,7 @@*/ #include <linux/device.h> -#include <linux/dmar.h> #include <linux/idr.h> -#include <linux/iommu.h> #include <linux/module.h> #include <linux/pm_runtime.h> #include <linux/slab.h> @@ -257,13 +255,9 @@ static ssize_t iommu_dma_protection_show(struct device *dev,struct device_attribute *attr, char *buf) { -/* -* Kernel DMA protection is a feature where Thunderbolt security is -* handled natively using IOMMU. It is enabled when IOMMU is -* enabled and ACPI DMAR table has DMAR_PLATFORM_OPT_IN set. -*/ -return sprintf (buf,"%d\n" , -iommu_present (&pci_bus_type) &&dmar_platform_optin ()); + struct tb *tb = container_of(dev, struct tb, dev); + +return sysfs_emit (buf,"%d\n" , tb->nhi->iommu_dma_protection); }static DEVICE_ATTR_RO (iommu_dma_protection);drivers/thunderbolt/nhi.c +1
-0@@ -14,6 +14,7 @@#include <linux/errno.h> #include <linux/pci.h> #include <linux/interrupt.h> +#include <linux/iommu.h> #include <linux/module.h> #include <linux/delay.h> #include <linux/property.h>
drivers/thunderbolt/nhi.c static void nhi_check_quirks(struct tb_nhi *nhi) · line 1109
Quoted 7 lines
drivers/thunderbolt/nhi.c +4
-0@@ -1102,6 +1103,45 @@ static void nhi_check_quirks(struct tb_nhi *nhi) nhi->quirks |=QUIRK_AUTO_CLEAR_INT ; } +static int nhi_check_iommu_pdev (struct pci_dev *pdev,void *data) +{ +if (!pdev->untrusted || + !dev_iommu_capable (&pdev->dev,IOMMU_CAP_PRE_BOOT_PROTECTION ))
This one needs to take the pdev->external_facing into account too because most of the time there are no existing tunnels when the driver is loaded so we only see the PCIe root/downstream port. I think this is enough actually:
if (!pdev->external_facing || !dev_iommu_capable(&pdev->dev, IOMMU_CAP_PRE_BOOT_PROTECTION))
Quoted 23 lines
drivers/thunderbolt/nhi.c +23
-0+return 0 ; + *(bool *)data = true; +return 1 ;/* Stop walking */ +} + +static void nhi_check_iommu (struct tb_nhi *nhi) +{ +struct pci_bus *bus = nhi->pdev->bus; +bool port_ok = false; + +/* +* Ideally what we'd do here is grab every PCI device that +* represents a tunnelling adapter for this NHI and check their +* status directly, but unfortunately USB4 seems to make it +* obnoxiously difficult to reliably make any correlation. +* +* So for now we'll have to bodge it... Hoping that the system +* is at least sane enough that an adapter is in the same PCI +* segment as its NHI, if we can find *something* on that segment +* which meets the requirements for Kernel DMA Protection, we'll +* take that to imply that firmware is aware and has (hopefully) +* done the right thing in general. We need to know that the PCI +* layer has seen the ExternalFacingPort property and propagated
drivers/thunderbolt/nhi.c static void nhi_check_quirks(struct tb_nhi *nhi) · line 1142
Quoted 10 lines
drivers/thunderbolt/nhi.c +10
-0+* it to the "untrusted" flag that the IOMMU layer will then +* enforce, but also that the IOMMU driver itself can be trusted +* not to have been subverted by a pre-boot DMA attack. +*/ +while (bus->parent) + bus = bus->parent; + +pci_walk_bus (bus, nhi_check_iommu_pdev, &port_ok); + + nhi->iommu_dma_protection = port_ok;
I would put here a log debug, something like this:
dev_dbg(&nhi->pdev->dev, "IOMMU DMA protection is %sabled\n",
port_ok ? "en" : "dis");
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Robin Murphy
reviewerQuoted 98 lines · Mika Westerberg
Hi Robin,
I tried this now on two Intel systems. One with integrated Thunderbolt and one with discrete. There was a small issue, see below but once fixed it worked as expected :)
Quoted 78 lines · Robin Murphy
Between me trying to get rid of
iommu_present()and Mario wanting to support the AMD equivalent ofDMAR_PLATFORM_OPT_IN, scrutiny has shown that theiommu_dma_protectionattribute is being far too optimistic. Even if an IOMMU might be present for some PCI segment in the system, that doesn't necessarily mean it provides translation for the device(s) we care about. Furthermore, all thatDMAR_PLATFORM_OPT_INreally does is tell us that memory was protected before the kernel was loaded, and prevent the user from disabling the intel-iommu driver entirely. While that lets us assume kernel integrity, what matters for actual runtime DMA protection is whether we trust individual devices, based on the "external facing" property that we expect firmware to describe for Thunderbolt ports.It's proven challenging to determine the appropriate ports accurately given the variety of possible topologies, so while still not getting a perfect answer, by putting enough faith in firmware we can at least get a good bit closer. If we can see that any device near a Thunderbolt NHI has all the requisites for Kernel DMA Protection, chances are that it *is* a relevant port, but moreover that implies that firmware is playing the game overall, so we'll use that to assume that all Thunderbolt ports should be correctly marked and thus will end up fully protected.
CC Mario Limonciello <[email protected]>Signed-off-by Robin Murphy <[email protected]>---
v2: Give up trying to look for specific devices, just look for evidence
that firmware cares at all.drivers/thunderbolt/domain.c | 12 +++-------- drivers/thunderbolt/nhi.c | 41 ++++++++++++++++++++++++++++++++++++ include/linux/thunderbolt.h | 2 ++ 3 files changed, 46 insertions(+), 9 deletions(-)drivers/thunderbolt/domain.c +3
-9@@ -7,9 +7,7 @@*/ #include <linux/device.h> -#include <linux/dmar.h> #include <linux/idr.h> -#include <linux/iommu.h> #include <linux/module.h> #include <linux/pm_runtime.h> #include <linux/slab.h> @@ -257,13 +255,9 @@ static ssize_t iommu_dma_protection_show(struct device *dev,struct device_attribute *attr, char *buf) { -/* -* Kernel DMA protection is a feature where Thunderbolt security is -* handled natively using IOMMU. It is enabled when IOMMU is -* enabled and ACPI DMAR table has DMAR_PLATFORM_OPT_IN set. -*/ -return sprintf (buf,"%d\n" , -iommu_present (&pci_bus_type) &&dmar_platform_optin ()); + struct tb *tb = container_of(dev, struct tb, dev); + +return sysfs_emit (buf,"%d\n" , tb->nhi->iommu_dma_protection); }static DEVICE_ATTR_RO (iommu_dma_protection);drivers/thunderbolt/nhi.c +1
-0@@ -14,6 +14,7 @@#include <linux/errno.h> #include <linux/pci.h> #include <linux/interrupt.h> +#include <linux/iommu.h> #include <linux/module.h> #include <linux/delay.h> #include <linux/property.h> drivers/thunderbolt/nhi.c static void nhi_check_quirks(struct tb_nhi *nhi) · line 1109
Quoted 7 lines
drivers/thunderbolt/nhi.c +4
-0@@ -1102,6 +1103,45 @@ static void nhi_check_quirks(struct tb_nhi *nhi) nhi->quirks |=QUIRK_AUTO_CLEAR_INT ; } +static int nhi_check_iommu_pdev (struct pci_dev *pdev,void *data) +{ +if (!pdev->untrusted || + !dev_iommu_capable (&pdev->dev,IOMMU_CAP_PRE_BOOT_PROTECTION ))This one needs to take the
pdev->external_facinginto account too because most of the time there are no existing tunnels when the driver is loaded so we only see the PCIe root/downstream port. I think this is enough actually:
Quoted 2 lines
if (!pdev->external_facing || !dev_iommu_capable(&pdev->dev, IOMMU_CAP_PRE_BOOT_PROTECTION))
Ah yes, my bad, for some reason I got the misapprehension into my head that untrusted was propagated to the port as well, not just the devices behind it. I'll fix this and tweak the comment below to match.
Quoted 36 lines
Quoted 23 lines
drivers/thunderbolt/nhi.c +23
-0+return 0 ; + *(bool *)data = true; +return 1 ;/* Stop walking */ +} + +static void nhi_check_iommu (struct tb_nhi *nhi) +{ +struct pci_bus *bus = nhi->pdev->bus; +bool port_ok = false; + +/* +* Ideally what we'd do here is grab every PCI device that +* represents a tunnelling adapter for this NHI and check their +* status directly, but unfortunately USB4 seems to make it +* obnoxiously difficult to reliably make any correlation. +* +* So for now we'll have to bodge it... Hoping that the system +* is at least sane enough that an adapter is in the same PCI +* segment as its NHI, if we can find *something* on that segment +* which meets the requirements for Kernel DMA Protection, we'll +* take that to imply that firmware is aware and has (hopefully) +* done the right thing in general. We need to know that the PCI +* layer has seen the ExternalFacingPort property and propagated drivers/thunderbolt/nhi.c static void nhi_check_quirks(struct tb_nhi *nhi) · line 1142
Quoted 10 lines
drivers/thunderbolt/nhi.c +10
-0+* it to the "untrusted" flag that the IOMMU layer will then +* enforce, but also that the IOMMU driver itself can be trusted +* not to have been subverted by a pre-boot DMA attack. +*/ +while (bus->parent) + bus = bus->parent; + +pci_walk_bus (bus, nhi_check_iommu_pdev, &port_ok); + + nhi->iommu_dma_protection = port_ok;I would put here a log debug, something like this:
Quoted 2 lines
dev_dbg(&nhi->pdev->dev, "IOMMU DMA protection is %sabled\n",
port_ok ? "en" : "dis");
Ack. I'll wait and send a v3 once the merge window's over, and can roll Mario's AMD IOMMU patch into that too.
Thanks,
Robin.