[v2,1/4] PCI / ACPI: Identify untrusted PCI devices

Commit Message

Mika Westerberg Nov. 26, 2018, 11:15 a.m. UTC

Recent systems with Thunderbolt ports may support IOMMU natively. This
means that the platform utilizes IOMMU to prevent DMA attacks over
externally exposed PCIe root ports (typically Thunderbolt ports)

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:

  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 <[e-mail]>
---
 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(+)

Comments

Bjorn Helgaas Nov. 27, 2018, 12:17 a.m. UTC | #1

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 <[e-mail]>
> ---
>  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
>

Mika Westerberg Nov. 27, 2018, 8:54 a.m. UTC | #2

On Mon, Nov 26, 2018 at 06:17:11PM -0600, Bjorn Helgaas wrote:
> Hi Mika,

Hi,

> 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.

I guess I'm trying to say here that the recent changes add such support
to the platform BIOS that allows the OS to enable IOMMU without being
compromised by a malicious device that is already connected. The BIOS
sets the new ACPI DMAR bit in that case.

> > 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.

There is no standard but I'm quite sure this is something that will be
required to be implemented properly by the OEM by Microsoft hardware
compatibility suite.

> 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].

OK, I will update the commit message accordingly.

> >   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 <[e-mail]>
> > ---
> >  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;

OK.

> > +}
> > +
> >  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;

I agree the loop is not necessary. I'll change it in the next version.

