[PATCH 1/4] pci: Keep the ACS capability offset in device

[PATCH 1/4] pci: Keep the ACS capability offset in device

[Rajat Jain]

Currently this is being looked up at a number of places. Read
and store it once at bootup so that it can be used by all later.

Signed-off-by: Rajat Jain <rajatja@google.com>
---
 drivers/pci/p2pdma.c |  2 +-
 drivers/pci/pci.c    | 21 +++++++++++++++++----
 drivers/pci/pci.h    |  2 +-
 drivers/pci/probe.c  |  2 +-
 drivers/pci/quirks.c |  8 ++++----
 include/linux/pci.h  |  1 +
 6 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
index e8e444eeb1cd2..f29a48f8fa594 100644
--- a/drivers/pci/p2pdma.c
+++ b/drivers/pci/p2pdma.c
@@ -253,7 +253,7 @@ static int pci_bridge_has_acs_redir(struct pci_dev *pdev)
 	int pos;
 	u16 ctrl;
 
-	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ACS);
+	pos = pdev->acs_cap;
 	if (!pos)
 		return 0;
 
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index ce096272f52b1..d2ff987585855 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -51,6 +51,7 @@ EXPORT_SYMBOL(pci_pci_problems);
 
 unsigned int pci_pm_d3_delay;
 
+static void pci_enable_acs(struct pci_dev *dev);
 static void pci_pme_list_scan(struct work_struct *work);
 
 static LIST_HEAD(pci_pme_list);
@@ -3284,7 +3285,7 @@ static void pci_disable_acs_redir(struct pci_dev *dev)
 	if (!pci_dev_specific_disable_acs_redir(dev))
 		return;
 
-	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS);
+	pos = dev->acs_cap;
 	if (!pos) {
 		pci_warn(dev, "cannot disable ACS redirect for this hardware as it does not have ACS capabilities\n");
 		return;
@@ -3310,7 +3311,7 @@ static void pci_std_enable_acs(struct pci_dev *dev)
 	u16 cap;
 	u16 ctrl;
 
-	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS);
+	pos = dev->acs_cap;
 	if (!pos)
 		return;
 
@@ -3336,7 +3337,7 @@ static void pci_std_enable_acs(struct pci_dev *dev)
  * pci_enable_acs - enable ACS if hardware support it
  * @dev: the PCI device
  */
-void pci_enable_acs(struct pci_dev *dev)
+static void pci_enable_acs(struct pci_dev *dev)
 {
 	if (!pci_acs_enable)
 		goto disable_acs_redir;
@@ -3362,7 +3363,7 @@ static bool pci_acs_flags_enabled(struct pci_dev *pdev, u16 acs_flags)
 	int pos;
 	u16 cap, ctrl;
 
-	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ACS);
+	pos = pdev->acs_cap;
 	if (!pos)
 		return false;
 
@@ -3487,6 +3488,18 @@ bool pci_acs_path_enabled(struct pci_dev *start,
 	return true;
 }
 
+/**
+ * pci_acs_init - Initialize if hardware supports it
+ * @dev: the PCI device
+ */
+void pci_acs_init(struct pci_dev *dev)
+{
+	dev->acs_cap = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS);
+
+	if (dev->acs_cap)
+		pci_enable_acs(dev);
+}
+
 /**
  * pci_rebar_find_pos - find position of resize ctrl reg for BAR
  * @pdev: PCI device
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 6d3f758671064..12fb79fbe29d3 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -532,7 +532,7 @@ static inline resource_size_t pci_resource_alignment(struct pci_dev *dev,
 	return resource_alignment(res);
 }
 
-void pci_enable_acs(struct pci_dev *dev);
+void pci_acs_init(struct pci_dev *dev);
 #ifdef CONFIG_PCI_QUIRKS
 int pci_dev_specific_acs_enabled(struct pci_dev *dev, u16 acs_flags);
 int pci_dev_specific_enable_acs(struct pci_dev *dev);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 2f66988cea257..6d87066a5ecc5 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2390,7 +2390,7 @@ static void pci_init_capabilities(struct pci_dev *dev)
 	pci_ats_init(dev);		/* Address Translation Services */
 	pci_pri_init(dev);		/* Page Request Interface */
 	pci_pasid_init(dev);		/* Process Address Space ID */
-	pci_enable_acs(dev);		/* Enable ACS P2P upstream forwarding */
+	pci_acs_init(dev);		/* Access Control Services */
 	pci_ptm_init(dev);		/* Precision Time Measurement */
 	pci_aer_init(dev);		/* Advanced Error Reporting */
 	pci_dpc_init(dev);		/* Downstream Port Containment */
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 812bfc32ecb82..b341628e47527 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -4653,7 +4653,7 @@ static int pci_quirk_intel_spt_pch_acs(struct pci_dev *dev, u16 acs_flags)
 	if (!pci_quirk_intel_spt_pch_acs_match(dev))
 		return -ENOTTY;
 
-	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS);
+	pos = dev->acs_cap;
 	if (!pos)
 		return -ENOTTY;
 
@@ -4961,7 +4961,7 @@ static int pci_quirk_enable_intel_spt_pch_acs(struct pci_dev *dev)
 	if (!pci_quirk_intel_spt_pch_acs_match(dev))
 		return -ENOTTY;
 
-	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS);
+	pos = dev->acs_cap;
 	if (!pos)
 		return -ENOTTY;
 
@@ -4988,7 +4988,7 @@ static int pci_quirk_disable_intel_spt_pch_acs_redir(struct pci_dev *dev)
 	if (!pci_quirk_intel_spt_pch_acs_match(dev))
 		return -ENOTTY;
 
-	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS);
+	pos = dev->acs_cap;
 	if (!pos)
 		return -ENOTTY;
 
@@ -5355,7 +5355,7 @@ int pci_idt_bus_quirk(struct pci_bus *bus, int devfn, u32 *l, int timeout)
 	bool found;
 	struct pci_dev *bridge = bus->self;
 
