* [PATCH v2 1/2] PCI: Allow internal devices to be marked as untrusted @ 2022-02-02 2:01 Rajat Jain 2022-02-02 2:01 ` [PATCH v2 2/2] dt-bindings: Document "UntrustedDevice" property for PCI devices Rajat Jain ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Rajat Jain @ 2022-02-02 2:01 UTC (permalink / raw) To: Rob Herring, Rafael J. Wysocki, Len Brown, linux-pci, devicetree, Mika Westerberg, Greg Kroah-Hartman, Bjorn Helgaas, Bjorn Helgaas, ACPI Devel Maling List, Linux Kernel Mailing List, Rajat Jain, Dmitry Torokhov, Jesse Barnes, Jean-Philippe Brucker, Pavel Machek, Oliver O'Halloran, Joerg Roedel Cc: 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 2/2] dt-bindings: Document "UntrustedDevice" property for PCI devices 2022-02-02 2:01 [PATCH v2 1/2] PCI: Allow internal devices to be marked as untrusted Rajat Jain @ 2022-02-02 2:01 ` Rajat Jain 2022-02-09 21:57 ` Rob Herring 2022-02-09 0:23 ` [PATCH v2 1/2] PCI: Allow internal devices to be marked as untrusted Rajat Jain 2022-02-09 19:11 ` Rafael J. Wysocki 2 siblings, 1 reply; 12+ messages in thread From: Rajat Jain @ 2022-02-02 2:01 UTC (permalink / raw) To: Rob Herring, Rafael J. Wysocki, Len Brown, linux-pci, devicetree, Mika Westerberg, Greg Kroah-Hartman, Bjorn Helgaas, Bjorn Helgaas, ACPI Devel Maling List, Linux Kernel Mailing List, Rajat Jain, Dmitry Torokhov, Jesse Barnes, Jean-Philippe Brucker, Pavel Machek, Oliver O'Halloran, Joerg Roedel Cc: 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] dt-bindings: Document "UntrustedDevice" property for PCI devices 2022-02-02 2:01 ` [PATCH v2 2/2] dt-bindings: Document "UntrustedDevice" property for PCI devices Rajat Jain @ 2022-02-09 21:57 ` Rob Herring 0 siblings, 0 replies; 12+ messages in thread From: Rob Herring @ 2022-02-09 21:57 UTC (permalink / raw) To: Rajat Jain Cc: Rafael J. Wysocki, Len Brown, linux-pci, devicetree, Mika Westerberg, Greg Kroah-Hartman, Bjorn Helgaas, Bjorn Helgaas, ACPI Devel Maling List, Linux Kernel Mailing List, Rajat Jain, Dmitry Torokhov, Jesse Barnes, Jean-Philippe Brucker, Pavel Machek, Oliver O'Halloran, Joerg Roedel 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] PCI: Allow internal devices to be marked as untrusted 2022-02-02 2:01 [PATCH v2 1/2] PCI: Allow internal devices to be marked as untrusted Rajat Jain 2022-02-02 2:01 ` [PATCH v2 2/2] dt-bindings: Document "UntrustedDevice" property for PCI devices Rajat Jain @ 2022-02-09 0:23 ` Rajat Jain 2022-02-09 5:46 ` Greg Kroah-Hartman 2022-02-09 19:11 ` Rafael J. Wysocki 2 siblings, 1 reply; 12+ messages in thread From: Rajat Jain @ 2022-02-09 0:23 UTC (permalink / raw) To: Rob Herring, Rafael J. Wysocki, Len Brown, linux-pci, devicetree, Mika Westerberg, Greg Kroah-Hartman, Bjorn Helgaas, Bjorn Helgaas, ACPI Devel Maling List, Linux Kernel Mailing List, Rajat Jain, Dmitry Torokhov, Jesse Barnes, Jean-Philippe Brucker, Pavel Machek, Oliver O'Halloran, Joerg Roedel 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] PCI: Allow internal devices to be marked as untrusted 2022-02-09 0:23 ` [PATCH v2 1/2] PCI: Allow internal devices to be marked as untrusted Rajat Jain @ 2022-02-09 5:46 ` Greg Kroah-Hartman 2022-02-09 18:39 ` Bjorn Helgaas 0 siblings, 1 reply; 12+ messages in thread From: Greg Kroah-Hartman @ 2022-02-09 5:46 UTC (permalink / raw) To: Rajat Jain Cc: Rob Herring, Rafael J. Wysocki, Len Brown, linux-pci, devicetree, Mika Westerberg, Bjorn Helgaas, Bjorn Helgaas, ACPI Devel Maling List, Linux Kernel Mailing List, Rajat Jain, Dmitry Torokhov, Jesse Barnes, Jean-Philippe Brucker, Pavel Machek, Oliver O'Halloran, Joerg Roedel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] PCI: Allow internal devices to be marked as untrusted 2022-02-09 5:46 ` Greg Kroah-Hartman @ 2022-02-09 18:39 ` Bjorn Helgaas 2022-02-09 18:49 ` Rafael J. Wysocki 0 siblings, 1 reply; 12+ messages in thread From: Bjorn Helgaas @ 2022-02-09 18:39 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Rajat Jain, Rob Herring, Rafael J. Wysocki, Len Brown, linux-pci, devicetree, Mika Westerberg, Bjorn Helgaas, ACPI Devel Maling List, Linux Kernel Mailing List, Rajat Jain, Dmitry Torokhov, Jesse Barnes, Jean-Philippe Brucker, Pavel Machek, Oliver O'Halloran, Joerg Roedel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] PCI: Allow internal devices to be marked as untrusted 2022-02-09 18:39 ` Bjorn Helgaas @ 2022-02-09 18:49 ` Rafael J. Wysocki 2022-02-09 22:00 ` Rajat Jain 0 siblings, 1 reply; 12+ messages in thread From: Rafael J. Wysocki @ 2022-02-09 18:49 UTC (permalink / raw) To: Bjorn Helgaas Cc: Greg Kroah-Hartman, Rajat Jain, Rob Herring, Rafael J. Wysocki, Len Brown, Linux PCI, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Mika Westerberg, Bjorn Helgaas, ACPI Devel Maling List, Linux Kernel Mailing List, Rajat Jain, Dmitry Torokhov, Jesse Barnes, Jean-Philippe Brucker, Pavel Machek, Oliver O'Halloran, Joerg Roedel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] PCI: Allow internal devices to be marked as untrusted 2022-02-09 18:49 ` Rafael J. Wysocki @ 2022-02-09 22:00 ` Rajat Jain 2022-02-10 7:53 ` Greg Kroah-Hartman 0 siblings, 1 reply; 12+ messages in thread From: Rajat Jain @ 2022-02-09 22:00 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Bjorn Helgaas, Greg Kroah-Hartman, Rob Herring, Len Brown, Linux PCI, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Mika Westerberg, Bjorn Helgaas, ACPI Devel Maling List, Linux Kernel Mailing List, Rajat Jain, Dmitry Torokhov, Jesse Barnes, Jean-Philippe Brucker, Pavel Machek, Oliver O'Halloran, Joerg Roedel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] PCI: Allow internal devices to be marked as untrusted 2022-02-09 22:00 ` Rajat Jain @ 2022-02-10 7:53 ` Greg Kroah-Hartman 0 siblings, 0 replies; 12+ messages in thread From: Greg Kroah-Hartman @ 2022-02-10 7:53 UTC (permalink / raw) To: Rajat Jain Cc: Rafael J. Wysocki, Bjorn Helgaas, Rob Herring, Len Brown, Linux PCI, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Mika Westerberg, Bjorn Helgaas, ACPI Devel Maling List, Linux Kernel Mailing List, Rajat Jain, Dmitry Torokhov, Jesse Barnes, Jean-Philippe Brucker, Pavel Machek, Oliver O'Halloran, Joerg Roedel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] PCI: Allow internal devices to be marked as untrusted 2022-02-02 2:01 [PATCH v2 1/2] PCI: Allow internal devices to be marked as untrusted Rajat Jain 2022-02-02 2:01 ` [PATCH v2 2/2] dt-bindings: Document "UntrustedDevice" property for PCI devices Rajat Jain 2022-02-09 0:23 ` [PATCH v2 1/2] PCI: Allow internal devices to be marked as untrusted Rajat Jain @ 2022-02-09 19:11 ` Rafael J. Wysocki 2022-02-09 19:18 ` Rafael J. Wysocki 2022-02-09 22:03 ` Rajat Jain 2 siblings, 2 replies; 12+ messages in thread From: Rafael J. Wysocki @ 2022-02-09 19:11 UTC (permalink / raw) To: Rajat Jain Cc: Rob Herring, Rafael J. Wysocki, Len Brown, Linux PCI, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Mika Westerberg, Greg Kroah-Hartman, Bjorn Helgaas, Bjorn Helgaas, ACPI Devel Maling List, Linux Kernel Mailing List, Rajat Jain, Dmitry Torokhov, Jesse Barnes, Jean-Philippe Brucker, Pavel Machek, Oliver O'Halloran, Joerg Roedel 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] PCI: Allow internal devices to be marked as untrusted 2022-02-09 19:11 ` Rafael J. Wysocki @ 2022-02-09 19:18 ` Rafael J. Wysocki 2022-02-09 22:03 ` Rajat Jain 1 sibling, 0 replies; 12+ messages in thread From: Rafael J. Wysocki @ 2022-02-09 19:18 UTC (permalink / raw) To: Rajat Jain Cc: Rob Herring, Rafael J. Wysocki, Len Brown, Linux PCI, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Mika Westerberg, Greg Kroah-Hartman, Bjorn Helgaas, Bjorn Helgaas, ACPI Devel Maling List, Linux Kernel Mailing List, Rajat Jain, Dmitry Torokhov, Jesse Barnes, Jean-Philippe Brucker, Pavel Machek, Oliver O'Halloran, Joerg Roedel 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 > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] PCI: Allow internal devices to be marked as untrusted 2022-02-09 19:11 ` Rafael J. Wysocki 2022-02-09 19:18 ` Rafael J. Wysocki @ 2022-02-09 22:03 ` Rajat Jain 1 sibling, 0 replies; 12+ messages in thread From: Rajat Jain @ 2022-02-09 22:03 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Rob Herring, Len Brown, Linux PCI, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Mika Westerberg, Greg Kroah-Hartman, Bjorn Helgaas, Bjorn Helgaas, ACPI Devel Maling List, Linux Kernel Mailing List, Rajat Jain, Dmitry Torokhov, Jesse Barnes, Jean-Philippe Brucker, Pavel Machek, Oliver O'Halloran, Joerg Roedel 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 > > ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-02-10 7:53 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-02-02 2:01 [PATCH v2 1/2] PCI: Allow internal devices to be marked as untrusted Rajat Jain 2022-02-02 2:01 ` [PATCH v2 2/2] dt-bindings: Document "UntrustedDevice" property for PCI devices Rajat Jain 2022-02-09 21:57 ` Rob Herring 2022-02-09 0:23 ` [PATCH v2 1/2] PCI: Allow internal devices to be marked as untrusted Rajat Jain 2022-02-09 5:46 ` Greg Kroah-Hartman 2022-02-09 18:39 ` Bjorn Helgaas 2022-02-09 18:49 ` Rafael J. Wysocki 2022-02-09 22:00 ` Rajat Jain 2022-02-10 7:53 ` Greg Kroah-Hartman 2022-02-09 19:11 ` Rafael J. Wysocki 2022-02-09 19:18 ` Rafael J. Wysocki 2022-02-09 22:03 ` Rajat Jain
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).