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

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

[Rajat Jain]

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.

Signed-off-by: Rajat Jain <rajatja@google.com>
---
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)
+	    && val)
+		pdev->untrusted = 1;
+}
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 3d60cabde1a1..6c273ce5e0ba 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -761,4 +761,6 @@ static inline pci_power_t mid_pci_get_power_state(struct pci_dev *pdev)
 }
 #endif
 
+void pci_set_untrusted(struct pci_dev *pdev);
+
 #endif /* DRIVERS_PCI_H */
-- 
2.35.0.rc2.247.g8bbb082509-goog

[PATCH v2 2/2] dt-bindings: Document "UntrustedDevice" property for PCI devices

[Rajat Jain]

Add the new "UntrustedDevice" property for PCI devices. This property
is optional and can be applied to any PCI device.

Signed-off-by: Rajat Jain <rajatja@google.com>
---
v2: Initial version (added documentation based on comments)
v1: Does not exist.

 Documentation/devicetree/bindings/pci/pci.txt | 35 +++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/pci.txt b/Documentation/devicetree/bindings/pci/pci.txt
index 6a8f2874a24d..bc1ba10f51e1 100644
--- a/Documentation/devicetree/bindings/pci/pci.txt
+++ b/Documentation/devicetree/bindings/pci/pci.txt
@@ -82,3 +82,38 @@ pcie@10000000 {
 		external-facing;
 	};
 };
+
+PCI Device Properties
+---------------------
+Following optional properties may be present for any PCI device:
+
+- UntrustedDevice:
+   When present, this property is an indicator that this PCI device (and
+   any downstream devices) are to be treated as untrusted by the kernel.
+   The kernel can, for example, use this information to isolate such
+   devices using a strict DMA protection via the IOMMU.
+
+   Example device tree node:
+	pcie@0008 {
+		/* PCI device 00:01.0 is an untrusted device */
+		reg = <0x00000800 0 0 0 0>;
+		UntrustedDevice = <1>;
+	};
+
+   Example ACPI node:
+	Scope (\_SB.PCI0.WFA3)
+	    {
+	        Name (_DSD, Package (0x02)  // _DSD: Device-Specific Data
+	        {
+	            ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device
+	Properties for _DSD */,
+	            Package (0x01)
+	            {
+	                Package (0x02)
+	                {
+	                    "UntrustedDevice",
+	                    One
+	                }
+	            }
+	        })
+	    }
-- 
2.35.0.rc2.247.g8bbb082509-goog

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

[Rajat Jain]

Hello Folks,


On Tue, Feb 1, 2022 at 6:01 PM Rajat Jain <rajatja@google.com> 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.

Thanks & Best Regards,

Rajat


>
> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---
> 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)
> +           && val)
> +               pdev->untrusted = 1;
> +}
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 3d60cabde1a1..6c273ce5e0ba 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -761,4 +761,6 @@ static inline pci_power_t mid_pci_get_power_state(struct pci_dev *pdev)
>  }
>  #endif
>
> +void pci_set_untrusted(struct pci_dev *pdev);
> +
>  #endif /* DRIVERS_PCI_H */
> --
> 2.35.0.rc2.247.g8bbb082509-goog
>

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

[Greg Kroah-Hartman]

On Tue, Feb 08, 2022 at 04:23:27PM -0800, Rajat Jain wrote:
> Hello Folks,
> 
> 
> On Tue, Feb 1, 2022 at 6:01 PM Rajat Jain <rajatja@google.com> 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.
> 
> Thanks & Best Regards,
> 
> Rajat
> 
> 
> >
> > Signed-off-by: Rajat Jain <rajatja@google.com>
> > ---
> > 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)

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?

By putting this value in a firmware requirement like this, it better be
documented VERY VERY well.

thanks,

greg k-h

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@google.com> 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@google.com>
> > > ---
> > > 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

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

[Rafael J. Wysocki]

On Wed, Feb 9, 2022 at 7:39 PM Bjorn Helgaas <helgaas@kernel.org> 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@google.com> 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@google.com>
> > > > ---
> > > > 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.

FWIW, I agree that looking at these things in drivers would be better.

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

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

[Rafael J. Wysocki]

On Wed, Feb 2, 2022 at 3:01 AM Rajat Jain <rajatja@google.com> 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.
>
> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---
> 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)
> +           && val)
> +               pdev->untrusted = 1;

I'm not sure why you ignore val = 0.  Is it not a valid value?

The property is not particularly well defined here.  It is not clear
from its name that it only applies to PCI devices and how.

AFAICS, the "untrusted" bit affected by it is only used by the ATS
code and in one PCH ACS quirk, but I'm not sure if this is all you
have in mind.

> +}
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 3d60cabde1a1..6c273ce5e0ba 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -761,4 +761,6 @@ static inline pci_power_t mid_pci_get_power_state(struct pci_dev *pdev)
>  }
>  #endif
>
> +void pci_set_untrusted(struct pci_dev *pdev);
> +
>  #endif /* DRIVERS_PCI_H */
> --
> 2.35.0.rc2.247.g8bbb082509-goog
>

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

[Rafael J. Wysocki]

On Wed, Feb 9, 2022 at 8:11 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Feb 2, 2022 at 3:01 AM Rajat Jain <rajatja@google.com> 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.
> >
> > Signed-off-by: Rajat Jain <rajatja@google.com>
> > ---
> > 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)
> > +           && val)
> > +               pdev->untrusted = 1;
>
> I'm not sure why you ignore val = 0.  Is it not a valid value?
>
> The property is not particularly well defined here.  It is not clear
> from its name that it only applies to PCI devices and how.
>
> AFAICS, the "untrusted" bit affected by it is only used by the ATS
> code and in one PCH ACS quirk, but I'm not sure if this is all you
> have in mind.