-	pos = pci_find_ext_capability(bridge, PCI_EXT_CAP_ID_ACS);
+	pos = bridge->acs_cap;
 
 	/* Disable ACS SV before initial config reads */
 	if (pos) {
diff --git a/include/linux/pci.h b/include/linux/pci.h
index c79d83304e529..a26be5332bba6 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -486,6 +486,7 @@ struct pci_dev {
 #ifdef CONFIG_PCI_P2PDMA
 	struct pci_p2pdma *p2pdma;
 #endif
+	u16		acs_cap;	/* ACS Capability offset */
 	phys_addr_t	rom;		/* Physical address if not from BAR */
 	size_t		romlen;		/* Length if not from BAR */
 	char		*driver_override; /* Driver name to force a match */
-- 
2.27.0.290.gba653c62da-goog

[PATCH 2/4] pci: set "untrusted" flag for truly external devices only

[Rajat Jain]

The "ExternalFacing" devices (root ports) are still internal devices
that sit on the internal system fabric and thus trusted. Currently they
were being marked untrusted - likely as an unintended border case.

This patch uses the platform flag to identify the external facing devices
and then use it to mark any downstream devices as "untrusted". The
external-facing devices themselves are left as "trusted". This was
discussed here: https://lkml.org/lkml/2020/6/10/1049

Signed-off-by: Rajat Jain <rajatja@google.com>
---
 drivers/iommu/intel/iommu.c |  2 +-
 drivers/pci/of.c            |  2 +-
 drivers/pci/pci-acpi.c      | 13 +++++++------
 drivers/pci/probe.c         |  2 +-
 include/linux/pci.h         |  8 ++++++++
 5 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 9129663a7406b..1256ca89fb519 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4735,7 +4735,7 @@ static inline bool has_untrusted_dev(void)
 	struct pci_dev *pdev = NULL;
 
 	for_each_pci_dev(pdev)
-		if (pdev->untrusted)
+		if (pdev->untrusted || pdev->external_facing)
 			return true;
 
 	return false;
diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index 27839cd2459f6..22727fc9558df 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -42,7 +42,7 @@ void pci_set_bus_of_node(struct pci_bus *bus)
 	} else {
 		node = of_node_get(bus->self->dev.of_node);
 		if (node && of_property_read_bool(node, "external-facing"))
-			bus->self->untrusted = true;
+			bus->self->external_facing = true;
 	}
 
 	bus->dev.of_node = node;
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 7224b1e5f2a83..492c07805caf8 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -1213,22 +1213,23 @@ static void pci_acpi_optimize_delay(struct pci_dev *pdev,
 	ACPI_FREE(obj);
 }
 
-static void pci_acpi_set_untrusted(struct pci_dev *dev)
+static void pci_acpi_set_external_facing(struct pci_dev *dev)
 {
 	u8 val;
 
-	if (pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT)
+	if (pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT &&
+	    pci_pcie_type(dev) != PCI_EXP_TYPE_DOWNSTREAM)
 		return;
 	if (device_property_read_u8(&dev->dev, "ExternalFacingPort", &val))
 		return;
 
 	/*
-	 * These root ports expose PCIe (including DMA) outside of the
-	 * system so make sure we treat them and everything behind as
+	 * These root/down ports expose PCIe (including DMA) outside of the
+	 * system so make sure we treat everything behind them as
 	 * untrusted.
 	 */
 	if (val)
-		dev->untrusted = 1;
+		dev->external_facing = 1;
 }
 
 static void pci_acpi_setup(struct device *dev)
@@ -1240,7 +1241,7 @@ static void pci_acpi_setup(struct device *dev)
 		return;
 
 	pci_acpi_optimize_delay(pci_dev, adev->handle);
-	pci_acpi_set_untrusted(pci_dev);
+	pci_acpi_set_external_facing(pci_dev);
 	pci_acpi_add_edr_notifier(pci_dev);
 
 	pci_acpi_add_pm_notifier(adev, pci_dev);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 6d87066a5ecc5..8c40c00413e74 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1552,7 +1552,7 @@ static void set_pcie_untrusted(struct pci_dev *dev)
 	 * untrusted as well.
 	 */
 	parent = pci_upstream_bridge(dev);
-	if (parent && parent->untrusted)
+	if (parent && (parent->untrusted || parent->external_facing))
 		dev->untrusted = true;
 }
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index a26be5332bba6..fe1bc603fda40 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -432,6 +432,14 @@ struct pci_dev {
 	 * mappings to make sure they cannot access arbitrary memory.
 	 */
 	unsigned int	untrusted:1;
+	/*
+	 * Devices are marked as external-facing using info from platform
+	 * (ACPI / devicetree). An external-facing device is still an internal
+	 * trusted device, but it faces external untrusted devices. Thus any
+	 * devices enumerated downstream an external-facing device is marked
+	 * as untrusted.
+	 */
+	unsigned int	external_facing:1;
 	unsigned int	broken_intx_masking:1;	/* INTx masking can't be used */
 	unsigned int	io_window_1k:1;		/* Intel bridge 1K I/O windows */
 	unsigned int	irq_managed:1;
-- 
2.27.0.290.gba653c62da-goog

[PATCH 3/4] pci: acs: Enable PCI_ACS_TB for untrusted/external-facing devices

[Rajat Jain]

When enabling ACS, currently the bit "translation blocking" was
not getting changed at all. Set it to disable translation blocking
too for all external facing or untrusted devices. This is OK
because ATS is only allowed on internal devces.

Signed-off-by: Rajat Jain <rajatja@google.com>
---
 drivers/pci/pci.c    |  4 ++++
 drivers/pci/quirks.c | 11 +++++++++++
 2 files changed, 15 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index d2ff987585855..79853b52658a2 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3330,6 +3330,10 @@ static void pci_std_enable_acs(struct pci_dev *dev)
 	/* Upstream Forwarding */
 	ctrl |= (cap & PCI_ACS_UF);
 
+	if (dev->external_facing || dev->untrusted)
+		/* Translation Blocking */
+		ctrl |= (cap & PCI_ACS_TB);
+
 	pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
 }
 
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index b341628e47527..6294adeac4049 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -4934,6 +4934,13 @@ static void pci_quirk_enable_intel_rp_mpc_acs(struct pci_dev *dev)
 	}
 }
 
