Re: [PATCH v2 1/2] PCI: Allow internal devices to be marked as untrusted

[Rajat Jain]

Hello,

On Wed, Feb 9, 2022 at 10:49 AM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
>
> On Wed, Feb 9, 2022 at 7:39 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> >
> > On Wed, Feb 09, 2022 at 06:46:12AM +0100, Greg Kroah-Hartman wrote:
> > > On Tue, Feb 08, 2022 at 04:23:27PM -0800, Rajat Jain wrote:
> > > > On Tue, Feb 1, 2022 at 6:01 PM Rajat Jain <rajatja@xxxxxxxxxx> wrote:
> > > > >
> > > > > Today the pci_dev->untrusted is set for any devices sitting downstream
> > > > > an external facing port (determined via "ExternalFacingPort" or the
> > > > > "external-facing" properties).
> > > > >
> > > > > However, currently there is no way for internal devices to be marked as
> > > > > untrusted.
> > > > >
> > > > > There are use-cases though, where a platform would like to treat an
> > > > > internal device as untrusted (perhaps because it runs untrusted firmware
> > > > > or offers an attack surface by handling untrusted network data etc).
> > > > >
> > > > > Introduce a new "UntrustedDevice" property that can be used by the
> > > > > firmware to mark any device as untrusted.
> > > >
> > > > Just to unite the threads (from
> > > > https://www.spinics.net/lists/linux-pci/msg120221.html). I did reach
> > > > out to Microsoft but they haven't acknowledged my email. I also pinged
> > > > them again yesterday, but I suspect I may not be able to break the
> > > > ice. So this patch may be ready to go in my opinion.
> > > >
> > > > I don't see any outstanding comments on this patch, but please let me
> > > > know if you have any comments.
> > > >
> > > > > Signed-off-by: Rajat Jain <rajatja@xxxxxxxxxx>
> > > > > ---
> > > > > v2: * Also use the same property for device tree based systems.
> > > > >     * Add documentation (next patch)
> > > > >
> > > > >  drivers/pci/of.c       | 2 ++
> > > > >  drivers/pci/pci-acpi.c | 1 +
> > > > >  drivers/pci/pci.c      | 9 +++++++++
> > > > >  drivers/pci/pci.h      | 2 ++
> > > > >  4 files changed, 14 insertions(+)
> > > > >
> > > > > diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> > > > > index cb2e8351c2cc..e8b804664b69 100644
> > > > > --- a/drivers/pci/of.c
> > > > > +++ b/drivers/pci/of.c
> > > > > @@ -24,6 +24,8 @@ void pci_set_of_node(struct pci_dev *dev)
> > > > >                                                     dev->devfn);
> > > > >         if (dev->dev.of_node)
> > > > >                 dev->dev.fwnode = &dev->dev.of_node->fwnode;
> > > > > +
> > > > > +       pci_set_untrusted(dev);
> > > > >  }
> > > > >
> > > > >  void pci_release_of_node(struct pci_dev *dev)
> > > > > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> > > > > index a42dbf448860..2bffbd5c6114 100644
> > > > > --- a/drivers/pci/pci-acpi.c
> > > > > +++ b/drivers/pci/pci-acpi.c
> > > > > @@ -1356,6 +1356,7 @@ void pci_acpi_setup(struct device *dev, struct acpi_device *adev)
> > > > >
> > > > >         pci_acpi_optimize_delay(pci_dev, adev->handle);
> > > > >         pci_acpi_set_external_facing(pci_dev);
> > > > > +       pci_set_untrusted(pci_dev);
> > > > >         pci_acpi_add_edr_notifier(pci_dev);
> > > > >
> > > > >         pci_acpi_add_pm_notifier(adev, pci_dev);
> > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > > index 9ecce435fb3f..41e887c27004 100644
> > > > > --- a/drivers/pci/pci.c
> > > > > +++ b/drivers/pci/pci.c
> > > > > @@ -6869,3 +6869,12 @@ static int __init pci_realloc_setup_params(void)
> > > > >         return 0;
> > > > >  }
> > > > >  pure_initcall(pci_realloc_setup_params);
> > > > > +
> > > > > +void pci_set_untrusted(struct pci_dev *pdev)
> > > > > +{
> > > > > +       u8 val;
> > > > > +
> > > > > +       if (!device_property_read_u8(&pdev->dev, "UntrustedDevice", &val)
> >
> > If we do this, can we combine it with set_pcie_untrusted(), where we
> > already set pdev->untrusted?  Maybe that needs to be renamed; I don't
> > see anything PCIe-specific there, and it looks like it works for
> > conventional PCI as well.

Yes, I agree it makes sense to combine with set_pcie_untrusted(). I
can do that in the next iteration of my patch, that I intend to work
on after we reach some sort of conclusion on the other major comments
below.

> >
> > > Please no, "Untrusted" does not really convey much, if anything here.
> > > You are taking an odd in-kernel-value and making it a user api.
> > >
> > > Where is this "trust" defined?  Who defines it?  What policy does the
> > > kernel impose on it?
> >
> > I'm a bit hesitant about this, too.  It really doesn't have anything
> > in particular to do with the PCI core.  It's not part of the PCI
> > specs, and it could apply to any kind of device, not just PCI (ACPI,
> > platform, USB, etc).
> >
> > We have:
> >
> >   dev->removable                # struct device
> >   pdev->is_thunderbolt
> >   pdev->untrusted
> >   pdev->external_facing
> >
> > and it feels a little hard to keep everything straight.  Most of them
> > are "discovered" based on some DT or ACPI firmware property.  None of
> > them really has anything specifically to do with *PCI*, and I don't
> > think the PCI core depends on any of them.  I think
> > pdev->is_thunderbolt is the only one we discover based on a PCI
> > feature (the Thunderbolt Capability), and the things we *use* it for
> > are actually not things specified by that capability [1].
> >
> > Could drivers just look for these properties directly instead of
> > relying on the PCI core to get in the middle?  Most callers of
> > device_property_read_*() are in drivers.  I do see that doing it in
> > the PCI core might help enforce standard usage in DT/ACPI, but we
> > could probably do that in other ways, too.
>
> FWIW, I agree that looking at these things in drivers would be better.

The pci_dev->untrusted property is currently used by:

- IOMMU drivers to determine whether bounce buffers should be used,
and whether flush queue should be used for these devices.
- PCI subsystem to determine ACS settings (ATS / TB etc)

As we can see from the usage above, the current primary use of
untrusted property in the kernel is to flag and protect against
devices that can create a DMA attack on the host physical memory
address space (also documented for these properties in [1][2]). IMHO,
this property belongs to PCI devices because:
 * I do not know of any other bus (other than PCI) that can allow DMA
access of the host memory, to a device on that bus.
 * There is some use of this property within the PCI (see above),
although I agree it is not much.
 * The existing properties are currently documented [1][2] to be part
of PCIe root ports / PCI-PCI bridges (only):

[1] https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports
[2] Documentation/devicetree/bindings/pci/pci.txt

One can possibly read the device properties in IOMMU drivers, but
they'd need to keep it in some device structure. I understand moving
the pci_dev->untrusted into struct device has been brought up a couple
of times in the past, and has met with much stronger resistance. The
discussion turned into a discussion on security, and the semantics of
this property, and allowing userspace to change this property etc,
requiring major changes, and thus fizzled out of motivation.

I'd like to mention that I'm not proposing any changes to the way
(already existing) pci_dev->untrusted is being used, or the semantics
of this flag. I'm only trying to solve a corner case here i.e.
internal devices don't have a way to specify this attribute. Thus
requiring us (Chromeos) to carry hacks like [3]. I believe there are
others who are also looking for this corner case. From [4]:

==============================
We have a similar trust issue with the BMC in servers even
though they're internal devices. They're typically network accessible
and infrequently updated so treating them as trustworthy isn't a great
idea. We have been slowly de-privileging the BMC over the last few
years, but the PCIe interface isn't locked down enough for my liking
since the SoCs we use do allow software to set the VDID and perform
arbitrary DMAs (thankfully limited to 32bit). If we're going to add in
infrastructure for handling possibly untrustworthy PCI devices then
I'd like to use that for BMCs too.
=============================

[3] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/3171209
[4] https://lkml.org/lkml/2020/6/9/1467

So from what I see, there is a need to solve this problem for internal
PCI devices. And presently what I have, seemed like the path of least
resistance to me (i.e. without running into big discussions, and major
code changes).

I'd greatly appreciate if you could please consider the information I
presented above, and suggest an approach that you think is more
acceptable.

Thanks & Best Regards,

Rajat

>
> > [1] https://lore.kernel.org/r/20220204222956.GA220908@bhelgaas