| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 17 Mar 2022 17:09:25 +0000
From: "Limonciello, Mario" <Mario.Limonciello@....com>
To: Robin Murphy <robin.murphy@....com>,
"andreas.noever@...il.com" <andreas.noever@...il.com>,
"michael.jamet@...el.com" <michael.jamet@...el.com>,
"mika.westerberg@...ux.intel.com" <mika.westerberg@...ux.intel.com>,
"YehezkelShB@...il.com" <YehezkelShB@...il.com>
CC: "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>
Subject: RE: [PATCH] thunderbolt: Make iommu_dma_protection more accurate
[Public]
> -----Original Message-----
> From: Robin Murphy <robin.murphy@....com>
> Sent: Thursday, March 17, 2022 11:17
> To: andreas.noever@...il.com; michael.jamet@...el.com;
> mika.westerberg@...ux.intel.com; YehezkelShB@...il.com
> Cc: linux-usb@...r.kernel.org; linux-kernel@...r.kernel.org;
> iommu@...ts.linux-foundation.org; linux-pci@...r.kernel.org; Limonciello,
> Mario <Mario.Limonciello@....com>
> Subject: [PATCH] thunderbolt: Make iommu_dma_protection more accurate
>
> Between 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. What
> actually matters is whether we trust individual devices, based on the
> "external facing" property that we expect firmware to describe for
> Thunderbolt ports.
>
> Avoid false positives by looking as close as possible to the same PCI
> topology that the IOMMU layer will consider once a Thunderbolt endpoint
> appears. Crucially, we can't assume that IOMMU translation being enabled
> for any reason is sufficient on its own; full (expensive) DMA protection
> will still only be imposed on untrusted devices.
>
> CC: Mario Limonciello <mario.limonciello@....com>
> Signed-off-by: Robin Murphy <robin.murphy@....com>
> ---
>
> This supersedes my previous attempt just trying to replace
> iommu_present() at [1], further to the original discussion at [2].
>
> [1]
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.
> kernel.org%2Flinux-
> iommu%2FBL1PR12MB515799C0BE396377DBBEF055E2119%40BL1PR12MB515
> 7.namprd12.prod.outlook.com%2FT%2F&data=04%7C01%7Cmario.limo
> nciello%40amd.com%7C14f5afbba9624b4d0ef508da08319b2a%7C3dd8961fe4
> 884e608e11a82d994e183d%7C0%7C0%7C637831306409535091%7CUnknown
> %7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1ha
> WwiLCJXVCI6Mn0%3D%7C3000&sdata=9xJ9bNT3pR3YhqOOqiJtGv94ln2
> IJSvrXllbPZjTI6M%3D&reserved=0
> [2]
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.
> kernel.org%2Flinux-iommu%2F202203160844.lKviWR1Q-
> lkp%40intel.com%2FT%2F&data=04%7C01%7Cmario.limonciello%40amd
> .com%7C14f5afbba9624b4d0ef508da08319b2a%7C3dd8961fe4884e608e11a8
> 2d994e183d%7C0%7C0%7C637831306409535091%7CUnknown%7CTWFpbGZs
> b3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn
> 0%3D%7C3000&sdata=wSbpjpPQk8ulX8ifOTt%2BNMdO5svwQceQthyca
> txzScI%3D&reserved=0
>
> drivers/thunderbolt/domain.c | 12 +++---------
> drivers/thunderbolt/nhi.c | 35
> +++++++++++++++++++++++++++++++++++
> include/linux/thunderbolt.h | 2 ++
> 3 files changed, 40 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/thunderbolt/domain.c b/drivers/thunderbolt/domain.c
> index 7018d959f775..d5c825e84ac8 100644
> --- a/drivers/thunderbolt/domain.c
> +++ b/drivers/thunderbolt/domain.c
> @@ -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 sprintf(buf, "%d\n", tb->nhi->iommu_dma_protection);
> }
> static DEVICE_ATTR_RO(iommu_dma_protection);
>
> diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
> index c73da0532be4..e12c2e266741 100644
> --- a/drivers/thunderbolt/nhi.c
> +++ b/drivers/thunderbolt/nhi.c
> @@ -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,39 @@ static void nhi_check_quirks(struct tb_nhi *nhi)
> nhi->quirks |= QUIRK_AUTO_CLEAR_INT;
> }
>
> +static void nhi_check_iommu(struct tb_nhi *nhi)
> +{
> + struct pci_dev *pdev;
> + bool port_ok = false;
> +
> + /*
> + * Check for sibling devices that look like they should be our
> + * tunnelled ports. We can reasonably assume that if an IOMMU is
> + * managing the bridge it will manage any future devices beyond it
> + * too. If firmware has described a port as external-facing as
> + * expected then we can trust the IOMMU layer to enforce isolation;
> + * otherwise even if translation is enabled for existing devices it
> + * may potentially be overridden for a future tunnelled endpoint.
> + */
> + for_each_pci_bridge(pdev, nhi->pdev->bus) {
> + if (!pci_is_pcie(pdev) ||
> + !(pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT ||
> + pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM))
> + continue;
> +
Unfortunately I don't think this logic holds for the topology I see.
Here is the NHI on a system I have here:
$ lspci -vvv -s 64:00.5
64:00.5 USB controller: Advanced Micro Devices, Inc. [AMD] Device 162e (prog-if 40)
Subsystem: Advanced Micro Devices, Inc. [AMD] Device 162e
Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
Latency: 0, Cache Line Size: 32 bytes
Interrupt: pin A routed to IRQ 42
Region 0: Memory at b0500000 (64-bit, non-prefetchable) [size=512K]
Capabilities: <access denied>
Kernel driver in use: thunderbolt
Kernel modules: thunderbolt
The links it makes (from those _DSD) are:
$ ls /sys/bus/pci/drivers/thunderbolt/0000\:64\:00.5/ | grep consumer
consumer:pci:0000:00:03.1
consumer:pci:0000:64:00.3
$ lspci -s 64:00.3
64:00.3 USB controller: Advanced Micro Devices, Inc. [AMD] Device 15d6
$ lspci -s 00:03.1
00:03.1 PCI bridge: Advanced Micro Devices, Inc. [AMD] Device 14cd
Looking at the topology the PCIE root port for tunneling (00:03.1) isn't actually on the same bridge.
$ lspci -t
-[0000:00]-+-00.0
+-00.2
+-01.0
+-01.2-[01]----00.0
+-02.0
+-02.4-[02]----00.0
+-03.0
+-03.1-[03-32]--
+-04.0
+-04.1-[33-62]--
+-08.0
+-08.1-[63]--+-00.0
| +-00.1
| +-00.2
| +-00.3
| +-00.4
| +-00.5
| +-00.6
| \-00.7
+-08.3-[64]--+-00.0
| +-00.3
| +-00.4
| +-00.5
| \-00.6
+-14.0
+-14.3
+-18.0
+-18.1
+-18.2
+-18.3
+-18.4
+-18.5
+-18.6
\-18.7
How about in this function to have two cases:
* the one that looks at links
* and if no links then the logic you have in place?
> + if (!device_iommu_mapped(&pdev->dev))
> + return;
> +
> + if (!pdev->untrusted) {
> + dev_info(&nhi->pdev->dev,
> + "Assuming unreliable Kernel DMA
> protection\n");
> + return;
> + }
> + port_ok = true;
> + }
> + nhi->iommu_dma_protection = port_ok;
> +}
> +
> static int nhi_init_msi(struct tb_nhi *nhi)
> {
> struct pci_dev *pdev = nhi->pdev;
> @@ -1219,6 +1253,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);
> if (res) {
> diff --git a/include/linux/thunderbolt.h b/include/linux/thunderbolt.h
> index 124e13cb1469..7a8ad984e651 100644
> --- a/include/linux/thunderbolt.h
> +++ b/include/linux/thunderbolt.h
> @@ -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;
> --
> 2.28.0.dirty
Powered by blists - more mailing lists