+/*
+ * Currently this quirk does the equivalent of
+ * PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_SV
+ *
+ * Currently missing, it also needs to do equivalent of PCI_ACS_TB,
+ * if dev->external_facing || dev->untrusted
+ */
 static int pci_quirk_enable_intel_pch_acs(struct pci_dev *dev)
 {
 	if (!pci_quirk_intel_pch_acs_match(dev))
@@ -4973,6 +4980,10 @@ static int pci_quirk_enable_intel_spt_pch_acs(struct pci_dev *dev)
 	ctrl |= (cap & PCI_ACS_CR);
 	ctrl |= (cap & PCI_ACS_UF);
 
+	if (dev->external_facing || dev->untrusted)
+		/* Translation Blocking */
+		ctrl |= (cap & PCI_ACS_TB);
+
 	pci_write_config_dword(dev, pos + INTEL_SPT_ACS_CTRL, ctrl);
 
 	pci_info(dev, "Intel SPT PCH root port ACS workaround enabled\n");
-- 
2.27.0.290.gba653c62da-goog

[PATCH 4/4] pci: export untrusted attribute in sysfs

[Rajat Jain]

This is needed to allow the userspace to determine when an untrusted
device has been added, and thus allowing it to bind the driver manually
to it, if it so wishes. This is being done as part of the approach
discussed at https://lkml.org/lkml/2020/6/9/1331

Signed-off-by: Rajat Jain <rajatja@google.com>
---
 drivers/pci/pci-sysfs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 6d78df981d41a..574e9c613ba26 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -50,6 +50,7 @@ pci_config_attr(subsystem_device, "0x%04x\n");
 pci_config_attr(revision, "0x%02x\n");
 pci_config_attr(class, "0x%06x\n");
 pci_config_attr(irq, "%u\n");
+pci_config_attr(untrusted, "%u\n");
 
 static ssize_t broken_parity_status_show(struct device *dev,
 					 struct device_attribute *attr,
@@ -608,6 +609,7 @@ static struct attribute *pci_dev_attrs[] = {
 #endif
 	&dev_attr_driver_override.attr,
 	&dev_attr_ari_enabled.attr,
+	&dev_attr_untrusted.attr,
 	NULL,
 };
 
-- 
2.27.0.290.gba653c62da-goog

Re: [PATCH 4/4] pci: export untrusted attribute in sysfs

[Greg Kroah-Hartman]

On Mon, Jun 15, 2020 at 06:17:42PM -0700, Rajat Jain wrote:
> This is needed to allow the userspace to determine when an untrusted
> device has been added, and thus allowing it to bind the driver manually
> to it, if it so wishes. This is being done as part of the approach
> discussed at https://lkml.org/lkml/2020/6/9/1331
> 
> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---
>  drivers/pci/pci-sysfs.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 6d78df981d41a..574e9c613ba26 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -50,6 +50,7 @@ pci_config_attr(subsystem_device, "0x%04x\n");
>  pci_config_attr(revision, "0x%02x\n");
>  pci_config_attr(class, "0x%06x\n");
>  pci_config_attr(irq, "%u\n");
> +pci_config_attr(untrusted, "%u\n");
>  
>  static ssize_t broken_parity_status_show(struct device *dev,
>  					 struct device_attribute *attr,
> @@ -608,6 +609,7 @@ static struct attribute *pci_dev_attrs[] = {
>  #endif
>  	&dev_attr_driver_override.attr,
>  	&dev_attr_ari_enabled.attr,
> +	&dev_attr_untrusted.attr,
>  	NULL,
>  };

You also need a Documentation/ABI/ update for this new file.

thanks,

greg k-h

Re: [PATCH 4/4] pci: export untrusted attribute in sysfs

[Christoph Hellwig]

On Mon, Jun 15, 2020 at 06:17:42PM -0700, Rajat Jain via iommu wrote:
> This is needed to allow the userspace to determine when an untrusted
> device has been added, and thus allowing it to bind the driver manually
> to it, if it so wishes. This is being done as part of the approach
> discussed at https://lkml.org/lkml/2020/6/9/1331

Please move the attribute to struct device instead of further
entrenching it in PCIe.  I'm starting to grow tired of saying this
every other week while you guys are all moving into the entirely
wrong direction.

Re: [PATCH 2/4] pci: set "untrusted" flag for truly external devices only

[Mika Westerberg]

On Mon, Jun 15, 2020 at 06:17:40PM -0700, Rajat Jain wrote:
> The "ExternalFacing" devices (root ports) are still internal devices
> that sit on the internal system fabric and thus trusted. Currently they
> were being marked untrusted - likely as an unintended border case.

It was actually intentional :) At the time this was added we did not see
benefits from doing this and even with this you actually are going to
still miss things like a TBT chip that is soldered on the motherboard, I
guess that can be though as an internal device as well.

No objections to this patch, though.

Re: [PATCH 4/4] pci: export untrusted attribute in sysfs

[Rajat Jain]

On Tue, Jun 16, 2020 at 12:32 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Mon, Jun 15, 2020 at 06:17:42PM -0700, Rajat Jain via iommu wrote:
> > This is needed to allow the userspace to determine when an untrusted
> > device has been added, and thus allowing it to bind the driver manually
> > to it, if it so wishes. This is being done as part of the approach
> > discussed at https://lkml.org/lkml/2020/6/9/1331
>
> Please move the attribute to struct device instead of further
> entrenching it in PCIe.

Need clarification. The flag "untrusted" is currently a part of
pci_dev struct, and is populated within the PCI subsystem.

1) Is your suggestion to move this flag as well as the attribute to
device core (in "struct device")? This would allow other buses to
populate/use this flag if they want. By default it'll be set to 0 for
all devices (PCI subsystem will populate it based on platform info,
like it does today).

OR

2) Are you suggesting to keep the "untrusted" flag within PCI, but
attach the sysfs attribute to the base device? (&pci_dev->dev)?

Thanks,

Rajat

>  I'm starting to grow tired of saying this
> every other week while you guys are all moving into the entirely
> wrong direction.

Re: [PATCH 4/4] pci: export untrusted attribute in sysfs

[Christoph Hellwig]

On Tue, Jun 16, 2020 at 12:27:35PM -0700, Rajat Jain wrote:
> Need clarification. The flag "untrusted" is currently a part of
> pci_dev struct, and is populated within the PCI subsystem.

Yes, and that is the problem.

> 
> 1) Is your suggestion to move this flag as well as the attribute to
> device core (in "struct device")? This would allow other buses to
> populate/use this flag if they want. By default it'll be set to 0 for
> all devices (PCI subsystem will populate it based on platform info,
> like it does today).
> 
> OR
> 
> 2) Are you suggesting to keep the "untrusted" flag within PCI, but
> attach the sysfs attribute to the base device? (&pci_dev->dev)?

(1).  As for IOMMUs and userspace policy it really should not matter
what bus a device is on if it is external and not trustworthy.

Re: [PATCH 4/4] pci: export untrusted attribute in sysfs

[Rajat Jain]

Hi Greg, Christoph,

On Wed, Jun 17, 2020 at 12:31 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Tue, Jun 16, 2020 at 12:27:35PM -0700, Rajat Jain wrote:
> > Need clarification. The flag "untrusted" is currently a part of
> > pci_dev struct, and is populated within the PCI subsystem.
>
> Yes, and that is the problem.
>
> >
> > 1) Is your suggestion to move this flag as well as the attribute to
> > device core (in "struct device")? This would allow other buses to
> > populate/use this flag if they want. By default it'll be set to 0 for
> > all devices (PCI subsystem will populate it based on platform info,
> > like it does today).
> >
> > OR
> >
> > 2) Are you suggesting to keep the "untrusted" flag within PCI, but
> > attach the sysfs attribute to the base device? (&pci_dev->dev)?
>
> (1).  As for IOMMUs and userspace policy it really should not matter
> what bus a device is on if it is external and not trustworthy.

Sure. I can move the flag to the "struct device" (and likely call
it "external" instead of "untrusted" so as to make it suitable for
more use cases later).  The buses can fill this up if they know which
devices are external and which ones are not (otherwise it will be 0 by
default). The PCI can fill this up like it does today, from platform
info (ACPI / Device tree). Greg, how does this sound?

Thanks,

Rajat

Re: [PATCH 4/4] pci: export untrusted attribute in sysfs

[Greg Kroah-Hartman]

