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

[Mario.Limonciello]

> -----Original Message-----
> From: Rafael J. Wysocki <rafael@xxxxxxxxxx>
> 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; ckellner@xxxxxxxxxx; 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
> <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.

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.