[PATCH] PCI: ACPI: Allow internal devices to be marked as untrusted

[PATCH] PCI: ACPI: Allow internal devices to be marked as untrusted

[Rajat Jain]

Today the pci_dev->untrusted is set for any devices sitting downstream
an external facing port (determined via "ExternalFacingPort" 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;
+}
+
 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

Re: [PATCH] PCI: ACPI: Allow internal devices to be marked as untrusted

[Dmitry Torokhov]

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

Re: [PATCH] PCI: ACPI: Allow internal devices to be marked as untrusted

[Rajat Jain]

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

Re: [PATCH] PCI: ACPI: Allow internal devices to be marked as untrusted

[Rajat Jain]

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

Re: [PATCH] PCI: ACPI: Allow internal devices to be marked as untrusted

[Bjorn Helgaas]

[+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
> 

Re: [PATCH] PCI: ACPI: Allow internal devices to be marked as untrusted

[Greg Kroah-Hartman]

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

Re: [PATCH] PCI: ACPI: Allow internal devices to be marked as untrusted

[Mika Westerberg]

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.

Re: [PATCH] PCI: ACPI: Allow internal devices to be marked as untrusted

[Mika Westerberg]

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.

Re: [PATCH] PCI: ACPI: Allow internal devices to be marked as untrusted

[Greg Kroah-Hartman]

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

Re: [PATCH] PCI: ACPI: Allow internal devices to be marked as untrusted

[Mika Westerberg]

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.

Re: [PATCH] PCI: ACPI: Allow internal devices to be marked as untrusted

[Rafael J. Wysocki]

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.

Re: [PATCH] PCI: ACPI: Allow internal devices to be marked as untrusted

[Rajat Jain]

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.

Re: [PATCH] PCI: ACPI: Allow internal devices to be marked as untrusted

[Mika Westerberg]

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.

Re: [PATCH] PCI: ACPI: Allow internal devices to be marked as untrusted

[Rajat Jain]

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

Re: [PATCH] PCI: ACPI: Allow internal devices to be marked as untrusted

[Rafael J. Wysocki]

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.

Re: [PATCH] PCI: ACPI: Allow internal devices to be marked as untrusted

[Mika Westerberg]

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.

Re: [PATCH] PCI: ACPI: Allow internal devices to be marked as untrusted

[Rajat Jain]

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.

Re: [PATCH] PCI: ACPI: Allow internal devices to be marked as untrusted

[Rajat Jain]

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.

Re: [PATCH] PCI: ACPI: Allow internal devices to be marked as untrusted

[Jean-Philippe Brucker]

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

Re: [PATCH] PCI: ACPI: Allow internal devices to be marked as untrusted

[Rafael J. Wysocki]

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.