On Wed, Jun 17, 2020 at 12:53:03PM -0700, Rajat Jain wrote:
> Hi Greg, Christoph,
> 
> On Wed, Jun 17, 2020 at 12:31 AM Christoph Hellwig <hch@infradead.org> wrote:
> >
> > On Tue, Jun 16, 2020 at 12:27:35PM -0700, Rajat Jain wrote:
> > > Need clarification. The flag "untrusted" is currently a part of
> > > pci_dev struct, and is populated within the PCI subsystem.
> >
> > Yes, and that is the problem.
> >
> > >
> > > 1) Is your suggestion to move this flag as well as the attribute to
> > > device core (in "struct device")? This would allow other buses to
> > > populate/use this flag if they want. By default it'll be set to 0 for
> > > all devices (PCI subsystem will populate it based on platform info,
> > > like it does today).
> > >
> > > OR
> > >
> > > 2) Are you suggesting to keep the "untrusted" flag within PCI, but
> > > attach the sysfs attribute to the base device? (&pci_dev->dev)?
> >
> > (1).  As for IOMMUs and userspace policy it really should not matter
> > what bus a device is on if it is external and not trustworthy.
> 
> Sure. I can move the flag to the "struct device" (and likely call
> it "external" instead of "untrusted" so as to make it suitable for
> more use cases later).  The buses can fill this up if they know which
> devices are external and which ones are not (otherwise it will be 0 by
> default). The PCI can fill this up like it does today, from platform
> info (ACPI / Device tree). Greg, how does this sound?

That's fine, convert USB over to use it at the same time if you get the
chance :)

thanks,

greg k-h

Re: [PATCH 4/4] pci: export untrusted attribute in sysfs

[Andy Shevchenko]

On Wed, Jun 17, 2020 at 10:56 PM Rajat Jain <rajatja@google.com> wrote:
> On Wed, Jun 17, 2020 at 12:31 AM Christoph Hellwig <hch@infradead.org> wrote:

...

> (and likely call it "external" instead of "untrusted".

Which is not okay. 'External' to what? 'untrusted' has been carefully
chosen by the meaning of it.
What external does mean for M.2. WWAN card in my laptop? It's in ACPI
tables, but I can replace it.
This is only one example. Or if firmware of some device is altered,
and it's internal (whatever it means) is it trusted or not?

So, please leave it as is (I mean name).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/4] pci: export untrusted attribute in sysfs

[Greg Kroah-Hartman]

On Thu, Jun 18, 2020 at 11:12:56AM +0300, Andy Shevchenko wrote:
> On Wed, Jun 17, 2020 at 10:56 PM Rajat Jain <rajatja@google.com> wrote:
> > On Wed, Jun 17, 2020 at 12:31 AM Christoph Hellwig <hch@infradead.org> wrote:
> 
> ...
> 
> > (and likely call it "external" instead of "untrusted".
> 
> Which is not okay. 'External' to what? 'untrusted' has been carefully
> chosen by the meaning of it.
> What external does mean for M.2. WWAN card in my laptop? It's in ACPI
> tables, but I can replace it.

Then your ACPI tables should show this, there is an attribute for it,
right?

> This is only one example. Or if firmware of some device is altered,
> and it's internal (whatever it means) is it trusted or not?

That is what people are using policy for today, if you object to this,
please bring it up to those developers :)

> So, please leave it as is (I mean name).

firmware today exports this attribute, why do you not want userspace to
also know it?

Trust is different, yes, don't get the two mixed up please.  That should
be a different sysfs attribute for obvious reasons.

thanks,

greg k-h

Re: [PATCH 4/4] pci: export untrusted attribute in sysfs

[Andy Shevchenko]

On Thu, Jun 18, 2020 at 11:36 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Jun 18, 2020 at 11:12:56AM +0300, Andy Shevchenko wrote:
> > On Wed, Jun 17, 2020 at 10:56 PM Rajat Jain <rajatja@google.com> wrote:
> > > On Wed, Jun 17, 2020 at 12:31 AM Christoph Hellwig <hch@infradead.org> wrote:
> >
> > ...
> >
> > > (and likely call it "external" instead of "untrusted".
> >
> > Which is not okay. 'External' to what? 'untrusted' has been carefully
> > chosen by the meaning of it.
> > What external does mean for M.2. WWAN card in my laptop? It's in ACPI
> > tables, but I can replace it.
>
> Then your ACPI tables should show this, there is an attribute for it,
> right?

There is a _PLD() method, but it's for the USB devices (or optional
for others, I don't remember by heart). So, most of the ACPI tables,
alas, don't show this.

> > This is only one example. Or if firmware of some device is altered,
> > and it's internal (whatever it means) is it trusted or not?
>
> That is what people are using policy for today, if you object to this,
> please bring it up to those developers :)

> > So, please leave it as is (I mean name).
>
> firmware today exports this attribute, why do you not want userspace to
> also know it?
>
> Trust is different, yes, don't get the two mixed up please.  That should
> be a different sysfs attribute for obvious reasons.

Yes, as a bottom line that's what I meant as well.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/4] pci: export untrusted attribute in sysfs

[Greg Kroah-Hartman]

On Thu, Jun 18, 2020 at 12:14:41PM +0300, Andy Shevchenko wrote:
> On Thu, Jun 18, 2020 at 11:36 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Jun 18, 2020 at 11:12:56AM +0300, Andy Shevchenko wrote:
> > > On Wed, Jun 17, 2020 at 10:56 PM Rajat Jain <rajatja@google.com> wrote:
> > > > On Wed, Jun 17, 2020 at 12:31 AM Christoph Hellwig <hch@infradead.org> wrote:
> > >
> > > ...
> > >
> > > > (and likely call it "external" instead of "untrusted".
> > >
> > > Which is not okay. 'External' to what? 'untrusted' has been carefully
> > > chosen by the meaning of it.
> > > What external does mean for M.2. WWAN card in my laptop? It's in ACPI
> > > tables, but I can replace it.
> >
> > Then your ACPI tables should show this, there is an attribute for it,
> > right?
> 
> There is a _PLD() method, but it's for the USB devices (or optional
> for others, I don't remember by heart). So, most of the ACPI tables,
> alas, don't show this.

There is something like this for PCI as well, otherwise they wouldn't be
getting this info from "the ether" :)

thanks,

greg k-h

Re: [PATCH 4/4] pci: export untrusted attribute in sysfs

[Rajat Jain]

Hello,