> > +}
> > +
> >  /**
> >   * 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.

Works for me :)

Rafael J. Wysocki Nov. 27, 2018, 4:49 p.m. UTC | #3

On Tue, Nov 27, 2018 at 9:54 AM Mika Westerberg
<[e-mail]> wrote:
>
> On Mon, Nov 26, 2018 at 06:17:11PM -0600, Bjorn Helgaas wrote:
> > Hi Mika,
>
> Hi,
>
> > 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.
>
> I guess I'm trying to say here that the recent changes add such support
> to the platform BIOS that allows the OS to enable IOMMU without being
> compromised by a malicious device that is already connected. The BIOS
> sets the new ACPI DMAR bit in that case.
>
> > > 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.
>
> There is no standard but I'm quite sure this is something that will be
> required to be implemented properly by the OEM by Microsoft hardware
> compatibility suite.

I think it would be fair to say that future versions of Windows will
expect the firmware to identify the "externally facing" root PCIe
ports as per the above which practically means that it is as good as a
formal standard in the Windows world.

Rafael J. Wysocki Nov. 27, 2018, 5:14 p.m. UTC | #4

On Mon, Nov 26, 2018 at 12:15 PM Mika Westerberg
<[e-mail]> wrote:
>
> Recent systems with Thunderbolt ports may support IOMMU natively. This
> means that the platform utilizes IOMMU to prevent DMA attacks over
> externally exposed PCIe root ports (typically Thunderbolt ports)
>
> 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:
>
>   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 <[e-mail]>
> ---
>  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),
>  };

One observation here.  Note that I really have no strong opinion on
that, so this is not an objection, but IMO it is fair to make things
clear for everybody.

So this causes the External facing port GUID (which already is the
case with the Hotplug in D3 GUID for that matter) to be practically
equivalent to the ACPI _DSD device properties GUID.  This means that
Linux will consider a combination of any of these GUIDs with any
properties defined for any of them as valid, which need not be the
case in Windows.

For example, since the ExternalFacingPort property is defined along
with the External facing port GUID, Windows will likely ignore it if
it is used in a combination with the Hotplug in D3 GUID or the ACPI
_DSD device properties GUID.  If the firmware combines the Hotplug in
D3 GUID or the ACPI _DSD device properties GUID with that property,
Windows will not regard it as valid, most likely, but Linux will use
it just fine.  In the face of ASL bugs, which are inevitable (at least
just because ASL is code written by humans), that may become a real
problem, as systems successfully tested with Windows may not work with
Linux as expected and vice versa because of it.

From the ecosystem purity perspective this should be avoided, but then
I do realize that this allows code to be re-used and avoids quite a
bit of complexity.  The likelihood of an ASL bug that will expose this
issue is arguably small, so maybe it is not a practical concern after
all.

Limonciello, Mario Nov. 27, 2018, 7:10 p.m. UTC | #5

> -----Original Message-----
> From: Rafael J. Wysocki <[e-mail]>
> Sent: Tuesday, November 27, 2018 11:15 AM
> To: Mika Westerberg
> Cc: open list:AMD IOMMU (AMD-VI); Joerg Roedel; David Woodhouse; Lu Baolu;
> Raj, Ashok; Bjorn Helgaas; Rafael J. Wysocki; Pan, Jacob jun; Andreas Noever;
> Michael Jamet; Yehezkel Bernat; Lukas Wunner; [e-mail]; Limonciello,
> Mario; Anthony Wong; Lorenzo Pieralisi; Christoph Hellwig; Alex Williamson; ACPI
> Devel Maling List; Linux PCI; Linux Kernel Mailing List
> Subject: Re: [PATCH v2 1/4] PCI / ACPI: Identify untrusted PCI devices
> 
> 
> [EXTERNAL EMAIL]
> 
> On Mon, Nov 26, 2018 at 12:15 PM Mika Westerberg
> <[e-mail]> wrote:
> >
> > Recent systems with Thunderbolt ports may support IOMMU natively. This
> > means that the platform utilizes IOMMU to prevent DMA attacks over
> > externally exposed PCIe root ports (typically Thunderbolt ports)
> >
> > 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:
> >
> >   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 <[e-mail]>
> > ---
> >  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),
> >  };
> 
> One observation here.  Note that I really have no strong opinion on
> that, so this is not an objection, but IMO it is fair to make things
> clear for everybody.
> 
> So this causes the External facing port GUID (which already is the
> case with the Hotplug in D3 GUID for that matter) to be practically
> equivalent to the ACPI _DSD device properties GUID.  This means that
> Linux will consider a combination of any of these GUIDs with any
> properties defined for any of them as valid, which need not be the
> case in Windows.
> 
> For example, since the ExternalFacingPort property is defined along
> with the External facing port GUID, Windows will likely ignore it if
> it is used in a combination with the Hotplug in D3 GUID or the ACPI
> _DSD device properties GUID.  If the firmware combines the Hotplug in
> D3 GUID or the ACPI _DSD device properties GUID with that property,
> Windows will not regard it as valid, most likely, but Linux will use
> it just fine.  In the face of ASL bugs, which are inevitable (at least
> just because ASL is code written by humans), that may become a real
> problem, as systems successfully tested with Windows may not work with
> Linux as expected and vice versa because of it.
> 
> From the ecosystem purity perspective this should be avoided, but then
> I do realize that this allows code to be re-used and avoids quite a
> bit of complexity.  The likelihood of an ASL bug that will expose this
> issue is arguably small, so maybe it is not a practical concern after
> all.

This is the perfect type of situation that should be raised as a highly
marked bug in FWTS.  FWTS is already in the UEFI self certification suite and 
is being used by IBVs, OEMs and ODMs to find and fix ASL issues.

Mika Westerberg Nov. 28, 2018, 10:54 a.m. UTC | #6

On Tue, Nov 27, 2018 at 06:14:43PM +0100, Rafael J. Wysocki wrote:
> > 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),
> >  };
> 
> One observation here.  Note that I really have no strong opinion on
> that, so this is not an objection, but IMO it is fair to make things
> clear for everybody.
> 
> So this causes the External facing port GUID (which already is the
> case with the Hotplug in D3 GUID for that matter) to be practically
> equivalent to the ACPI _DSD device properties GUID.  This means that
> Linux will consider a combination of any of these GUIDs with any
> properties defined for any of them as valid, which need not be the
> case in Windows.
> 
> For example, since the ExternalFacingPort property is defined along
> with the External facing port GUID, Windows will likely ignore it if
> it is used in a combination with the Hotplug in D3 GUID or the ACPI
> _DSD device properties GUID.  If the firmware combines the Hotplug in
> D3 GUID or the ACPI _DSD device properties GUID with that property,
> Windows will not regard it as valid, most likely, but Linux will use
> it just fine.  In the face of ASL bugs, which are inevitable (at least
> just because ASL is code written by humans), that may become a real
> problem, as systems successfully tested with Windows may not work with
> Linux as expected and vice versa because of it.

That's a fair point.

> >From the ecosystem purity perspective this should be avoided, but then
> I do realize that this allows code to be re-used and avoids quite a
> bit of complexity.  The likelihood of an ASL bug that will expose this
> issue is arguably small, so maybe it is not a practical concern after
> all.

One option assuming we want to prevent the potential discrepancy you
described above is to add ACPI specific property accessors that take
GUID as parameter. Then it would only look properties inside that
particular _DSD entry. I'm not sure if it is worth the added complexity,
though.

Rafael J. Wysocki Nov. 28, 2018, 11:24 a.m. UTC | #7

On Wed, Nov 28, 2018 at 11:54 AM Mika Westerberg
<[e-mail]> wrote:
>
> On Tue, Nov 27, 2018 at 06:14:43PM +0100, Rafael J. Wysocki wrote:
> > > 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),
> > >  };
> >
> > One observation here.  Note that I really have no strong opinion on
> > that, so this is not an objection, but IMO it is fair to make things
> > clear for everybody.
> >
> > So this causes the External facing port GUID (which already is the
> > case with the Hotplug in D3 GUID for that matter) to be practically
> > equivalent to the ACPI _DSD device properties GUID.  This means that
> > Linux will consider a combination of any of these GUIDs with any
> > properties defined for any of them as valid, which need not be the
> > case in Windows.
> >
> > For example, since the ExternalFacingPort property is defined along
> > with the External facing port GUID, Windows will likely ignore it if
> > it is used in a combination with the Hotplug in D3 GUID or the ACPI
> > _DSD device properties GUID.  If the firmware combines the Hotplug in
> > D3 GUID or the ACPI _DSD device properties GUID with that property,
> > Windows will not regard it as valid, most likely, but Linux will use
> > it just fine.  In the face of ASL bugs, which are inevitable (at least
> > just because ASL is code written by humans), that may become a real
> > problem, as systems successfully tested with Windows may not work with
> > Linux as expected and vice versa because of it.
>
> That's a fair point.
>
> > >From the ecosystem purity perspective this should be avoided, but then
> > I do realize that this allows code to be re-used and avoids quite a
> > bit of complexity.  The likelihood of an ASL bug that will expose this
> > issue is arguably small, so maybe it is not a practical concern after
> > all.
>
> One option assuming we want to prevent the potential discrepancy you
> described above is to add ACPI specific property accessors that take
> GUID as parameter. Then it would only look properties inside that
> particular _DSD entry. I'm not sure if it is worth the added complexity,
> though.

I'm not sure if this is worth the extra complexity either, which is
why I have no strong opinion here. :-)

Maybe you can add a comment, next to the prp_guids[] definition, to
explain that the GUIDs are made equivalent to each other in order to
avoid extra complexity in the properties handling code, with the
caveat that the kernel will accept certain combinations of GUIDs and
properties that are not defined strictly speaking without warning, but
those combinations of GUIDs and properties are not expected to be used
by firmware and they should be caught by firmware validation tools and
reported as errors anyway.

Mika Westerberg Nov. 28, 2018, 11:39 a.m. UTC | #8

On Wed, Nov 28, 2018 at 12:24:27PM +0100, Rafael J. Wysocki wrote:
> I'm not sure if this is worth the extra complexity either, which is
> why I have no strong opinion here. :-)
> 
> Maybe you can add a comment, next to the prp_guids[] definition, to
> explain that the GUIDs are made equivalent to each other in order to
> avoid extra complexity in the properties handling code, with the
> caveat that the kernel will accept certain combinations of GUIDs and
> properties that are not defined strictly speaking without warning, but
> those combinations of GUIDs and properties are not expected to be used
> by firmware and they should be caught by firmware validation tools and
> reported as errors anyway.

Sure, I'll add the comment in the next version of the series.

Bjorn Helgaas Nov. 28, 2018, 8:31 p.m. UTC | #9

On Tue, Nov 27, 2018 at 10:54:26AM +0200, Mika Westerberg wrote:
> On Mon, Nov 26, 2018 at 06:17:11PM -0600, Bjorn Helgaas wrote:
> > Hi Mika,
> 
> Hi,
> 
> > 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.

I think by "platform" you're referring to the system firmware; I was
only thinking of the hardware, so the IOMMU wouldn't be used unless
someone (the OS) enabled it.  But your cover letter talks about the
BIOS enabling some IOMMU functionality.

> I guess I'm trying to say here that the recent changes add such support
> to the platform BIOS that allows the OS to enable IOMMU without being
> compromised by a malicious device that is already connected. The BIOS
> sets the new ACPI DMAR bit in that case.

Ah, there's useful info to this effect in your [0/4] cover letter.
That info and the URL should be in the changelog of one of the patches so
it doesn't get lost.

> > > 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.
> 
> There is no standard but I'm quite sure this is something that will be
> required to be implemented properly by the OEM by Microsoft hardware
> compatibility suite.

Sure.  Your statement suggests that all external ports will be marked
with the _DSD.  I'm just pointing out that the OS can't assume that
because there are probably systems in the field that predate the _DSD.

Bjorn

Patch

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;
+}
+
 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);
+	}
+}
+
 /**
  * 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;
 	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 */