diff mbox series

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

Message ID 20220202020103.2149130-1-rajatja@google.com
State New
Headers show
Series [v2,1/2] PCI: Allow internal devices to be marked as untrusted | expand

Commit Message

Rajat Jain Feb. 2, 2022, 2:01 a.m. UTC
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(+)

Comments

Rajat Jain Feb. 9, 2022, 12:23 a.m. UTC | #1
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
>
Greg KH Feb. 9, 2022, 5:46 a.m. UTC | #2
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
Rafael J. Wysocki Feb. 9, 2022, 7:11 p.m. UTC | #3
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
>
Rafael J. Wysocki Feb. 9, 2022, 7:18 p.m. UTC | #4
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
> >
Rajat Jain Feb. 9, 2022, 10:03 p.m. UTC | #5
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
> >
diff mbox series

Patch

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 */