On Thu, Jun 18, 2020 at 2:14 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Thu, Jun 18, 2020 at 11:36 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Jun 18, 2020 at 11:12:56AM +0300, Andy Shevchenko wrote:
> > > On Wed, Jun 17, 2020 at 10:56 PM Rajat Jain <rajatja@google.com> wrote:
> > > > On Wed, Jun 17, 2020 at 12:31 AM Christoph Hellwig <hch@infradead.org> wrote:
> > >
> > > ...
> > >
> > > > (and likely call it "external" instead of "untrusted".
> > >
> > > Which is not okay. 'External' to what? 'untrusted' has been carefully
> > > chosen by the meaning of it.
> > > What external does mean for M.2. WWAN card in my laptop? It's in ACPI
> > > tables, but I can replace it.
> >
> > Then your ACPI tables should show this, there is an attribute for it,
> > right?
>
> There is a _PLD() method, but it's for the USB devices (or optional
> for others, I don't remember by heart). So, most of the ACPI tables,
> alas, don't show this.
>
> > > This is only one example. Or if firmware of some device is altered,
> > > and it's internal (whatever it means) is it trusted or not?
> >
> > That is what people are using policy for today, if you object to this,
> > please bring it up to those developers :)
>
> > > So, please leave it as is (I mean name).
> >
> > firmware today exports this attribute, why do you not want userspace to
> > also know it?

To clarify, the attribute exposed by the firmware today is
"ExternalFacingPort" and "external-facing" respectively:

617654aae50e ("PCI / ACPI: Identify untrusted PCI devices")
9cb30a71ac45d("PCI: OF: Support "external-facing" property")

The kernel flag was named "untrusted" though, hence the assumption
that "external=untrusted" is currently baked into the kernel today.
IMHO, using "external" would fix that (The assumption can thus be
contained in the IOMMU drivers) and at the same time allow more use of
this attribute.

> >
> > Trust is different, yes, don't get the two mixed up please.  That should
> > be a different sysfs attribute for obvious reasons.
>
> Yes, as a bottom line that's what I meant as well.

So what is the consensus here? I don't have a strong opinion - but it
seemed to me Greg is saying "external" and Andy is saying "untrusted"?

Thanks,
Rajat

>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH 4/4] pci: export untrusted attribute in sysfs

[Andy Shevchenko]

On Thu, Jun 18, 2020 at 6:04 PM Rajat Jain <rajatja@google.com> wrote:
> On Thu, Jun 18, 2020 at 2:14 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:

...

> To clarify, the attribute exposed by the firmware today is
> "ExternalFacingPort" and "external-facing" respectively:
>
> 617654aae50e ("PCI / ACPI: Identify untrusted PCI devices")
> 9cb30a71ac45d("PCI: OF: Support "external-facing" property")
>
> The kernel flag was named "untrusted" though, hence the assumption
> that "external=untrusted" is currently baked into the kernel today.
> IMHO, using "external" would fix that (The assumption can thus be
> contained in the IOMMU drivers) and at the same time allow more use of
> this attribute.

That discussion had been held, IIRC, during introduction of the
untrusted member in struct pci_dev...

> > > Trust is different, yes, don't get the two mixed up please.  That should
> > > be a different sysfs attribute for obvious reasons.
> >
> > Yes, as a bottom line that's what I meant as well.
>
> So what is the consensus here? I don't have a strong opinion - but it
> seemed to me Greg is saying "external" and Andy is saying "untrusted"?

...and a conclusion has been made as you may see. So, I would highly
recommend to speak to the author(s) of the patch that introduced /
adopted 'untrusted' member.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/4] pci: export untrusted attribute in sysfs

[Greg Kroah-Hartman]

On Thu, Jun 18, 2020 at 08:03:49AM -0700, Rajat Jain wrote:
> Hello,
> 
> On Thu, Jun 18, 2020 at 2:14 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Thu, Jun 18, 2020 at 11:36 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Thu, Jun 18, 2020 at 11:12:56AM +0300, Andy Shevchenko wrote:
> > > > On Wed, Jun 17, 2020 at 10:56 PM Rajat Jain <rajatja@google.com> wrote:
> > > > > On Wed, Jun 17, 2020 at 12:31 AM Christoph Hellwig <hch@infradead.org> wrote:
> > > >
> > > > ...
> > > >
> > > > > (and likely call it "external" instead of "untrusted".
> > > >
> > > > Which is not okay. 'External' to what? 'untrusted' has been carefully
> > > > chosen by the meaning of it.
> > > > What external does mean for M.2. WWAN card in my laptop? It's in ACPI
> > > > tables, but I can replace it.
> > >
> > > Then your ACPI tables should show this, there is an attribute for it,
> > > right?
> >
> > There is a _PLD() method, but it's for the USB devices (or optional
> > for others, I don't remember by heart). So, most of the ACPI tables,
> > alas, don't show this.
> >
> > > > This is only one example. Or if firmware of some device is altered,
> > > > and it's internal (whatever it means) is it trusted or not?
> > >
> > > That is what people are using policy for today, if you object to this,
> > > please bring it up to those developers :)
> >
> > > > So, please leave it as is (I mean name).
> > >
> > > firmware today exports this attribute, why do you not want userspace to
> > > also know it?
> 
> To clarify, the attribute exposed by the firmware today is
> "ExternalFacingPort" and "external-facing" respectively:
> 
> 617654aae50e ("PCI / ACPI: Identify untrusted PCI devices")
> 9cb30a71ac45d("PCI: OF: Support "external-facing" property")
> 
> The kernel flag was named "untrusted" though, hence the assumption
> that "external=untrusted" is currently baked into the kernel today.
> IMHO, using "external" would fix that (The assumption can thus be
> contained in the IOMMU drivers) and at the same time allow more use of
> this attribute.
> 
> > >
> > > Trust is different, yes, don't get the two mixed up please.  That should
> > > be a different sysfs attribute for obvious reasons.
> >
> > Yes, as a bottom line that's what I meant as well.
> 
> So what is the consensus here? I don't have a strong opinion - but it
> seemed to me Greg is saying "external" and Andy is saying "untrusted"?

Those two things are totally separate things when it comes to a device.

One (external) describes the location of the device in the system.

The other (untrusted) describes what you want the kernel to do with this
device (trust or not trust it).

One you can change (from trust to untrusted or back), the other you can
not, it is a fixed read-only property that describes the hardware device
as defined by the firmware.

Depending on the policy you wish to define, you can use the location of
the device to determine if you want to trust the device or not.

Again, this is what USB does, but I'm getting really tired of saying
this, so I'm going to stop now...

greg k-h

Re: [PATCH 4/4] pci: export untrusted attribute in sysfs

[Raj, Ashok]

Hi Greg,


