Hi Mika,
On Mon, Nov 26, 2018 at 02:15:23PM +0300, Mika Westerberg wrote:
> Recent systems with Thunderbolt ports may support IOMMU natively.
This sentence doesn't make sense to me. There's no logical connection
between having an IOMMU and having a Thunderbolt port.
> This means that the platform utilizes IOMMU to prevent DMA attacks
> over externally exposed PCIe root ports (typically Thunderbolt
> ports)
Nor this one. The platform only uses the IOMMU to prevent DMA attacks
if the OS chooses to do that.
> The system BIOS marks these PCIe root ports as being externally facing
> ports by implementing following ACPI _DSD [1] under the root port in
> question:
There's no standard that requires this, so the best we can say is that
a system BIOS *may* mark externally facing ports with this mechanism.
I think it would make sense to say something like:
A malicious PCI device may use DMA to attack the system. An
external Thunderbolt port is a convenient point to attach such a
device. The OS may use an IOMMU to defend against DMA attacks.
Some BIOSes mark externally facing Root Ports with the this ACPI
_DSD:
...
If we find such a Root Port, mark it and all its children as
"is_untrusted". The rest of the OS may use this to prevent use of
the device unless we have an IOMMU to prevent malicious DMA [I don't
know your actual policy yet].
> Name (_DSD, Package () {
> ToUUID ("efcc06cc-73ac-4bc3-bff0-76143807c389"),
> Package () {
> Package () {"ExternalFacingPort", 1},
> Package () {"UID", 0 }
> }
> })
>
> To make it possible for IOMMU code to identify these devices, we look up
> for this property and mark all children devices (including the root port
> itself) with a new flag (->is_untrusted). This flag is meant to be used
> with all PCIe devices (not just Thunderbolt) that cannot be trusted in
> the same level than integrated devices and may need to put behind full
> IOMMU protection.
>
> [1] https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-externally-exposed-pcie-root-ports
>
> Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> ---
> drivers/acpi/property.c | 3 +++
> drivers/pci/pci-acpi.c | 18 ++++++++++++++++++
> drivers/pci/probe.c | 22 ++++++++++++++++++++++
> include/linux/pci.h | 8 ++++++++
> 4 files changed, 51 insertions(+)
>
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index 8c7c4583b52d..4bdad32f62c8 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -31,6 +31,9 @@ static const guid_t prp_guids[] = {
> /* Hotplug in D3 GUID: 6211e2c0-58a3-4af3-90e1-927a4e0c55a4 */
> GUID_INIT(0x6211e2c0, 0x58a3, 0x4af3,
> 0x90, 0xe1, 0x92, 0x7a, 0x4e, 0x0c, 0x55, 0xa4),
> + /* External facing port GUID: efcc06cc-73ac-4bc3-bff0-76143807c389 */
> + GUID_INIT(0xefcc06cc, 0x73ac, 0x4bc3,
> + 0xbf, 0xf0, 0x76, 0x14, 0x38, 0x07, 0xc3, 0x89),
> };
>
> static const guid_t ads_guid =
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 921db6f80340..84233cf46fc2 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -789,6 +789,23 @@ static void pci_acpi_optimize_delay(struct pci_dev *pdev,
> ACPI_FREE(obj);
> }
>
> +static void pci_acpi_set_untrusted(struct pci_dev *dev)
> +{
> + u8 val;
> +
> + if (pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT)
> + return;
> + if (device_property_read_u8(&dev->dev, "ExternalFacingPort", &val))
> + return;
> +
> + /*
> + * These root ports expose PCIe (including DMA) outside of the
> + * system so make sure we treat them and everything behind as
> + * untrusted.
> + */
> + dev->is_untrusted = val == 1;
Simpler to parse:
if (val)
dev->is_untrusted = 1;
> +}
> +
> static void pci_acpi_setup(struct device *dev)
> {
> struct pci_dev *pci_dev = to_pci_dev(dev);
> @@ -798,6 +815,7 @@ static void pci_acpi_setup(struct device *dev)
> return;
>
> pci_acpi_optimize_delay(pci_dev, adev->handle);
> + pci_acpi_set_untrusted(pci_dev);
>
> pci_acpi_add_pm_notifier(adev, pci_dev);
> if (!adev->wakeup.flags.valid)
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index b1c05b5054a0..144623ae2e68 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1378,6 +1378,26 @@ static void set_pcie_thunderbolt(struct pci_dev *dev)
> }
> }
>
> +static void set_pcie_untrusted(struct pci_dev *dev)
> +{
> + struct pci_dev *parent;
> +
> + /*
> + * Walk up the device hierarchy and check for any upstream bridge
> + * that has is_untrusted set to true. In that case treat this one
> + * untrusted as well.
> + */
> + parent = pci_upstream_bridge(dev);
> + while (parent) {
> + if (parent->is_untrusted) {
> + dev->is_untrusted = true;
> + break;
> + }
> +
> + parent = pci_upstream_bridge(parent);
> + }
In what circumstance is this loop needed? I expected this would be
sufficient:
bridge = pci_upstream_bridge(dev);
if (bridge && bridge->is_untrusted)
dev->is_untrusted = true;
> +}
> +
> /**
> * pci_ext_cfg_is_aliased - Is ext config space just an alias of std config?
> * @dev: PCI device
> @@ -1638,6 +1658,8 @@ int pci_setup_device(struct pci_dev *dev)
> /* Need to have dev->cfg_size ready */
> set_pcie_thunderbolt(dev);
>
> + set_pcie_untrusted(dev);
> +
> /* "Unknown power state" */
> dev->current_state = PCI_UNKNOWN;
>
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 11c71c4ecf75..3fa73cc6cf68 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -396,6 +396,14 @@ struct pci_dev {
> unsigned int is_hotplug_bridge:1;
> unsigned int shpc_managed:1; /* SHPC owned by shpchp */
> unsigned int is_thunderbolt:1; /* Thunderbolt controller */
> + /*
> + * Devices marked being untrusted are the ones that can potentially
> + * execute DMA attacks and similar. They are typically connected
> + * through external ports such as Thunderbolt but not limited to
> + * that. When an IOMMU is enabled they should be getting full
> + * mappings to make sure they cannot access arbitrary memory.
> + */
> + unsigned int is_untrusted:1;
We have a mix of "is_X" and "X", but I think "untrusted" is sufficient
since "untrusted" is a perfectly good predicate all by itself, i.e.,
if (dev->untrusted)
reads easily and makes good sense.
> unsigned int __aer_firmware_first_valid:1;
> unsigned int __aer_firmware_first:1;
> unsigned int broken_intx_masking:1; /* INTx masking can't be used */
> --
> 2.19.1
>