Besides, sort of in the bikeshedding territory, its name doesn't
follow the guidelines given in the _DSD guide:
https://github.com/UEFI/DSD-Guide/blob/main/dsd-guide.pdf

I do realize that you want it to be valid for both ACPI and DT, but
that doesn't preclude following the guidelines AFAICS.

> > +}
> > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > index 3d60cabde1a1..6c273ce5e0ba 100644
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -761,4 +761,6 @@ static inline pci_power_t mid_pci_get_power_state(struct pci_dev *pdev)
> >  }
> >  #endif
> >
> > +void pci_set_untrusted(struct pci_dev *pdev);
> > +
> >  #endif /* DRIVERS_PCI_H */
> > --
> > 2.35.0.rc2.247.g8bbb082509-goog
> >

Re: [PATCH v2 2/2] dt-bindings: Document "UntrustedDevice" property for PCI devices

[Rob Herring]

On Tue, Feb 01, 2022 at 06:01:03PM -0800, Rajat Jain wrote:
> Add the new "UntrustedDevice" property for PCI devices. This property
> is optional and can be applied to any PCI device.
> 
> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---
> v2: Initial version (added documentation based on comments)
> v1: Does not exist.
> 
>  Documentation/devicetree/bindings/pci/pci.txt | 35 +++++++++++++++++++
>  1 file changed, 35 insertions(+)

New properties have to be in a schema which resides here:

https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/pci/pci-bus.yaml

> 
> diff --git a/Documentation/devicetree/bindings/pci/pci.txt b/Documentation/devicetree/bindings/pci/pci.txt
> index 6a8f2874a24d..bc1ba10f51e1 100644
> --- a/Documentation/devicetree/bindings/pci/pci.txt
> +++ b/Documentation/devicetree/bindings/pci/pci.txt
> @@ -82,3 +82,38 @@ pcie@10000000 {
>  		external-facing;
>  	};
>  };
> +
> +PCI Device Properties
> +---------------------
> +Following optional properties may be present for any PCI device:
> +
> +- UntrustedDevice:
> +   When present, this property is an indicator that this PCI device (and
> +   any downstream devices) are to be treated as untrusted by the kernel.
> +   The kernel can, for example, use this information to isolate such
> +   devices using a strict DMA protection via the IOMMU.
> +
> +   Example device tree node:
> +	pcie@0008 {
> +		/* PCI device 00:01.0 is an untrusted device */
> +		reg = <0x00000800 0 0 0 0>;
> +		UntrustedDevice = <1>;
> +	};
> +
> +   Example ACPI node:

Humm, your caret case smelled like ACPI to begin with. As far as ACPI 
bindings in Documentation/devicetree/bindings/ are concerned, NAK.

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@kernel.org> wrote:
>
> On Wed, Feb 9, 2022 at 7:39 PM Bjorn Helgaas <helgaas@kernel.org> 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@google.com> 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@google.com>
> > > > > ---
> > > > > 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

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

[Rajat Jain]

Hello,

On Wed, Feb 9, 2022 at 11:11 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Feb 2, 2022 at 3:01 AM Rajat Jain <rajatja@google.com> 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.
> >
> > Signed-off-by: Rajat Jain <rajatja@google.com>
> > ---
> > 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)
> > +           && val)
> > +               pdev->untrusted = 1;
>
> I'm not sure why you ignore val = 0.  Is it not a valid value?

I'm following the other similar properties that Bjorn mentioned. The
pdev->untrusted is already initialized to 0 so it wouldn't matter.

>
> The property is not particularly well defined here.  It is not clear
> from its name that it only applies to PCI devices and how.
>
> AFAICS, the "untrusted" bit affected by it is only used by the ATS
> code and in one PCH ACS quirk, but I'm not sure if this is all you
> have in mind.

I hope my other response addressed this one.

Thanks & Best Regards,

Rajat

>
> > +}
> > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > index 3d60cabde1a1..6c273ce5e0ba 100644
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -761,4 +761,6 @@ static inline pci_power_t mid_pci_get_power_state(struct pci_dev *pdev)
> >  }
> >  #endif
> >
> > +void pci_set_untrusted(struct pci_dev *pdev);
> > +
> >  #endif /* DRIVERS_PCI_H */
> > --
> > 2.35.0.rc2.247.g8bbb082509-goog
> >

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

[Greg Kroah-Hartman]

On Wed, Feb 09, 2022 at 02:00:54PM -0800, Rajat Jain wrote:
> Hello,
> 
> On Wed, Feb 9, 2022 at 10:49 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Wed, Feb 9, 2022 at 7:39 PM Bjorn Helgaas <helgaas@kernel.org> 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@google.com> 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@google.com>
> > > > > > ---
> > > > > > 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.

Then how about naming it "use_iommu" or something like that?  "Trust"
has nothing to do with this at all.

> - PCI subsystem to determine ACS settings (ATS / TB etc)

Why is this relevant?

> 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

Then let us mark these as "able to do DMA" or something like that.
"Trust" is a userspace policy decision, not a kernel decision to make.

And there are other busses that can do DMA, PCI is not unique here.

> One can possibly read the device properties in IOMMU drivers, but
> they'd need to keep it in some device structure.

That's fine, let's move it there.

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

Because of the issues I am raising here.  It's a bad name and doesn't
mean what people think it means.

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

So I guess no one really cares :)

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

Why does Chromeos care about this flag?  What userspace decisions do you
make based on it?

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

It needs those code changes, please do not try to keep adding more to
this to avoid the real-work that is needed here.  Refer to those other
discussions you mention above for what should happen to do this
correctly.

thanks,

greg k-h