On Thu, Jun 18, 2020 at 06:02:12PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Jun 18, 2020 at 08:03:49AM -0700, Rajat Jain wrote:
> > Hello,
> > 
> > On Thu, Jun 18, 2020 at 2:14 AM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > >
> > > On Thu, Jun 18, 2020 at 11:36 AM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Thu, Jun 18, 2020 at 11:12:56AM +0300, Andy Shevchenko wrote:
> > > > > On Wed, Jun 17, 2020 at 10:56 PM Rajat Jain <rajatja@google.com> wrote:
> > > > > > On Wed, Jun 17, 2020 at 12:31 AM Christoph Hellwig <hch@infradead.org> wrote:
> > > > >
> > > > > ...
> > > > >
> > > > > > (and likely call it "external" instead of "untrusted".
> > > > >
> > > > > Which is not okay. 'External' to what? 'untrusted' has been carefully
> > > > > chosen by the meaning of it.
> > > > > What external does mean for M.2. WWAN card in my laptop? It's in ACPI
> > > > > tables, but I can replace it.
> > > >
> > > > Then your ACPI tables should show this, there is an attribute for it,
> > > > right?
> > >
> > > There is a _PLD() method, but it's for the USB devices (or optional
> > > for others, I don't remember by heart). So, most of the ACPI tables,
> > > alas, don't show this.
> > >
> > > > > This is only one example. Or if firmware of some device is altered,
> > > > > and it's internal (whatever it means) is it trusted or not?
> > > >
> > > > That is what people are using policy for today, if you object to this,
> > > > please bring it up to those developers :)
> > >
> > > > > So, please leave it as is (I mean name).
> > > >
> > > > firmware today exports this attribute, why do you not want userspace to
> > > > also know it?
> > 
> > To clarify, the attribute exposed by the firmware today is
> > "ExternalFacingPort" and "external-facing" respectively:
> > 
> > 617654aae50e ("PCI / ACPI: Identify untrusted PCI devices")
> > 9cb30a71ac45d("PCI: OF: Support "external-facing" property")
> > 
> > The kernel flag was named "untrusted" though, hence the assumption
> > that "external=untrusted" is currently baked into the kernel today.
> > IMHO, using "external" would fix that (The assumption can thus be
> > contained in the IOMMU drivers) and at the same time allow more use of
> > this attribute.
> > 
> > > >
> > > > Trust is different, yes, don't get the two mixed up please.  That should
> > > > be a different sysfs attribute for obvious reasons.
> > >
> > > Yes, as a bottom line that's what I meant as well.
> > 
> > So what is the consensus here? I don't have a strong opinion - but it
> > seemed to me Greg is saying "external" and Andy is saying "untrusted"?
> 
> Those two things are totally separate things when it comes to a device.

Agree that these are two separate attributes, and better not mixed.

> 
> One (external) describes the location of the device in the system.
> 
> The other (untrusted) describes what you want the kernel to do with this
> device (trust or not trust it).
> 
> One you can change (from trust to untrusted or back), the other you can
> not, it is a fixed read-only property that describes the hardware device
> as defined by the firmware.

The genesis is due to lack of a mechanism to establish if the device 
is trusted or not was the due lack of some specs and implementation around
Component Measurement And Authentication (CMA). Treating external as
untrusted was the best first effort. i.e trust internal 
devices and don't trust external devices for enabling ATS.

But that said external is just describing topology, and if Linux wants to
use that in the policy that's different. Some day external device may also
use CMA to estabilish trust. FWIW even internal devices aren't trust
worthy, except maybe RCIEP's. 

> 
> Depending on the policy you wish to define, you can use the location of
> the device to determine if you want to trust the device or not.
> 

Cheers,
Ashok

Re: [PATCH 4/4] pci: export untrusted attribute in sysfs

[Rajat Jain]

Thanks Greg and Andy for your continued inputs, and thanks Ashok for chiming in.

On Thu, Jun 18, 2020 at 9:23 AM Raj, Ashok <ashok.raj@intel.com> wrote:
>
> Hi Greg,
>
>
> On Thu, Jun 18, 2020 at 06:02:12PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Jun 18, 2020 at 08:03:49AM -0700, Rajat Jain wrote:
> > > Hello,
> > >
> > > On Thu, Jun 18, 2020 at 2:14 AM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > >
> > > > On Thu, Jun 18, 2020 at 11:36 AM Greg Kroah-Hartman
> > > > <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Thu, Jun 18, 2020 at 11:12:56AM +0300, Andy Shevchenko wrote:
> > > > > > On Wed, Jun 17, 2020 at 10:56 PM Rajat Jain <rajatja@google.com> wrote:
> > > > > > > On Wed, Jun 17, 2020 at 12:31 AM Christoph Hellwig <hch@infradead.org> wrote:
> > > > > >
> > > > > > ...
> > > > > >
> > > > > > > (and likely call it "external" instead of "untrusted".
> > > > > >
> > > > > > Which is not okay. 'External' to what? 'untrusted' has been carefully
> > > > > > chosen by the meaning of it.
> > > > > > What external does mean for M.2. WWAN card in my laptop? It's in ACPI
> > > > > > tables, but I can replace it.
> > > > >
> > > > > Then your ACPI tables should show this, there is an attribute for it,
> > > > > right?
> > > >
> > > > There is a _PLD() method, but it's for the USB devices (or optional
> > > > for others, I don't remember by heart). So, most of the ACPI tables,
> > > > alas, don't show this.
> > > >
> > > > > > This is only one example. Or if firmware of some device is altered,
> > > > > > and it's internal (whatever it means) is it trusted or not?
> > > > >
> > > > > That is what people are using policy for today, if you object to this,
> > > > > please bring it up to those developers :)
> > > >
> > > > > > So, please leave it as is (I mean name).
> > > > >
> > > > > firmware today exports this attribute, why do you not want userspace to
> > > > > also know it?
> > >
> > > To clarify, the attribute exposed by the firmware today is
> > > "ExternalFacingPort" and "external-facing" respectively:
> > >
> > > 617654aae50e ("PCI / ACPI: Identify untrusted PCI devices")
> > > 9cb30a71ac45d("PCI: OF: Support "external-facing" property")
> > >
> > > The kernel flag was named "untrusted" though, hence the assumption
> > > that "external=untrusted" is currently baked into the kernel today.
> > > IMHO, using "external" would fix that (The assumption can thus be
> > > contained in the IOMMU drivers) and at the same time allow more use of
> > > this attribute.
> > >
> > > > >
> > > > > Trust is different, yes, don't get the two mixed up please.  That should
> > > > > be a different sysfs attribute for obvious reasons.
> > > >
> > > > Yes, as a bottom line that's what I meant as well.
> > >
> > > So what is the consensus here? I don't have a strong opinion - but it
> > > seemed to me Greg is saying "external" and Andy is saying "untrusted"?
> >
> > Those two things are totally separate things when it comes to a device.
>
> Agree that these are two separate attributes, and better not mixed.

+1.

>
> >
> > One (external) describes the location of the device in the system.
> >
> > The other (untrusted) describes what you want the kernel to do with this
> > device (trust or not trust it).
> >
> > One you can change (from trust to untrusted or back), the other you can
> > not, it is a fixed read-only property that describes the hardware device
> > as defined by the firmware.

Correct. I believe what is being described by the firmware is a fixed
read-only property describing the location of the device ("external")
- not what to do with it ("untrusted").

