| Message ID | 20220120000409.2706549-1-rajatja@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: ACPI: Allow internal devices to be marked as untrusted | expand |
Hi Rajat, On Wed, Jan 19, 2022 at 4:04 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" property). > This however, disallows any 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). > > This patch introduces 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> > --- > drivers/pci/pci-acpi.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > index a42dbf448860..3d9e5fa49451 100644 > --- a/drivers/pci/pci-acpi.c > +++ b/drivers/pci/pci-acpi.c > @@ -1350,12 +1350,25 @@ static void pci_acpi_set_external_facing(struct pci_dev *dev) > dev->external_facing = 1; > } > > +static void pci_acpi_set_untrusted(struct pci_dev *dev) > +{ > + u8 val; > + > + if (device_property_read_u8(&dev->dev, "UntrustedDevice", &val)) > + return; > + > + /* These PCI devices are not trustworthy */ > + if (val) > + dev->untrusted = 1; Should this all be replaced with: dev->untrusted = device_property_read_bool(&dev->dev, "UntrustedDevice"); ? Also, is this ACPI-specific? Why won't we need this for DT systems (or do we already have this)?. Thanks, Dmitry
Hi Dmitry, Bjorn, Thanks for your review and comments. On Wed, Jan 19, 2022 at 6:25 PM Dmitry Torokhov <dtor@google.com> wrote: > > Hi Rajat, > > On Wed, Jan 19, 2022 at 4:04 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" property). > > This however, disallows any 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). > > > > This patch introduces 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> > > --- > > drivers/pci/pci-acpi.c | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > > index a42dbf448860..3d9e5fa49451 100644 > > --- a/drivers/pci/pci-acpi.c > > +++ b/drivers/pci/pci-acpi.c > > @@ -1350,12 +1350,25 @@ static void pci_acpi_set_external_facing(struct pci_dev *dev) > > dev->external_facing = 1; > > } > > > > +static void pci_acpi_set_untrusted(struct pci_dev *dev) > > +{ > > + u8 val; > > + > > + if (device_property_read_u8(&dev->dev, "UntrustedDevice", &val)) > > + return; > > + > > + /* These PCI devices are not trustworthy */ > > + if (val) > > + dev->untrusted = 1; > > Should this all be replaced with: > > dev->untrusted = device_property_read_bool(&dev->dev, "UntrustedDevice"); > > ? Ack, yes, I will do this. > > Also, is this ACPI-specific? Why won't we need this for DT systems (or > do we already have this)?. Good point. Ack, Yes, I don't mind doing this for DT systems also. I wanted to get some feedback and acceptance within the PCI subsystem on the general idea of this property though. Bjorn? Thanks & Best Regards, Rajat > > Thanks, > Dmitry
[+cc Greg, Jean-Philippe, Mika, Pavel, Oliver, Joerg since they commented on previous "external-facing" discussion] On Wed, Jan 19, 2022 at 04:04:09PM -0800, Rajat Jain wrote: > Today the pci_dev->untrusted is set for any devices sitting downstream > an external facing port (determined via "ExternalFacingPort" property). > This however, disallows any internal devices to be marked as untrusted. This isn't stated quite accurately. "dev->untrusted" is currently set only by set_pcie_untrusted(), when "dev" has an upstream bridge that is either external-facing or untrusted. But that doesn't disallow or prevent internal devices from being marked as untrusted; it just doesn't implement that. > 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). > > This patch introduces a new "UntrustedDevice" property that can be used > by the firmware to mark any device as untrusted. I think I'm OK with this. Write this last sentence in imperative mood; see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/maintainer-tip.rst?id=v5.16#n134 for examples. > Signed-off-by: Rajat Jain <rajatja@google.com> > --- > drivers/pci/pci-acpi.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > index a42dbf448860..3d9e5fa49451 100644 > --- a/drivers/pci/pci-acpi.c > +++ b/drivers/pci/pci-acpi.c > @@ -1350,12 +1350,25 @@ static void pci_acpi_set_external_facing(struct pci_dev *dev) > dev->external_facing = 1; > } > > +static void pci_acpi_set_untrusted(struct pci_dev *dev) > +{ > + u8 val; > + > + if (device_property_read_u8(&dev->dev, "UntrustedDevice", &val)) > + return; > + > + /* These PCI devices are not trustworthy */ Comment is probably superfluous since the code seems obvious without it. > + if (val) > + dev->untrusted = 1; > +} > + > void pci_acpi_setup(struct device *dev, struct acpi_device *adev) > { > struct pci_dev *pci_dev = to_pci_dev(dev); > > pci_acpi_optimize_delay(pci_dev, adev->handle); > pci_acpi_set_external_facing(pci_dev); > + pci_acpi_set_untrusted(pci_dev); > pci_acpi_add_edr_notifier(pci_dev); > > pci_acpi_add_pm_notifier(adev, pci_dev); > -- > 2.34.1.703.g22d0c6ccf7-goog >
On Fri, Jan 21, 2022 at 03:41:17PM -0600, Bjorn Helgaas wrote: > [+cc Greg, Jean-Philippe, Mika, Pavel, Oliver, Joerg since they > commented on previous "external-facing" discussion] > > On Wed, Jan 19, 2022 at 04:04:09PM -0800, Rajat Jain wrote: > > Today the pci_dev->untrusted is set for any devices sitting downstream > > an external facing port (determined via "ExternalFacingPort" property). > > This however, disallows any internal devices to be marked as untrusted. > > This isn't stated quite accurately. "dev->untrusted" is currently set > only by set_pcie_untrusted(), when "dev" has an upstream bridge that > is either external-facing or untrusted. > > But that doesn't disallow or prevent internal devices from being > marked as untrusted; it just doesn't implement that. > > > 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). Who is making this policy decision? > > This patch introduces a new "UntrustedDevice" property that can be used > > by the firmware to mark any device as untrusted. Is this in the ACPI standard? If so, where? This notion of "trust" for PCI devices is crazy, as I have stated a number of times before. But at least you are not trying to say kernel code is trusted or not. thanks, greg k-h
Hi, On Fri, Jan 21, 2022 at 03:41:17PM -0600, Bjorn Helgaas wrote: > [+cc Greg, Jean-Philippe, Mika, Pavel, Oliver, Joerg since they > commented on previous "external-facing" discussion] > > On Wed, Jan 19, 2022 at 04:04:09PM -0800, Rajat Jain wrote: > > Today the pci_dev->untrusted is set for any devices sitting downstream > > an external facing port (determined via "ExternalFacingPort" property). > > This however, disallows any internal devices to be marked as untrusted. > > This isn't stated quite accurately. "dev->untrusted" is currently set > only by set_pcie_untrusted(), when "dev" has an upstream bridge that > is either external-facing or untrusted. > > But that doesn't disallow or prevent internal devices from being > marked as untrusted; it just doesn't implement that. > > > 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). > > > > This patch introduces a new "UntrustedDevice" property that can be used > > by the firmware to mark any device as untrusted. I think this new property should be documented somewhere too (also explain when to use it instead of ExternalFacingPort). If not in the next ACPI spec or some supplemental doc then perhaps in the DT bindings under Documentation/devicetree/bindings.
On Mon, Jan 24, 2022 at 08:27:17AM +0200, Mika Westerberg wrote: > > > This patch introduces a new "UntrustedDevice" property that can be used > > > by the firmware to mark any device as untrusted. > > I think this new property should be documented somewhere too (also > explain when to use it instead of ExternalFacingPort). If not in the > next ACPI spec or some supplemental doc then perhaps in the DT bindings > under Documentation/devicetree/bindings. Actually Microsoft has similar already: https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-internal-pcie-ports-accessible-to-users-and-requiring-dma-protection I think we should use that too here.
On Tue, Jan 25, 2022 at 12:58:52PM +0200, Mika Westerberg wrote: > On Mon, Jan 24, 2022 at 08:27:17AM +0200, Mika Westerberg wrote: > > > > This patch introduces a new "UntrustedDevice" property that can be used > > > > by the firmware to mark any device as untrusted. > > > > I think this new property should be documented somewhere too (also > > explain when to use it instead of ExternalFacingPort). If not in the > > next ACPI spec or some supplemental doc then perhaps in the DT bindings > > under Documentation/devicetree/bindings. > > Actually Microsoft has similar already: > > https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-internal-pcie-ports-accessible-to-users-and-requiring-dma-protection > > I think we should use that too here. But we do not have "dma protection" for Linux, so how will that value make sense? And shouldn't this be an ACPI standard? thanks, greg k-h
On Tue, Jan 25, 2022 at 12:15:02PM +0100, Greg Kroah-Hartman wrote: > On Tue, Jan 25, 2022 at 12:58:52PM +0200, Mika Westerberg wrote: > > On Mon, Jan 24, 2022 at 08:27:17AM +0200, Mika Westerberg wrote: > > > > > This patch introduces a new "UntrustedDevice" property that can be used > > > > > by the firmware to mark any device as untrusted. > > > > > > I think this new property should be documented somewhere too (also > > > explain when to use it instead of ExternalFacingPort). If not in the > > > next ACPI spec or some supplemental doc then perhaps in the DT bindings > > > under Documentation/devicetree/bindings. > > > > Actually Microsoft has similar already: > > > > https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-internal-pcie-ports-accessible-to-users-and-requiring-dma-protection > > > > I think we should use that too here. > > But we do not have "dma protection" for Linux, so how will that value > make sense? Yes I think we do - IOMMU. That's the same thing what we do now for "External Facing Ports". This one just is for internal ones. > And shouldn't this be an ACPI standard? Probably should or some supplemental doc but not sure how easy these "properties" can be added there to be honest. Some of these that we use in Linux too are from that same page: https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports Namely these: HotPlugSupportInD3, ExternalFacingPort, usb4-host-interface, usb4-port-number and StorageD3Enable.
On Tue, Jan 25, 2022 at 11:59 AM Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > > On Mon, Jan 24, 2022 at 08:27:17AM +0200, Mika Westerberg wrote: > > > > This patch introduces a new "UntrustedDevice" property that can be used > > > > by the firmware to mark any device as untrusted. > > > > I think this new property should be documented somewhere too (also > > explain when to use it instead of ExternalFacingPort). If not in the > > next ACPI spec or some supplemental doc then perhaps in the DT bindings > > under Documentation/devicetree/bindings. > > Actually Microsoft has similar already: > > https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-internal-pcie-ports-accessible-to-users-and-requiring-dma-protection > > I think we should use that too here. Agreed. This is what the platform firmware will need to use anyway for Windows compatibility and OEMs may not care about running Linux on their platforms.
On Tue, Jan 25, 2022 at 1:55 PM Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > > On Tue, Jan 25, 2022 at 12:15:02PM +0100, Greg Kroah-Hartman wrote: > > On Tue, Jan 25, 2022 at 12:58:52PM +0200, Mika Westerberg wrote: > > > On Mon, Jan 24, 2022 at 08:27:17AM +0200, Mika Westerberg wrote: > > > > > > This patch introduces a new "UntrustedDevice" property that can be used > > > > > > by the firmware to mark any device as untrusted. > > > > > > > > I think this new property should be documented somewhere too (also > > > > explain when to use it instead of ExternalFacingPort). If not in the > > > > next ACPI spec or some supplemental doc then perhaps in the DT bindings > > > > under Documentation/devicetree/bindings. > > > > > > Actually Microsoft has similar already: > > > > > > https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-internal-pcie-ports-accessible-to-users-and-requiring-dma-protection > > > > > > I think we should use that too here. > > > > But we do not have "dma protection" for Linux, so how will that value > > make sense? > > Yes I think we do - IOMMU. That's the same thing what we do now for > "External Facing Ports". This one just is for internal ones. > > > And shouldn't this be an ACPI standard? > > Probably should or some supplemental doc but not sure how easy these > "properties" can be added there to be honest. > > Some of these that we use in Linux too are from that same page: > > https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports > > Namely these: HotPlugSupportInD3, ExternalFacingPort, usb4-host-interface, > usb4-port-number and StorageD3Enable. Right. We are kind of on the receiving end here, because at the time we learn about these things the decisions to use them have been made already.
Hello Rafael, Bjorn, Mika, Dmitry, Greg, Thanks a lot for your comments. On Tue, Jan 25, 2022 at 6:45 AM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Tue, Jan 25, 2022 at 1:55 PM Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > > > On Tue, Jan 25, 2022 at 12:15:02PM +0100, Greg Kroah-Hartman wrote: > > > On Tue, Jan 25, 2022 at 12:58:52PM +0200, Mika Westerberg wrote: > > > > On Mon, Jan 24, 2022 at 08:27:17AM +0200, Mika Westerberg wrote: > > > > > > > This patch introduces a new "UntrustedDevice" property that can be used > > > > > > > by the firmware to mark any device as untrusted. > > > > > > > > > > I think this new property should be documented somewhere too (also > > > > > explain when to use it instead of ExternalFacingPort). If not in the > > > > > next ACPI spec or some supplemental doc then perhaps in the DT bindings > > > > > under Documentation/devicetree/bindings. > > > > > > > > Actually Microsoft has similar already: > > > > > > > > https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-internal-pcie-ports-accessible-to-users-and-requiring-dma-protection > > > > > > > > I think we should use that too here. But because this property also applies to a root port (only), it only helps if the device is downstream a PCIe root port. In our case, we have an internal (wifi) device 00:14.3 (sits on the internal PCI bus 0), so cannot use this. > > > > > > But we do not have "dma protection" for Linux, so how will that value > > > make sense? > > > > Yes I think we do - IOMMU. That's the same thing what we do now for > > "External Facing Ports". This one just is for internal ones. > > > > > And shouldn't this be an ACPI standard? > > > > Probably should or some supplemental doc but not sure how easy these > > "properties" can be added there to be honest. AIUI, the principal comment I have received here is that this property needs to be documented somewhere. I agree. Rafael, do you know if this new property can be added to the ACPI spec, and if so, how to do so? I'm happy to initiate a process if someone can point me to, I just hope that publishing a new property to the ACPI does not have to block this patch. The other option I was thinking of was to use the same property name (say "untrusted-device") for both ACPI and device tree platforms, and document it in Documentation/devicetree/bindings/pci/pci.txt along with others. Since there are other properties there that seem to be used similarly (Mika highlighted some below), perhaps that is an acceptable solution? I had one last question on the property name itself. I was trying to understand why a property might have 2 names i.e. "external-facing" for DT and "ExternalFacingPort" in ACPI? Are there any naming convention requirements that require ACPI and DT property names to be different? Is "untrusted-device" an acceptable ACPI property name? Thanks & Best Regards, Rajat > > > > Some of these that we use in Linux too are from that same page: > > > > https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports > > > > Namely these: HotPlugSupportInD3, ExternalFacingPort, usb4-host-interface, > > usb4-port-number and StorageD3Enable. > > Right. > > We are kind of on the receiving end here, because at the time we learn > about these things the decisions to use them have been made already.
Hi Dmitry, On Wed, Jan 19, 2022 at 6:25 PM Dmitry Torokhov <dtor@google.com> wrote: > > Hi Rajat, > > On Wed, Jan 19, 2022 at 4:04 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" property). > > This however, disallows any 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). > > > > This patch introduces 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> > > --- > > drivers/pci/pci-acpi.c | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > > index a42dbf448860..3d9e5fa49451 100644 > > --- a/drivers/pci/pci-acpi.c > > +++ b/drivers/pci/pci-acpi.c > > @@ -1350,12 +1350,25 @@ static void pci_acpi_set_external_facing(struct pci_dev *dev) > > dev->external_facing = 1; > > } > > > > +static void pci_acpi_set_untrusted(struct pci_dev *dev) > > +{ > > + u8 val; > > + > > + if (device_property_read_u8(&dev->dev, "UntrustedDevice", &val)) > > + return; > > + > > + /* These PCI devices are not trustworthy */ > > + if (val) > > + dev->untrusted = 1; > > Should this all be replaced with: > > dev->untrusted = device_property_read_bool(&dev->dev, "UntrustedDevice"); The device_property_read_bool() seems to be merely checking for property presence (and ignoring its value). I checked with our BIOS / ACPI team. Per them, the ACPI properties always have a value associated with them. So if I switch to device_property_read_bool(), "UntrustedDevice" property with a value of "0" in ACPI shall be marked as an untrusted device by the kernel. I understand that this may be a confusing corner case of bad ACPI, but I was thinking it may be better to use the ACPI value also in the kernel to decide. Thus I think the use of device_property_read_u8() (the current code) may be better. WDYT? Thanks & Best Regards, Rajat > > ? > > Also, is this ACPI-specific? Why won't we need this for DT systems (or > do we already have this)?. > > Thanks, > Dmitry
Hi, On Thu, Jan 27, 2022 at 02:26:07PM -0800, Rajat Jain wrote: > Hello Rafael, Bjorn, Mika, Dmitry, Greg, > > Thanks a lot for your comments. > > On Tue, Jan 25, 2022 at 6:45 AM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > On Tue, Jan 25, 2022 at 1:55 PM Mika Westerberg > > <mika.westerberg@linux.intel.com> wrote: > > > > > > On Tue, Jan 25, 2022 at 12:15:02PM +0100, Greg Kroah-Hartman wrote: > > > > On Tue, Jan 25, 2022 at 12:58:52PM +0200, Mika Westerberg wrote: > > > > > On Mon, Jan 24, 2022 at 08:27:17AM +0200, Mika Westerberg wrote: > > > > > > > > This patch introduces a new "UntrustedDevice" property that can be used > > > > > > > > by the firmware to mark any device as untrusted. > > > > > > > > > > > > I think this new property should be documented somewhere too (also > > > > > > explain when to use it instead of ExternalFacingPort). If not in the > > > > > > next ACPI spec or some supplemental doc then perhaps in the DT bindings > > > > > > under Documentation/devicetree/bindings. > > > > > > > > > > Actually Microsoft has similar already: > > > > > > > > > > https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-internal-pcie-ports-accessible-to-users-and-requiring-dma-protection > > > > > > > > > > I think we should use that too here. > > But because this property also applies to a root port (only), it only > helps if the device is downstream a PCIe root port. In our case, we > have an internal (wifi) device 00:14.3 (sits on the internal PCI bus > 0), so cannot use this. Right. I wonder if we can expand it to cover all internal devices, not just PCIe root ports? We anyways need to support that property so does not make much sense to me to invent yet another that does pretty much the same thing.
On Thu, Jan 27, 2022 at 02:26:07PM -0800, Rajat Jain wrote: > > > > And shouldn't this be an ACPI standard? > > > > > > Probably should or some supplemental doc but not sure how easy these > > > "properties" can be added there to be honest. > > AIUI, the principal comment I have received here is that this property > needs to be documented somewhere. I agree. > > Rafael, do you know if this new property can be added to the ACPI > spec, and if so, how to do so? I'm happy to initiate a process if > someone can point me to, I just hope that publishing a new property to > the ACPI does not have to block this patch. > > The other option I was thinking of was to use the same property name > (say "untrusted-device") for both ACPI and device tree platforms, and > document it in Documentation/devicetree/bindings/pci/pci.txt along > with others. Since there are other properties there that seem to be > used similarly (Mika highlighted some below), perhaps that is an > acceptable solution? > > I had one last question on the property name itself. I was trying to > understand why a property might have 2 names i.e. "external-facing" > for DT and "ExternalFacingPort" in ACPI? I picked "external-facing" for DT to be consistent with other device tree property names. There doesn't seem to be any CamelCase in device trees [1], so we should probably keep that convention for new properties as well. The internal device_property could use the DT name and the ACPI name can be different. We do something similar with properties "pasid-num-bits" and "dma-can-stall" which are extracted from the IORT table in iort_named_component_init() Thanks, Jean [1] git grep "\<[A-Z][,a-zA-Z0-9]\+ =" -- '*.dts' > Are there any naming > convention requirements that require ACPI and DT property names to be > different? Is "untrusted-device" an acceptable ACPI property name? > > Thanks & Best Regards, > > Rajat
Hi Mika, All, On Thu, Jan 27, 2022 at 11:49 PM Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > > Hi, > > On Thu, Jan 27, 2022 at 02:26:07PM -0800, Rajat Jain wrote: > > Hello Rafael, Bjorn, Mika, Dmitry, Greg, > > > > Thanks a lot for your comments. > > > > On Tue, Jan 25, 2022 at 6:45 AM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > On Tue, Jan 25, 2022 at 1:55 PM Mika Westerberg > > > <mika.westerberg@linux.intel.com> wrote: > > > > > > > > On Tue, Jan 25, 2022 at 12:15:02PM +0100, Greg Kroah-Hartman wrote: > > > > > On Tue, Jan 25, 2022 at 12:58:52PM +0200, Mika Westerberg wrote: > > > > > > On Mon, Jan 24, 2022 at 08:27:17AM +0200, Mika Westerberg wrote: > > > > > > > > > This patch introduces a new "UntrustedDevice" property that can be used > > > > > > > > > by the firmware to mark any device as untrusted. > > > > > > > > > > > > > > I think this new property should be documented somewhere too (also > > > > > > > explain when to use it instead of ExternalFacingPort). If not in the > > > > > > > next ACPI spec or some supplemental doc then perhaps in the DT bindings > > > > > > > under Documentation/devicetree/bindings. > > > > > > > > > > > > Actually Microsoft has similar already: > > > > > > > > > > > > https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-internal-pcie-ports-accessible-to-users-and-requiring-dma-protection > > > > > > > > > > > > I think we should use that too here. > > > > But because this property also applies to a root port (only), it only > > helps if the device is downstream a PCIe root port. In our case, we > > have an internal (wifi) device 00:14.3 (sits on the internal PCI bus > > 0), so cannot use this. > > Right. I wonder if we can expand it to cover all internal devices, not > just PCIe root ports? We anyways need to support that property so does > not make much sense to me to invent yet another that does pretty much > the same thing. I'm open to doing so if the others also feel the same way. IMHO though, the semantics of ACPI "DmaProperty" differ from the semantics of the property I'm proposing here. The current (documented) semantics (of "DmaProperty"): *This device (root port) is trusted*, but any devices downstream are not to be trusted. What I need and am proposing (new "UntrustedDevice"): *This device as well as any downstream devices* are untrusted. Note that there may be firmware implementing "DmaProperty" already out there (for windows), and if we decide to use it for my purposes, then there shall be a discrepancy in how Linux uses that property vs Windows. Is that acceptable? Thanks & Best Regards, Rajat
On Fri, Jan 28, 2022 at 10:34 PM Rajat Jain <rajatja@google.com> wrote: > > Hi Mika, All, > > On Thu, Jan 27, 2022 at 11:49 PM Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > > > Hi, > > > > On Thu, Jan 27, 2022 at 02:26:07PM -0800, Rajat Jain wrote: > > > Hello Rafael, Bjorn, Mika, Dmitry, Greg, > > > > > > Thanks a lot for your comments. > > > > > > On Tue, Jan 25, 2022 at 6:45 AM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > > > On Tue, Jan 25, 2022 at 1:55 PM Mika Westerberg > > > > <mika.westerberg@linux.intel.com> wrote: > > > > > > > > > > On Tue, Jan 25, 2022 at 12:15:02PM +0100, Greg Kroah-Hartman wrote: > > > > > > On Tue, Jan 25, 2022 at 12:58:52PM +0200, Mika Westerberg wrote: > > > > > > > On Mon, Jan 24, 2022 at 08:27:17AM +0200, Mika Westerberg wrote: > > > > > > > > > > This patch introduces a new "UntrustedDevice" property that can be used > > > > > > > > > > by the firmware to mark any device as untrusted. > > > > > > > > > > > > > > > > I think this new property should be documented somewhere too (also > > > > > > > > explain when to use it instead of ExternalFacingPort). If not in the > > > > > > > > next ACPI spec or some supplemental doc then perhaps in the DT bindings > > > > > > > > under Documentation/devicetree/bindings. > > > > > > > > > > > > > > Actually Microsoft has similar already: > > > > > > > > > > > > > > https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-internal-pcie-ports-accessible-to-users-and-requiring-dma-protection > > > > > > > > > > > > > > I think we should use that too here. > > > > > > But because this property also applies to a root port (only), it only > > > helps if the device is downstream a PCIe root port. In our case, we > > > have an internal (wifi) device 00:14.3 (sits on the internal PCI bus > > > 0), so cannot use this. > > > > Right. I wonder if we can expand it to cover all internal devices, not > > just PCIe root ports? We anyways need to support that property so does > > not make much sense to me to invent yet another that does pretty much > > the same thing. > > I'm open to doing so if the others also feel the same way. IMHO > though, the semantics of ACPI "DmaProperty" differ from the semantics > of the property I'm proposing here. > > The current (documented) semantics (of "DmaProperty"): *This device > (root port) is trusted*, but any devices downstream are not to be > trusted. > > What I need and am proposing (new "UntrustedDevice"): *This device as > well as any downstream devices* are untrusted. > > Note that there may be firmware implementing "DmaProperty" already out > there (for windows), and if we decide to use it for my purposes, then > there shall be a discrepancy in how Linux uses that property vs > Windows. Is that acceptable? It may be confusing, so I'd rather not do that. The platform firmware will use it with the Windows use case in mind and if it has side effects in Linux, problems are likely to appear in the field. So the question is rather not about it being acceptable, but about whether or not this is generally going to work.
Hi, On Sun, Jan 30, 2022 at 03:30:39PM +0100, Rafael J. Wysocki wrote: > > I'm open to doing so if the others also feel the same way. IMHO > > though, the semantics of ACPI "DmaProperty" differ from the semantics > > of the property I'm proposing here. > > > > The current (documented) semantics (of "DmaProperty"): *This device > > (root port) is trusted*, but any devices downstream are not to be > > trusted. > > > > What I need and am proposing (new "UntrustedDevice"): *This device as > > well as any downstream devices* are untrusted. > > > > Note that there may be firmware implementing "DmaProperty" already out > > there (for windows), and if we decide to use it for my purposes, then > > there shall be a discrepancy in how Linux uses that property vs > > Windows. Is that acceptable? > > It may be confusing, so I'd rather not do that. > > The platform firmware will use it with the Windows use case in mind > and if it has side effects in Linux, problems are likely to appear in > the field. > > So the question is rather not about it being acceptable, but about > whether or not this is generally going to work. I was kind of implying that we could perhaps contact Microsoft and ask them if the wording could be changed to cover all the devices, not just PCIe root ports. I think this is something they will also need for things like internal WI-FI controllers. If that's not possible then no objections adding "UntrustedDevice". We just need to deal with the "DmaProperty" anyway and both end up setting pdev->untrusted in the similar manner.
Hello Mika, Rafael, On Sun, Jan 30, 2022 at 10:42 PM Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > > Hi, > > On Sun, Jan 30, 2022 at 03:30:39PM +0100, Rafael J. Wysocki wrote: > > > I'm open to doing so if the others also feel the same way. IMHO > > > though, the semantics of ACPI "DmaProperty" differ from the semantics > > > of the property I'm proposing here. > > > > > > The current (documented) semantics (of "DmaProperty"): *This device > > > (root port) is trusted*, but any devices downstream are not to be > > > trusted. > > > > > > What I need and am proposing (new "UntrustedDevice"): *This device as > > > well as any downstream devices* are untrusted. > > > > > > Note that there may be firmware implementing "DmaProperty" already out > > > there (for windows), and if we decide to use it for my purposes, then > > > there shall be a discrepancy in how Linux uses that property vs > > > Windows. Is that acceptable? > > > > It may be confusing, so I'd rather not do that. > > > > The platform firmware will use it with the Windows use case in mind > > and if it has side effects in Linux, problems are likely to appear in > > the field. > > > > So the question is rather not about it being acceptable, but about > > whether or not this is generally going to work. > > I was kind of implying that we could perhaps contact Microsoft and ask > them if the wording could be changed to cover all the devices, not just > PCIe root ports. I think this is something they will also need for > things like internal WI-FI controllers. We (Chromeos) do not have a contact at Microsoft, not sure if Intel does. If someone can point me to a contact I will be happy to initiate a conversation. However, given that they have already published it, and changing the semantics might mean they will also have to change windows implementation. Not sure if we have enough leverage with Microsoft here, so I wouldn't have any high hopes though. Like Rafael said, we're on the receiving end here. Rafael, one last question: is "untrusted-device" an acceptable ACPI property name, or does it have to be Camel case? Thanks & Best Regards, Rajat > > If that's not possible then no objections adding "UntrustedDevice". We > just need to deal with the "DmaProperty" anyway and both end up setting > pdev->untrusted in the similar manner.
On Mon, Jan 31, 2022 at 11:57 AM Rajat Jain <rajatja@google.com> wrote: > > Hello Mika, Rafael, > > On Sun, Jan 30, 2022 at 10:42 PM Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > > > Hi, > > > > On Sun, Jan 30, 2022 at 03:30:39PM +0100, Rafael J. Wysocki wrote: > > > > I'm open to doing so if the others also feel the same way. IMHO > > > > though, the semantics of ACPI "DmaProperty" differ from the semantics > > > > of the property I'm proposing here. > > > > > > > > The current (documented) semantics (of "DmaProperty"): *This device > > > > (root port) is trusted*, but any devices downstream are not to be > > > > trusted. > > > > > > > > What I need and am proposing (new "UntrustedDevice"): *This device as > > > > well as any downstream devices* are untrusted. > > > > > > > > Note that there may be firmware implementing "DmaProperty" already out > > > > there (for windows), and if we decide to use it for my purposes, then > > > > there shall be a discrepancy in how Linux uses that property vs > > > > Windows. Is that acceptable? > > > > > > It may be confusing, so I'd rather not do that. > > > > > > The platform firmware will use it with the Windows use case in mind > > > and if it has side effects in Linux, problems are likely to appear in > > > the field. > > > > > > So the question is rather not about it being acceptable, but about > > > whether or not this is generally going to work. > > > > I was kind of implying that we could perhaps contact Microsoft and ask > > them if the wording could be changed to cover all the devices, not just > > PCIe root ports. I think this is something they will also need for > > things like internal WI-FI controllers. > > We (Chromeos) do not have a contact at Microsoft, not sure if Intel > does. If someone can point me to a contact I will be happy to initiate > a conversation. However, given that they have already published it, > and changing the semantics might mean they will also have to change > windows implementation. Not sure if we have enough leverage with > Microsoft here, so I wouldn't have any high hopes though. To keep everyone updated, Mika has helped me initiate a conversation with Microsoft on this (Thanks a lot Mika!). We're still waiting to hear their feedback. Until then, I've posted a v2 for review at: https://patchwork.kernel.org/project/linux-pci/patch/20220202020103.2149130-1-rajatja@google.com/ If we can reach an agreement with Microsoft, I can change the property name in the patch (to "DmaProperty"), but would appreciate review of any other aspects of v2 in the meantime. Thanks & Best Regards, Rajat > Like Rafael > said, we're on the receiving end here. > > Rafael, one last question: is "untrusted-device" an acceptable ACPI > property name, or does it have to be Camel case? > > Thanks & Best Regards, > > Rajat > > > > > If that's not possible then no objections adding "UntrustedDevice". We > > just need to deal with the "DmaProperty" anyway and both end up setting > > pdev->untrusted in the similar manner.
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c index a42dbf448860..3d9e5fa49451 100644 --- a/drivers/pci/pci-acpi.c +++ b/drivers/pci/pci-acpi.c @@ -1350,12 +1350,25 @@ static void pci_acpi_set_external_facing(struct pci_dev *dev) dev->external_facing = 1; } +static void pci_acpi_set_untrusted(struct pci_dev *dev) +{ + u8 val; + + if (device_property_read_u8(&dev->dev, "UntrustedDevice", &val)) + return; + + /* These PCI devices are not trustworthy */ + if (val) + dev->untrusted = 1; +} + void pci_acpi_setup(struct device *dev, struct acpi_device *adev) { struct pci_dev *pci_dev = to_pci_dev(dev); pci_acpi_optimize_delay(pci_dev, adev->handle); pci_acpi_set_external_facing(pci_dev); + pci_acpi_set_untrusted(pci_dev); pci_acpi_add_edr_notifier(pci_dev); pci_acpi_add_pm_notifier(adev, pci_dev);
Today the pci_dev->untrusted is set for any devices sitting downstream an external facing port (determined via "ExternalFacingPort" property). This however, disallows any 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). This patch introduces 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> --- drivers/pci/pci-acpi.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)