| 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
| ||
|
Message-ID: <65207fdf-c4ab-5165-dbda-8ab55b51adb7@arm.com>
Date: Fri, 18 Mar 2022 12:01:42 +0000
From: Robin Murphy <robin.murphy@....com>
To: "mika.westerberg@...ux.intel.com" <mika.westerberg@...ux.intel.com>,
"Limonciello, Mario" <Mario.Limonciello@....com>
Cc: "andreas.noever@...il.com" <andreas.noever@...il.com>,
"michael.jamet@...el.com" <michael.jamet@...el.com>,
"YehezkelShB@...il.com" <YehezkelShB@...il.com>,
"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
On 2022-03-18 11:38, mika.westerberg@...ux.intel.com wrote:
> Hi Mario,
>
> On Thu, Mar 17, 2022 at 08:36:13PM +0000, Limonciello, Mario wrote:
>> Here is a proposal on top of what you did for this.
>> The idea being check the ports right when the links are made if they exist
>> (all the new USB4 stuff) and then check all siblings on TBT3 stuff.
>>
>> diff --git a/drivers/thunderbolt/acpi.c b/drivers/thunderbolt/acpi.c
>> index 79b5abf9d042..89432456dbea 100644
>> --- a/drivers/thunderbolt/acpi.c
>> +++ b/drivers/thunderbolt/acpi.c
>> @@ -14,6 +14,7 @@
>> static acpi_status tb_acpi_add_link(acpi_handle handle, u32 level, void *data,
>> void **return_value)
>> {
>> + enum nhi_iommu_status iommu_status = IOMMU_UNKNOWN;
>> struct fwnode_reference_args args;
>> struct fwnode_handle *fwnode;
>> struct tb_nhi *nhi = data;
>> @@ -91,6 +92,8 @@ static acpi_status tb_acpi_add_link(acpi_handle handle, u32 level, void *data,
>> if (link) {
>> dev_dbg(&nhi->pdev->dev, "created link from %s\n",
>> dev_name(&pdev->dev));
>> + if (iommu_status != IOMMU_DISABLED)
>> + iommu_status = nhi_check_iommu_for_port(pdev);
>> } else {
>> dev_warn(&nhi->pdev->dev, "device link creation from %s failed\n",
>> dev_name(&pdev->dev));
>> @@ -101,6 +104,7 @@ static acpi_status tb_acpi_add_link(acpi_handle handle, u32 level, void *data,
>>
>> out_put:
>> fwnode_handle_put(args.fwnode);
>> + nhi->iommu_dma_protection = (iommu_status == IOMMU_ENABLED);
>> return AE_OK;
>> }
>>
>> diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
>> index e12c2e266741..b5eb0cab392f 100644
>> --- a/drivers/thunderbolt/nhi.c
>> +++ b/drivers/thunderbolt/nhi.c
>> @@ -1103,10 +1103,30 @@ static void nhi_check_quirks(struct tb_nhi *nhi)
>> nhi->quirks |= QUIRK_AUTO_CLEAR_INT;
>> }
>>
>> +enum nhi_iommu_status nhi_check_iommu_for_port(struct pci_dev *pdev)
>> +{
>> + if (!pci_is_pcie(pdev) ||
>> + !(pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT ||
>> + pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM)) {
>> + return IOMMU_UNKNOWN;
>> + }
>> +
>> + if (!device_iommu_mapped(&pdev->dev)) {
>> + return IOMMU_DISABLED;
>> + }
>> +
>> + if (!pdev->untrusted) {
>> + dev_info(&pdev->dev,
>> + "Assuming unreliable Kernel DMA protection\n");
>> + return IOMMU_DISABLED;
>> + }
>> + return IOMMU_ENABLED;
>> +}
>> +
>> static void nhi_check_iommu(struct tb_nhi *nhi)
>> {
>> - struct pci_dev *pdev;
>> - bool port_ok = false;
>> + enum nhi_iommu_status iommu_status = nhi->iommu_dma_protection ?
>> + IOMMU_ENABLED : IOMMU_UNKNOWN;
>>
>> /*
>> * Check for sibling devices that look like they should be our
>> @@ -1117,23 +1137,13 @@ static void nhi_check_iommu(struct tb_nhi *nhi)
>> * 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;
>> -
>> - 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;
>> + if (iommu_status == IOMMU_UNKNOWN) {
>> + struct pci_dev *pdev;
>> + for_each_pci_bridge(pdev, nhi->pdev->bus)
>> + if (iommu_status != IOMMU_DISABLED)
>> + iommu_status = nhi_check_iommu_for_port(pdev);
>> }
>> - nhi->iommu_dma_protection = port_ok;
>> + nhi->iommu_dma_protection = (iommu_status == IOMMU_ENABLED);
>> }
>>
>> static int nhi_init_msi(struct tb_nhi *nhi)
>>
>> diff --git a/drivers/thunderbolt/nhi.h b/drivers/thunderbolt/nhi.h
>> index 69083aab2736..1622d49b1763 100644
>> --- a/drivers/thunderbolt/nhi.h
>> +++ b/drivers/thunderbolt/nhi.h
>> @@ -11,6 +11,13 @@
>>
>> #include <linux/thunderbolt.h>
>>
>> +enum nhi_iommu_status {
>> + IOMMU_UNKNOWN,
>> + IOMMU_DISABLED,
>> + IOMMU_ENABLED,
>> +};
>> +enum nhi_iommu_status nhi_check_iommu_for_port(struct pci_dev *pdev);
>> +
>
> This adds quite a lot code and complexity, and honestly I would like to
> keep it as simple as possible (and this is not enough because we need to
> make sure the DMAR bit is there so that none of the possible connected
> devices were able to overwrite our memory already).
Shall we forget the standalone sibling check and just make the
pdev->untrusted check directly in tb_acpi_add_link() then? On reflection
I guess the DMAR bit makes iommu_dma_protection functionally dependent
on ACPI already, so we don't actually lose anything (and anyone can come
back and revisit firmware-agnostic methods later if a need appears).
Robin.
Powered by blists - more mailing lists