>
> The genesis is due to lack of a mechanism to establish if the device
> is trusted or not was the due lack of some specs and implementation around
> Component Measurement And Authentication (CMA). Treating external as
> untrusted was the best first effort. i.e trust internal
> devices and don't trust external devices for enabling ATS.
>
> But that said external is just describing topology, and if Linux wants to
> use that in the policy that's different. Some day external device may also
> use CMA to estabilish trust. FWIW even internal devices aren't trust
> worthy, except maybe RCIEP's.

Correct. Since the firmware is actually describing the unchangeable
topology (and not the policy), the takeaway I am taking from this
discussion is that the flag should be called "external".

Like I said, I don't have any hard opinions on this. So if you feel
that my conclusion is wrong and consensus was the other way around
("untrusted"), let me know and I'll be happy to change this.

Thanks,

Rajat

>
> >
> > Depending on the policy you wish to define, you can use the location of
> > the device to determine if you want to trust the device or not.
> >
>
> Cheers,
> Ashok

Re: [PATCH 4/4] pci: export untrusted attribute in sysfs

[Greg Kroah-Hartman]

On Thu, Jun 18, 2020 at 10:23:38AM -0700, Rajat Jain wrote:
> Thanks Greg and Andy for your continued inputs, and thanks Ashok for chiming in.
> 
> On Thu, Jun 18, 2020 at 9:23 AM Raj, Ashok <ashok.raj@intel.com> wrote:
> >
> > Hi Greg,
> >
> >
> > On Thu, Jun 18, 2020 at 06:02:12PM +0200, Greg Kroah-Hartman wrote:
> > > On Thu, Jun 18, 2020 at 08:03:49AM -0700, Rajat Jain wrote:
> > > > Hello,
> > > >
> > > > On Thu, Jun 18, 2020 at 2:14 AM Andy Shevchenko
> > > > <andy.shevchenko@gmail.com> wrote:
> > > > >
> > > > > On Thu, Jun 18, 2020 at 11:36 AM Greg Kroah-Hartman
> > > > > <gregkh@linuxfoundation.org> wrote:
> > > > > >
> > > > > > On Thu, Jun 18, 2020 at 11:12:56AM +0300, Andy Shevchenko wrote:
> > > > > > > On Wed, Jun 17, 2020 at 10:56 PM Rajat Jain <rajatja@google.com> wrote:
> > > > > > > > On Wed, Jun 17, 2020 at 12:31 AM Christoph Hellwig <hch@infradead.org> wrote:
> > > > > > >
> > > > > > > ...
> > > > > > >
> > > > > > > > (and likely call it "external" instead of "untrusted".
> > > > > > >
> > > > > > > Which is not okay. 'External' to what? 'untrusted' has been carefully
> > > > > > > chosen by the meaning of it.
> > > > > > > What external does mean for M.2. WWAN card in my laptop? It's in ACPI
> > > > > > > tables, but I can replace it.
> > > > > >
> > > > > > Then your ACPI tables should show this, there is an attribute for it,
> > > > > > right?
> > > > >
> > > > > There is a _PLD() method, but it's for the USB devices (or optional
> > > > > for others, I don't remember by heart). So, most of the ACPI tables,
> > > > > alas, don't show this.
> > > > >
> > > > > > > This is only one example. Or if firmware of some device is altered,
> > > > > > > and it's internal (whatever it means) is it trusted or not?
> > > > > >
> > > > > > That is what people are using policy for today, if you object to this,
> > > > > > please bring it up to those developers :)
> > > > >
> > > > > > > So, please leave it as is (I mean name).
> > > > > >
> > > > > > firmware today exports this attribute, why do you not want userspace to
> > > > > > also know it?
> > > >
> > > > To clarify, the attribute exposed by the firmware today is
> > > > "ExternalFacingPort" and "external-facing" respectively:
> > > >
> > > > 617654aae50e ("PCI / ACPI: Identify untrusted PCI devices")
> > > > 9cb30a71ac45d("PCI: OF: Support "external-facing" property")
> > > >
> > > > The kernel flag was named "untrusted" though, hence the assumption
> > > > that "external=untrusted" is currently baked into the kernel today.
> > > > IMHO, using "external" would fix that (The assumption can thus be
> > > > contained in the IOMMU drivers) and at the same time allow more use of
> > > > this attribute.
> > > >
> > > > > >
> > > > > > Trust is different, yes, don't get the two mixed up please.  That should
> > > > > > be a different sysfs attribute for obvious reasons.
> > > > >
> > > > > Yes, as a bottom line that's what I meant as well.
> > > >
> > > > So what is the consensus here? I don't have a strong opinion - but it
> > > > seemed to me Greg is saying "external" and Andy is saying "untrusted"?
> > >
> > > Those two things are totally separate things when it comes to a device.
> >
> > Agree that these are two separate attributes, and better not mixed.
> 
> +1.
> 
> >
> > >
> > > One (external) describes the location of the device in the system.
> > >
> > > The other (untrusted) describes what you want the kernel to do with this
> > > device (trust or not trust it).
> > >
> > > One you can change (from trust to untrusted or back), the other you can
> > > not, it is a fixed read-only property that describes the hardware device
> > > as defined by the firmware.
> 
> Correct. I believe what is being described by the firmware is a fixed
> read-only property describing the location of the device ("external")
> - not what to do with it ("untrusted").
> 
> >
> > The genesis is due to lack of a mechanism to establish if the device
> > is trusted or not was the due lack of some specs and implementation around
> > Component Measurement And Authentication (CMA). Treating external as
> > untrusted was the best first effort. i.e trust internal
> > devices and don't trust external devices for enabling ATS.
> >
> > But that said external is just describing topology, and if Linux wants to
> > use that in the policy that's different. Some day external device may also
> > use CMA to estabilish trust. FWIW even internal devices aren't trust
> > worthy, except maybe RCIEP's.
> 
> Correct. Since the firmware is actually describing the unchangeable
> topology (and not the policy), the takeaway I am taking from this
> discussion is that the flag should be called "external".

