| 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 |
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 >
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
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 >
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 > >
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 --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 */
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(+)