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

[Bjorn Helgaas]

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.

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

Bjorn

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