The attribute should be called something like "location" or something
like that (naming is hard), as you don't always know if something is
external or not (it could be internal, it could be unknown, it could be
internal to an external device that you trust (think PCI drawers for
"super" computers that are hot pluggable but yet really part of the
internal bus).

> Like I said, I don't have any hard opinions on this. So if you feel
> that my conclusion is wrong and consensus was the other way around
> ("untrusted"), let me know and I'll be happy to change this.

"trust" has no direct relation to the location, except in a policy of
what you wish to do with that device, so as long as you keep them
separate that way, I am fine with it.

thanks,

greg k-h

Re: [PATCH 4/4] pci: export untrusted attribute in sysfs

[Rajat Jain]

Hi Bjorn, All,

Thank you so much for your helpful review and inputs.

On Mon, Jun 15, 2020 at 6:17 PM Rajat Jain <rajatja@google.com> wrote:
>
> This is needed to allow the userspace to determine when an untrusted
> device has been added, and thus allowing it to bind the driver manually
> to it, if it so wishes. This is being done as part of the approach
> discussed at https://lkml.org/lkml/2020/6/9/1331
>
> Signed-off-by: Rajat Jain <rajatja@google.com>

Considering the suggestions obtained on this patch to move this
attribute to device core, please consider this particular patch
rescinded. I'll submit this as a separate patch since I think that
will take more bake time and more discussion, than the other patches
in this patchset.

Please consider applying the other 3 patches in this patchset though,
and I'd appreciate your review and comments.

Thanks & Best Regards,

Rajat


> ---
>  drivers/pci/pci-sysfs.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 6d78df981d41a..574e9c613ba26 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -50,6 +50,7 @@ pci_config_attr(subsystem_device, "0x%04x\n");
>  pci_config_attr(revision, "0x%02x\n");
>  pci_config_attr(class, "0x%06x\n");
>  pci_config_attr(irq, "%u\n");
> +pci_config_attr(untrusted, "%u\n");
>
>  static ssize_t broken_parity_status_show(struct device *dev,
>                                          struct device_attribute *attr,
> @@ -608,6 +609,7 @@ static struct attribute *pci_dev_attrs[] = {
>  #endif
>         &dev_attr_driver_override.attr,
>         &dev_attr_ari_enabled.attr,
> +       &dev_attr_untrusted.attr,
>         NULL,
>  };
>
> --
> 2.27.0.290.gba653c62da-goog
>

Re: [PATCH 3/4] pci: acs: Enable PCI_ACS_TB for untrusted/external-facing devices

[Raj, Ashok]

Hi Rajat


On Mon, Jun 15, 2020 at 06:17:41PM -0700, Rajat Jain wrote:
> When enabling ACS, currently the bit "translation blocking" was
> not getting changed at all. Set it to disable translation blocking

Maybe you meant "enable translation blocking" ?

Keep the commit log simple:

When enabling ACS, enable translation blocking for external facing ports
and untrusted devices.

> too for all external facing or untrusted devices. This is OK
> because ATS is only allowed on internal devces.
> 
> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---
>  drivers/pci/pci.c    |  4 ++++
>  drivers/pci/quirks.c | 11 +++++++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index d2ff987585855..79853b52658a2 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3330,6 +3330,10 @@ static void pci_std_enable_acs(struct pci_dev *dev)
>  	/* Upstream Forwarding */
>  	ctrl |= (cap & PCI_ACS_UF);
>  
> +	if (dev->external_facing || dev->untrusted)
> +		/* Translation Blocking */
> +		ctrl |= (cap & PCI_ACS_TB);
> +
>  	pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
>  }
>  
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index b341628e47527..6294adeac4049 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4934,6 +4934,13 @@ static void pci_quirk_enable_intel_rp_mpc_acs(struct pci_dev *dev)
>  	}
>  }
>  
> +/*
> + * Currently this quirk does the equivalent of
> + * PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_SV
> + *
> + * Currently missing, it also needs to do equivalent of PCI_ACS_TB,
> + * if dev->external_facing || dev->untrusted
> + */
>  static int pci_quirk_enable_intel_pch_acs(struct pci_dev *dev)
>  {
>  	if (!pci_quirk_intel_pch_acs_match(dev))
> @@ -4973,6 +4980,10 @@ static int pci_quirk_enable_intel_spt_pch_acs(struct pci_dev *dev)
>  	ctrl |= (cap & PCI_ACS_CR);
>  	ctrl |= (cap & PCI_ACS_UF);
>  
> +	if (dev->external_facing || dev->untrusted)
> +		/* Translation Blocking */
> +		ctrl |= (cap & PCI_ACS_TB);
> +
>  	pci_write_config_dword(dev, pos + INTEL_SPT_ACS_CTRL, ctrl);
>  
>  	pci_info(dev, "Intel SPT PCH root port ACS workaround enabled\n");
> -- 
> 2.27.0.290.gba653c62da-goog
> 

Re: [PATCH 3/4] pci: acs: Enable PCI_ACS_TB for untrusted/external-facing devices

[Rajat Jain]

Hi Ashok,

On Fri, Jun 19, 2020 at 9:10 AM Raj, Ashok <ashok.raj@intel.com> wrote:
>
> Hi Rajat
>
>
> On Mon, Jun 15, 2020 at 06:17:41PM -0700, Rajat Jain wrote:
> > When enabling ACS, currently the bit "translation blocking" was
> > not getting changed at all. Set it to disable translation blocking
>
> Maybe you meant "enable translation blocking" ?

Oops, yes.

>
> Keep the commit log simple:
>
> When enabling ACS, enable translation blocking for external facing ports
> and untrusted devices.

Ack. Will change in the next iteration (currently waiting to see if
there are any more comments).

Thanks & Regards,

Rajat

>
> > too for all external facing or untrusted devices. This is OK
> > because ATS is only allowed on internal devces.
> >
> > Signed-off-by: Rajat Jain <rajatja@google.com>
> > ---
> >  drivers/pci/pci.c    |  4 ++++
> >  drivers/pci/quirks.c | 11 +++++++++++
> >  2 files changed, 15 insertions(+)
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index d2ff987585855..79853b52658a2 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -3330,6 +3330,10 @@ static void pci_std_enable_acs(struct pci_dev *dev)
> >       /* Upstream Forwarding */
> >       ctrl |= (cap & PCI_ACS_UF);
> >
> > +     if (dev->external_facing || dev->untrusted)
> > +             /* Translation Blocking */
> > +             ctrl |= (cap & PCI_ACS_TB);
> > +
> >       pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
> >  }
> >
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index b341628e47527..6294adeac4049 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -4934,6 +4934,13 @@ static void pci_quirk_enable_intel_rp_mpc_acs(struct pci_dev *dev)
> >       }
> >  }
> >
> > +/*
> > + * Currently this quirk does the equivalent of
> > + * PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_SV
> > + *
> > + * Currently missing, it also needs to do equivalent of PCI_ACS_TB,
> > + * if dev->external_facing || dev->untrusted
> > + */
> >  static int pci_quirk_enable_intel_pch_acs(struct pci_dev *dev)
> >  {
> >       if (!pci_quirk_intel_pch_acs_match(dev))
> > @@ -4973,6 +4980,10 @@ static int pci_quirk_enable_intel_spt_pch_acs(struct pci_dev *dev)
> >       ctrl |= (cap & PCI_ACS_CR);
> >       ctrl |= (cap & PCI_ACS_UF);
> >
> > +     if (dev->external_facing || dev->untrusted)
> > +             /* Translation Blocking */
> > +             ctrl |= (cap & PCI_ACS_TB);
> > +
> >       pci_write_config_dword(dev, pos + INTEL_SPT_ACS_CTRL, ctrl);
> >
> >       pci_info(dev, "Intel SPT PCH root port ACS workaround enabled\n");
> > --
> > 2.27.0.290.gba653c62da-goog
> >