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

[Rafael J. Wysocki]

On Mon, Nov 26, 2018 at 12:15 PM Mika Westerberg
<mika.westerberg@xxxxxxxxxxxxxxx> 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 <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),
>  };

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.