[PATCH v7 2/2] PCI: Rename pci_dev->untrusted to pci_dev->untrusted_dma
Rename the field to make it more clear, that the device can execute DMA
attacks on the system, and thus the system may need protection from
such attacks from this device.
No functional change intended.
Signed-off-by: Rajat Jain
Reviewed-by: Mika Westerberg
Reviewed-by: Lu Baolu
Acked-by: Rafael J. Wysocki
---
v7: Added Lu Baolu's "Reviewed by" tag.
v6: No change in this patch, rebased on top of changes in other patch.
v5: Use "untrusted_dma" as property name, based on feedback.
Reorder the patches in the series.
v4: Initial version, created based on comments on other patch
drivers/iommu/dma-iommu.c | 6 +++---
drivers/iommu/intel/iommu.c | 2 +-
drivers/iommu/iommu.c | 2 +-
drivers/pci/ats.c | 2 +-
drivers/pci/pci-acpi.c | 2 +-
drivers/pci/pci.c | 2 +-
drivers/pci/probe.c | 8
drivers/pci/quirks.c| 2 +-
include/linux/pci.h | 5 +++--
9 files changed, 16 insertions(+), 15 deletions(-)
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 09f6e1c0f9c0..aeee4be7614d 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -497,14 +497,14 @@ static int iova_reserve_iommu_regions(struct device *dev,
return ret;
}
-static bool dev_is_untrusted(struct device *dev)
+static bool dev_has_untrusted_dma(struct device *dev)
{
- return dev_is_pci(dev) && to_pci_dev(dev)->untrusted;
+ return dev_is_pci(dev) && to_pci_dev(dev)->untrusted_dma;
}
static bool dev_use_swiotlb(struct device *dev)
{
- return IS_ENABLED(CONFIG_SWIOTLB) && dev_is_untrusted(dev);
+ return IS_ENABLED(CONFIG_SWIOTLB) && dev_has_untrusted_dma(dev);
}
/**
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index df5c62ecf942..b88f47391140 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4843,7 +4843,7 @@ static bool intel_iommu_is_attach_deferred(struct device
*dev)
*/
static bool risky_device(struct pci_dev *pdev)
{
- if (pdev->untrusted) {
+ if (pdev->untrusted_dma) {
pci_info(pdev,
"Skipping IOMMU quirk for dev [%04X:%04X] on untrusted
PCI link\n",
pdev->vendor, pdev->device);
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index f2c45b85b9fc..d8d3133e2947 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1525,7 +1525,7 @@ static int iommu_get_def_domain_type(struct device *dev)
{
const struct iommu_ops *ops = dev_iommu_ops(dev);
- if (dev_is_pci(dev) && to_pci_dev(dev)->untrusted)
+ if (dev_is_pci(dev) && to_pci_dev(dev)->untrusted_dma)
return IOMMU_DOMAIN_DMA;
if (ops->def_domain_type)
diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index c967ad6e2626..477c16ba9341 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -42,7 +42,7 @@ bool pci_ats_supported(struct pci_dev *dev)
if (!dev->ats_cap)
return false;
- return (dev->untrusted == 0);
+ return (dev->untrusted_dma == 0);
}
EXPORT_SYMBOL_GPL(pci_ats_supported);
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 8cb4725d41fa..bf04e873c96a 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -1396,7 +1396,7 @@ void pci_acpi_setup(struct device *dev, struct
acpi_device *adev)
pci_acpi_optimize_delay(pci_dev, adev->handle);
pci_acpi_set_external_facing(pci_dev);
- pci_dev->untrusted |= pci_dev_has_dma_property(pci_dev);
+ pci_dev->untrusted_dma |= pci_dev_has_dma_property(pci_dev);
pci_acpi_add_edr_notifier(pci_dev);
pci_acpi_add_pm_notifier(adev, pci_dev);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 9ecce435fb3f..1fb0eb8646c8 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -958,7 +958,7 @@ static void pci_std_enable_acs(struct pci_dev *dev)
ctrl |= (cap & PCI_ACS_UF);
/* Enable Translation Blocking for external devices and noats */
- if (pci_ats_disabled() || dev->external_facing || dev->untrusted)
+ if (pci_ats_disabled() || dev->external_facing || dev->untrusted_dma)
ctrl |= (cap & PCI_ACS_TB);
pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 17a969942d37..d2a9b26fcede 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1587,7 +1587,7 @@ static void set_pcie_thunderbolt(struct pci_dev *dev)
dev->is_thunderbolt = 1;
}
-static void set_pcie_untrusted(struct pci_dev *dev)
+static void pci_set_untrusted_dma(struct pci_dev *dev)
{
struct pci_dev *parent;
@@ -1596,8 +1596,8 @@ static void set_pcie_untrusted(struct pci_dev *dev)
* untrusted as well.
*/
parent = pci_upstream_bridge(dev);
- if (parent && (parent->untrusted || parent->external_facing))
- dev->untrusted = true;
+
[PATCH v7 1/2] PCI/ACPI: Support Microsoft's "DmaProperty"
The "DmaProperty" is supported and currently documented and used by
Microsoft [link 1 below], to flag internal PCIe root ports that need
DMA protection [link 2 below]. We have discussed with them and reached
a common understanding that they shall change their MSDN documentation
to say that the same property can be used to protect any PCI device,
and not just internal PCIe root ports (since there is no point
introducing yet another property for arbitrary PCI devices). This helps
with security from internal devices that offer an attack surface for
DMA attacks (e.g. internal network devices).
Support DmaProperty to mark DMA from a PCI device as untrusted.
Link: [1]
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
Link: [2]
https://docs.microsoft.com/en-us/windows/security/information-protection/kernel-dma-protection-for-thunderbolt
Signed-off-by: Rajat Jain
Reviewed-by: Mika Westerberg
Acked-by: Rafael J. Wysocki
---
v7: * Update the comment, based on feedback.
v6: * Take care of Bjorn's comments:
- Update the commit log
- Rename to pci_dev_has_dma_property()
- Use acpi_dev_get_property()
v5: * Reorder the patches in the series
v4: * Add the GUID.
* Update the comment and commitlog.
v3: * Use Microsoft's documented property "DmaProperty"
* Resctrict to ACPI only
drivers/acpi/property.c | 3 +++
drivers/pci/pci-acpi.c | 22 ++
2 files changed, 25 insertions(+)
diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 12bbfe833609..bafe35c301ac 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -48,6 +48,9 @@ static const guid_t prp_guids[] = {
/* Storage device needs D3 GUID: 5025030f-842f-4ab4-a561-99a5189762d0 */
GUID_INIT(0x5025030f, 0x842f, 0x4ab4,
0xa5, 0x61, 0x99, 0xa5, 0x18, 0x97, 0x62, 0xd0),
+ /* DmaProperty for PCI devices GUID:
70d24161-6dd5-4c9e-8070-705531292865 */
+ GUID_INIT(0x70d24161, 0x6dd5, 0x4c9e,
+ 0x80, 0x70, 0x70, 0x55, 0x31, 0x29, 0x28, 0x65),
};
/* ACPI _DSD data subnodes GUID: dbb8e3e6-5886-4ba6-8795-1319f52a966b */
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 3ae435beaf0a..8cb4725d41fa 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -1369,12 +1369,34 @@ static void pci_acpi_set_external_facing(struct pci_dev
*dev)
dev->external_facing = 1;
}
+static int pci_dev_has_dma_property(struct pci_dev *dev)
+{
+ struct acpi_device *adev;
+ const union acpi_object *obj;
+
+ adev = ACPI_COMPANION(>dev);
+ if (!adev)
+ return 0;
+
+ /*
+* Property used by Microsoft Windows to enforce IOMMU DMA
+* protection from any device, that the system may not fully trust;
+* we'll honour it the same way.
+*/
+ if (!acpi_dev_get_property(adev, "DmaProperty", ACPI_TYPE_INTEGER,
+ ) && obj->integer.value == 1)
+ return 1;
+
+ return 0;
+}
+
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_dev->untrusted |= pci_dev_has_dma_property(pci_dev);
pci_acpi_add_edr_notifier(pci_dev);
pci_acpi_add_pm_notifier(adev, pci_dev);
--
2.36.0.rc2.479.g8af0fa9b8e-goog
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 1/2] PCI/ACPI: Support Microsoft's "DmaProperty"
On Tue, Apr 26, 2022 at 4:15 AM Robin Murphy wrote:
>
> On 2022-04-26 01:06, Rajat Jain via iommu wrote:
> > The "DmaProperty" is supported and currently documented and used by
> > Microsoft [link 1 below], to flag internal PCIe root ports that need
> > DMA protection [link 2 below]. We have discussed with them and reached
> > a common understanding that they shall change their MSDN documentation
> > to say that the same property can be used to protect any PCI device,
> > and not just internal PCIe root ports (since there is no point
> > introducing yet another property for arbitrary PCI devices). This helps
> > with security from internal devices that offer an attack surface for
> > DMA attacks (e.g. internal network devices).
> >
> > Support DmaProperty to mark DMA from a PCI device as untrusted.
> >
> > Link: [1]
> > 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
> > Link: [2]
> > https://docs.microsoft.com/en-us/windows/security/information-protection/kernel-dma-protection-for-thunderbolt
> > Signed-off-by: Rajat Jain
> > Reviewed-by: Mika Westerberg
> > Acked-by: Rafael J. Wysocki
> > ---
> > v6: * Take care of Bjorn's comments:
> > - Update the commit log
> > - Rename to pci_dev_has_dma_property()
> > - Use acpi_dev_get_property()
> > v5: * Reorder the patches in the series
> > v4: * Add the GUID.
> > * Update the comment and commitlog.
> > v3: * Use Microsoft's documented property "DmaProperty"
> > * Resctrict to ACPI only
> >
> > drivers/acpi/property.c | 3 +++
> > drivers/pci/pci-acpi.c | 21 +
> > 2 files changed, 24 insertions(+)
> >
> > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > index 12bbfe833609..bafe35c301ac 100644
> > --- a/drivers/acpi/property.c
> > +++ b/drivers/acpi/property.c
> > @@ -48,6 +48,9 @@ static const guid_t prp_guids[] = {
> > /* Storage device needs D3 GUID: 5025030f-842f-4ab4-a561-99a5189762d0
> > */
> > GUID_INIT(0x5025030f, 0x842f, 0x4ab4,
> > 0xa5, 0x61, 0x99, 0xa5, 0x18, 0x97, 0x62, 0xd0),
> > + /* DmaProperty for PCI devices GUID:
> > 70d24161-6dd5-4c9e-8070-705531292865 */
> > + GUID_INIT(0x70d24161, 0x6dd5, 0x4c9e,
> > + 0x80, 0x70, 0x70, 0x55, 0x31, 0x29, 0x28, 0x65),
> > };
> >
> > /* ACPI _DSD data subnodes GUID: dbb8e3e6-5886-4ba6-8795-1319f52a966b */
> > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> > index 3ae435beaf0a..d7c6ba48486f 100644
> > --- a/drivers/pci/pci-acpi.c
> > +++ b/drivers/pci/pci-acpi.c
> > @@ -1369,12 +1369,33 @@ static void pci_acpi_set_external_facing(struct
> > pci_dev *dev)
> > dev->external_facing = 1;
> > }
> >
> > +static int pci_dev_has_dma_property(struct pci_dev *dev)
> > +{
> > + struct acpi_device *adev;
> > + const union acpi_object *obj;
> > +
> > + adev = ACPI_COMPANION(>dev);
> > + if (!adev)
> > + return 0;
> > +
> > + /*
> > + * Property also used by Microsoft Windows for same purpose,
> > + * (to implement DMA protection from a device, using the IOMMU).
>
> Nit: there is no context for "same purpose" here, so this comment is
> more confusing than helpful. Might I suggest a rewording like:
>
> /*
> * Property used by Microsoft Windows to enforce IOMMU DMA
> * protection for any device that the system might not fully
> * trust; we'll honour it the same way.
> */
>
> ?
Sure, will do.
>
> Personally I think it would have been more logical to handle this in
> pci_set_dma_untrusted(), but I see some of those implementation aspects
> have already been discussed, and Bjorn's preference definitely wins over
> mine here :)
Yes, this was discussed. The primary reason is that ACPI properties
for PCI devices are not available at the time pci_set_untrusted_dma()
is called.
Thanks & Best Regards,
Rajat
>
> Thanks,
> Robin.
>
> > + */
> > + if (!acpi_dev_get_property(adev, "DmaProperty", ACPI_TYPE_INTEGER,
> > +) && obj->integer.value == 1)
> > + return 1;
> > +
> > + return 0;
> > +}
> > +
> > 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_dev->untrusted |= pci_dev_has_dma_property(pci_dev); >
> > pci_acpi_add_edr_notifier(pci_dev);
> >
> > pci_acpi_add_pm_notifier(adev, pci_dev);
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v6 1/2] PCI/ACPI: Support Microsoft's "DmaProperty"
The "DmaProperty" is supported and currently documented and used by
Microsoft [link 1 below], to flag internal PCIe root ports that need
DMA protection [link 2 below]. We have discussed with them and reached
a common understanding that they shall change their MSDN documentation
to say that the same property can be used to protect any PCI device,
and not just internal PCIe root ports (since there is no point
introducing yet another property for arbitrary PCI devices). This helps
with security from internal devices that offer an attack surface for
DMA attacks (e.g. internal network devices).
Support DmaProperty to mark DMA from a PCI device as untrusted.
Link: [1]
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
Link: [2]
https://docs.microsoft.com/en-us/windows/security/information-protection/kernel-dma-protection-for-thunderbolt
Signed-off-by: Rajat Jain
Reviewed-by: Mika Westerberg
Acked-by: Rafael J. Wysocki
---
v6: * Take care of Bjorn's comments:
- Update the commit log
- Rename to pci_dev_has_dma_property()
- Use acpi_dev_get_property()
v5: * Reorder the patches in the series
v4: * Add the GUID.
* Update the comment and commitlog.
v3: * Use Microsoft's documented property "DmaProperty"
* Resctrict to ACPI only
drivers/acpi/property.c | 3 +++
drivers/pci/pci-acpi.c | 21 +
2 files changed, 24 insertions(+)
diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 12bbfe833609..bafe35c301ac 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -48,6 +48,9 @@ static const guid_t prp_guids[] = {
/* Storage device needs D3 GUID: 5025030f-842f-4ab4-a561-99a5189762d0 */
GUID_INIT(0x5025030f, 0x842f, 0x4ab4,
0xa5, 0x61, 0x99, 0xa5, 0x18, 0x97, 0x62, 0xd0),
+ /* DmaProperty for PCI devices GUID:
70d24161-6dd5-4c9e-8070-705531292865 */
+ GUID_INIT(0x70d24161, 0x6dd5, 0x4c9e,
+ 0x80, 0x70, 0x70, 0x55, 0x31, 0x29, 0x28, 0x65),
};
/* ACPI _DSD data subnodes GUID: dbb8e3e6-5886-4ba6-8795-1319f52a966b */
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 3ae435beaf0a..d7c6ba48486f 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -1369,12 +1369,33 @@ static void pci_acpi_set_external_facing(struct pci_dev
*dev)
dev->external_facing = 1;
}
+static int pci_dev_has_dma_property(struct pci_dev *dev)
+{
+ struct acpi_device *adev;
+ const union acpi_object *obj;
+
+ adev = ACPI_COMPANION(>dev);
+ if (!adev)
+ return 0;
+
+ /*
+* Property also used by Microsoft Windows for same purpose,
+* (to implement DMA protection from a device, using the IOMMU).
+*/
+ if (!acpi_dev_get_property(adev, "DmaProperty", ACPI_TYPE_INTEGER,
+ ) && obj->integer.value == 1)
+ return 1;
+
+ return 0;
+}
+
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_dev->untrusted |= pci_dev_has_dma_property(pci_dev);
pci_acpi_add_edr_notifier(pci_dev);
pci_acpi_add_pm_notifier(adev, pci_dev);
--
2.36.0.rc2.479.g8af0fa9b8e-goog
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v6 2/2] PCI: Rename pci_dev->untrusted to pci_dev->untrusted_dma
Rename the field to make it more clear, that the device can execute DMA
attacks on the system, and thus the system may need protection from
such attacks from this device.
No functional change intended.
Signed-off-by: Rajat Jain
Reviewed-by: Mika Westerberg
Acked-by: Rafael J. Wysocki
---
v6: No change in this patch, rebased on top of changes in other patch.
v5: Use "untrusted_dma" as property name, based on feedback.
Reorder the patches in the series.
v4: Initial version, created based on comments on other patch
drivers/iommu/dma-iommu.c | 6 +++---
drivers/iommu/intel/iommu.c | 2 +-
drivers/iommu/iommu.c | 2 +-
drivers/pci/ats.c | 2 +-
drivers/pci/pci-acpi.c | 2 +-
drivers/pci/pci.c | 2 +-
drivers/pci/probe.c | 8
drivers/pci/quirks.c| 2 +-
include/linux/pci.h | 5 +++--
9 files changed, 16 insertions(+), 15 deletions(-)
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 09f6e1c0f9c0..aeee4be7614d 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -497,14 +497,14 @@ static int iova_reserve_iommu_regions(struct device *dev,
return ret;
}
-static bool dev_is_untrusted(struct device *dev)
+static bool dev_has_untrusted_dma(struct device *dev)
{
- return dev_is_pci(dev) && to_pci_dev(dev)->untrusted;
+ return dev_is_pci(dev) && to_pci_dev(dev)->untrusted_dma;
}
static bool dev_use_swiotlb(struct device *dev)
{
- return IS_ENABLED(CONFIG_SWIOTLB) && dev_is_untrusted(dev);
+ return IS_ENABLED(CONFIG_SWIOTLB) && dev_has_untrusted_dma(dev);
}
/**
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index df5c62ecf942..b88f47391140 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4843,7 +4843,7 @@ static bool intel_iommu_is_attach_deferred(struct device
*dev)
*/
static bool risky_device(struct pci_dev *pdev)
{
- if (pdev->untrusted) {
+ if (pdev->untrusted_dma) {
pci_info(pdev,
"Skipping IOMMU quirk for dev [%04X:%04X] on untrusted
PCI link\n",
pdev->vendor, pdev->device);
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index f2c45b85b9fc..d8d3133e2947 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1525,7 +1525,7 @@ static int iommu_get_def_domain_type(struct device *dev)
{
const struct iommu_ops *ops = dev_iommu_ops(dev);
- if (dev_is_pci(dev) && to_pci_dev(dev)->untrusted)
+ if (dev_is_pci(dev) && to_pci_dev(dev)->untrusted_dma)
return IOMMU_DOMAIN_DMA;
if (ops->def_domain_type)
diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index c967ad6e2626..477c16ba9341 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -42,7 +42,7 @@ bool pci_ats_supported(struct pci_dev *dev)
if (!dev->ats_cap)
return false;
- return (dev->untrusted == 0);
+ return (dev->untrusted_dma == 0);
}
EXPORT_SYMBOL_GPL(pci_ats_supported);
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index d7c6ba48486f..7c2784e7e954 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -1395,7 +1395,7 @@ void pci_acpi_setup(struct device *dev, struct
acpi_device *adev)
pci_acpi_optimize_delay(pci_dev, adev->handle);
pci_acpi_set_external_facing(pci_dev);
- pci_dev->untrusted |= pci_dev_has_dma_property(pci_dev);
+ pci_dev->untrusted_dma |= pci_dev_has_dma_property(pci_dev);
pci_acpi_add_edr_notifier(pci_dev);
pci_acpi_add_pm_notifier(adev, pci_dev);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 9ecce435fb3f..1fb0eb8646c8 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -958,7 +958,7 @@ static void pci_std_enable_acs(struct pci_dev *dev)
ctrl |= (cap & PCI_ACS_UF);
/* Enable Translation Blocking for external devices and noats */
- if (pci_ats_disabled() || dev->external_facing || dev->untrusted)
+ if (pci_ats_disabled() || dev->external_facing || dev->untrusted_dma)
ctrl |= (cap & PCI_ACS_TB);
pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 17a969942d37..d2a9b26fcede 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1587,7 +1587,7 @@ static void set_pcie_thunderbolt(struct pci_dev *dev)
dev->is_thunderbolt = 1;
}
-static void set_pcie_untrusted(struct pci_dev *dev)
+static void pci_set_untrusted_dma(struct pci_dev *dev)
{
struct pci_dev *parent;
@@ -1596,8 +1596,8 @@ static void set_pcie_untrusted(struct pci_dev *dev)
* untrusted as well.
*/
parent = pci_upstream_bridge(dev);
- if (parent && (parent->untrusted || parent->external_facing))
- dev->untrusted = true;
+ if (parent && (parent->untrusted_dma ||
Re: [PATCH v5 1/2] PCI: ACPI: Support Microsoft's "DmaProperty"
Hello Bjorn,
On Thu, Apr 7, 2022 at 12:17 PM Bjorn Helgaas wrote:
>
> In subject,
>
> PCI/ACPI: ...
>
> would be consistent with previous history (at least things coming
> through the PCI tree :)).
Will do.
>
> On Fri, Mar 25, 2022 at 11:46:08AM -0700, Rajat Jain wrote:
> > The "DmaProperty" is supported and documented by Microsoft here:
> > https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports
>
> Here's a more specific link (could probably be referenced below to
> avoid cluttering the text here):
>
> 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
Will do.
>
> > They use this property for DMA protection:
> > https://docs.microsoft.com/en-us/windows/security/information-protection/kernel-dma-protection-for-thunderbolt
> >
> > Support the "DmaProperty" with the same semantics. This is useful for
> > internal PCI devices that do not hang off a PCIe rootport, but offer
> > an attack surface for DMA attacks (e.g. internal network devices).
>
> Same semantics as what?
Er, I meant the same semantics as the "DmaProperty". Please also see below.
>
> The MS description of "ExternalFacingPort" says:
>
> This ACPI object enables the operating system to identify externally
> exposed PCIe hierarchies, such as Thunderbolt.
>
No, my patch doesn't have to do with this one.
> and "DmaProperty" says:
>
> This ACPI object enables the operating system to identify internal
> PCIe hierarchies that are easily accessible by users (such as,
> Laptop M.2 PCIe slots accessible by way of a latch) and require
> protection by the OS Kernel DMA Protection mechanism.
Yes, this is the property that my patch uses. Microsoft has agreed to
update this documentation (in a sideband thread that I also copied you
on), with the updated semantics that this property can be used to
identify any PCI devices that require Kernel DMA protection. i.e. the
property is not restricted to identify "internal PCIe hierarchies"
(starting at root port), but to "any PCI device".
>
> I don't really understand why they called out "laptop M.2 PCIe slots"
> here. Is the idea that those are more accessible than a standard
> internal PCIe slot? Seems like a pretty small distinction to me.
>
> I can understand your example of internal network devices adding an
> attack surface. But I don't see how "DmaProperty" helps identify
> those. Wouldn't a NIC in a standard internal PCIe slot add the same
> attack surface?
Yes it would. The attack surface is the same. They probably only
thought of devices external to the SoC (starting from a root port)
when designing this property and thus called out internal M.2 PCI
slots. But nowhave realized that this could be opened to any PCI
device.
>
> > Signed-off-by: Rajat Jain
> > Reviewed-by: Mika Westerberg
> > ---
> > v5: * Reorder the patches in the series
> > v4: * Add the GUID.
> > * Update the comment and commitlog.
> > v3: * Use Microsoft's documented property "DmaProperty"
> > * Resctrict to ACPI only
> >
> > drivers/acpi/property.c | 3 +++
> > drivers/pci/pci-acpi.c | 16
> > 2 files changed, 19 insertions(+)
> >
> > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > index d0986bda2964..20603cacc28d 100644
> > --- a/drivers/acpi/property.c
> > +++ b/drivers/acpi/property.c
> > @@ -48,6 +48,9 @@ static const guid_t prp_guids[] = {
> > /* Storage device needs D3 GUID: 5025030f-842f-4ab4-a561-99a5189762d0
> > */
> > GUID_INIT(0x5025030f, 0x842f, 0x4ab4,
> > 0xa5, 0x61, 0x99, 0xa5, 0x18, 0x97, 0x62, 0xd0),
> > + /* DmaProperty for PCI devices GUID:
> > 70d24161-6dd5-4c9e-8070-705531292865 */
> > + GUID_INIT(0x70d24161, 0x6dd5, 0x4c9e,
> > + 0x80, 0x70, 0x70, 0x55, 0x31, 0x29, 0x28, 0x65),
> > };
> >
> > /* ACPI _DSD data subnodes GUID: dbb8e3e6-5886-4ba6-8795-1319f52a966b */
> > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> > index 1f15ab7eabf8..378e05096c52 100644
> > --- a/drivers/pci/pci-acpi.c
> > +++ b/drivers/pci/pci-acpi.c
> > @@ -1350,12 +1350,28 @@ static void pci_acpi_set_external_facing(struct
> > pci_dev *dev)
> > dev->external_facing = 1;
> > }
> >
> > +static void pci_acpi_check_for_dma_protection(struct pci_dev *dev)
>
> I try to avoid function names like *_check_*() because they don't give
> any hint about whether there's a side effect or what direction things
> are going. I prefer things that return a value or make sense when
> used as a predicate. Maybe something like this?
>
> int pci_dev_has_dma_property(struct pci_dev *dev)
>
> dev->untrusted |= pci_dev_has_dma_property(pci_dev);
>
OK, will do.
> > +{
> > + u8 val;
> > +
> > + /*
> > + * Property also used by Microsoft Windows for same purpose,
> > + * (to implement DMA protection from a device, using the IOMMU).
> > +
[PATCH v5 2/2] PCI: Rename pci_dev->untrusted to pci_dev->untrusted_dma
Rename the field to make it more clear, that the device can execute DMA
attacks on the system, and thus the system may need protection from
such attacks from this device.
No functional change intended.
Signed-off-by: Rajat Jain
Reviewed-by: Mika Westerberg
---
v5: Use "untrusted_dma" as property name, based on feedback.
Reorder the patches in the series.
v4: Initial version, created based on comments on other patch
drivers/iommu/dma-iommu.c | 6 +++---
drivers/iommu/intel/iommu.c | 2 +-
drivers/iommu/iommu.c | 2 +-
drivers/pci/ats.c | 2 +-
drivers/pci/pci-acpi.c | 2 +-
drivers/pci/pci.c | 2 +-
drivers/pci/probe.c | 8
drivers/pci/quirks.c| 2 +-
include/linux/pci.h | 5 +++--
9 files changed, 16 insertions(+), 15 deletions(-)
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index d85d54f2b549..7cbe300fe907 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -497,14 +497,14 @@ static int iova_reserve_iommu_regions(struct device *dev,
return ret;
}
-static bool dev_is_untrusted(struct device *dev)
+static bool dev_has_untrusted_dma(struct device *dev)
{
- return dev_is_pci(dev) && to_pci_dev(dev)->untrusted;
+ return dev_is_pci(dev) && to_pci_dev(dev)->untrusted_dma;
}
static bool dev_use_swiotlb(struct device *dev)
{
- return IS_ENABLED(CONFIG_SWIOTLB) && dev_is_untrusted(dev);
+ return IS_ENABLED(CONFIG_SWIOTLB) && dev_has_untrusted_dma(dev);
}
/**
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 92fea3fbbb11..9246b7c9ab46 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -5570,7 +5570,7 @@ intel_iommu_enable_nesting(struct iommu_domain *domain)
*/
static bool risky_device(struct pci_dev *pdev)
{
- if (pdev->untrusted) {
+ if (pdev->untrusted_dma) {
pci_info(pdev,
"Skipping IOMMU quirk for dev [%04X:%04X] on untrusted
PCI link\n",
pdev->vendor, pdev->device);
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 8b86406b7162..79fb66af2e68 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1522,7 +1522,7 @@ static int iommu_get_def_domain_type(struct device *dev)
{
const struct iommu_ops *ops = dev->bus->iommu_ops;
- if (dev_is_pci(dev) && to_pci_dev(dev)->untrusted)
+ if (dev_is_pci(dev) && to_pci_dev(dev)->untrusted_dma)
return IOMMU_DOMAIN_DMA;
if (ops->def_domain_type)
diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index c967ad6e2626..477c16ba9341 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -42,7 +42,7 @@ bool pci_ats_supported(struct pci_dev *dev)
if (!dev->ats_cap)
return false;
- return (dev->untrusted == 0);
+ return (dev->untrusted_dma == 0);
}
EXPORT_SYMBOL_GPL(pci_ats_supported);
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 378e05096c52..1d5a284c3661 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -1362,7 +1362,7 @@ static void pci_acpi_check_for_dma_protection(struct
pci_dev *dev)
return;
if (val)
- dev->untrusted = 1;
+ dev->untrusted_dma = 1;
}
void pci_acpi_setup(struct device *dev, struct acpi_device *adev)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 9ecce435fb3f..1fb0eb8646c8 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -958,7 +958,7 @@ static void pci_std_enable_acs(struct pci_dev *dev)
ctrl |= (cap & PCI_ACS_UF);
/* Enable Translation Blocking for external devices and noats */
- if (pci_ats_disabled() || dev->external_facing || dev->untrusted)
+ if (pci_ats_disabled() || dev->external_facing || dev->untrusted_dma)
ctrl |= (cap & PCI_ACS_TB);
pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 17a969942d37..d2a9b26fcede 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1587,7 +1587,7 @@ static void set_pcie_thunderbolt(struct pci_dev *dev)
dev->is_thunderbolt = 1;
}
-static void set_pcie_untrusted(struct pci_dev *dev)
+static void pci_set_untrusted_dma(struct pci_dev *dev)
{
struct pci_dev *parent;
@@ -1596,8 +1596,8 @@ static void set_pcie_untrusted(struct pci_dev *dev)
* untrusted as well.
*/
parent = pci_upstream_bridge(dev);
- if (parent && (parent->untrusted || parent->external_facing))
- dev->untrusted = true;
+ if (parent && (parent->untrusted_dma || parent->external_facing))
+ dev->untrusted_dma = true;
}
static void pci_set_removable(struct pci_dev *dev)
@@ -1862,7 +1862,7 @@ int pci_setup_device(struct pci_dev *dev)
/* Need to have dev->cfg_size ready */
[PATCH v5 1/2] PCI: ACPI: Support Microsoft's "DmaProperty"
The "DmaProperty" is supported and documented by Microsoft here:
https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports
They use this property for DMA protection:
https://docs.microsoft.com/en-us/windows/security/information-protection/kernel-dma-protection-for-thunderbolt
Support the "DmaProperty" with the same semantics. This is useful for
internal PCI devices that do not hang off a PCIe rootport, but offer
an attack surface for DMA attacks (e.g. internal network devices).
Signed-off-by: Rajat Jain
Reviewed-by: Mika Westerberg
---
v5: * Reorder the patches in the series
v4: * Add the GUID.
* Update the comment and commitlog.
v3: * Use Microsoft's documented property "DmaProperty"
* Resctrict to ACPI only
drivers/acpi/property.c | 3 +++
drivers/pci/pci-acpi.c | 16
2 files changed, 19 insertions(+)
diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index d0986bda2964..20603cacc28d 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -48,6 +48,9 @@ static const guid_t prp_guids[] = {
/* Storage device needs D3 GUID: 5025030f-842f-4ab4-a561-99a5189762d0 */
GUID_INIT(0x5025030f, 0x842f, 0x4ab4,
0xa5, 0x61, 0x99, 0xa5, 0x18, 0x97, 0x62, 0xd0),
+ /* DmaProperty for PCI devices GUID:
70d24161-6dd5-4c9e-8070-705531292865 */
+ GUID_INIT(0x70d24161, 0x6dd5, 0x4c9e,
+ 0x80, 0x70, 0x70, 0x55, 0x31, 0x29, 0x28, 0x65),
};
/* ACPI _DSD data subnodes GUID: dbb8e3e6-5886-4ba6-8795-1319f52a966b */
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 1f15ab7eabf8..378e05096c52 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -1350,12 +1350,28 @@ static void pci_acpi_set_external_facing(struct pci_dev
*dev)
dev->external_facing = 1;
}
+static void pci_acpi_check_for_dma_protection(struct pci_dev *dev)
+{
+ u8 val;
+
+ /*
+* Property also used by Microsoft Windows for same purpose,
+* (to implement DMA protection from a device, using the IOMMU).
+*/
+ if (device_property_read_u8(>dev, "DmaProperty", ))
+ return;
+
+ 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_check_for_dma_protection(pci_dev);
pci_acpi_add_edr_notifier(pci_dev);
pci_acpi_add_pm_notifier(adev, pci_dev);
--
2.35.1.1021.g381101b075-goog
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 1/2] PCI: Rename "pci_dev->untrusted" to "pci_dev->poses_dma_risk"
On Tue, Mar 22, 2022 at 4:12 AM Rafael J. Wysocki wrote: > > On Tue, Mar 22, 2022 at 10:02 AM Christoph Hellwig wrote: > > > > On Sat, Mar 19, 2022 at 11:29:05PM -0700, Rajat Jain wrote: > > > Rename the field to make it more clear, that the device can execute DMA > > > attacks on the system, and thus the system may need protection from > > > such attacks from this device. > > > > > > No functional change intended. > > > > > > Signed-off-by: Rajat Jain > > > --- > > > v4: Initial version, created based on comments on other patch > > > > What a horrible name. Why not untrusted_dma which captures the > > intent much better? > > FWIW, I like this one much better too. Sure, no problems. I can change the name to "untrusted_dma". Mika, can I carry forward your "Reviewed-by" tag with this name change too? Thanks & Best Regards, Rajat ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v4 2/2] PCI: ACPI: Support Microsoft's "DmaProperty"
The "DmaProperty" is supported and documented by Microsoft here:
https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports
They use this property for DMA protection:
https://docs.microsoft.com/en-us/windows/security/information-protection/kernel-dma-protection-for-thunderbolt
Support the "DmaProperty" with the same semantics. This is useful for
internal PCI devices that do not hang off a PCIe rootport, but offer
an attack surface for DMA attacks (e.g. internal network devices).
Signed-off-by: Rajat Jain
---
v4: * Add the GUID.
* Use the (now) renamed property - pci_dev->poses_dma_risk)
* Update the comment and commitlog.
v3: * Use Microsoft's documented property "DmaProperty"
* Resctrict to ACPI only
drivers/acpi/property.c | 3 +++
drivers/pci/pci-acpi.c | 16
2 files changed, 19 insertions(+)
diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index d0986bda2964..20603cacc28d 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -48,6 +48,9 @@ static const guid_t prp_guids[] = {
/* Storage device needs D3 GUID: 5025030f-842f-4ab4-a561-99a5189762d0 */
GUID_INIT(0x5025030f, 0x842f, 0x4ab4,
0xa5, 0x61, 0x99, 0xa5, 0x18, 0x97, 0x62, 0xd0),
+ /* DmaProperty for PCI devices GUID:
70d24161-6dd5-4c9e-8070-705531292865 */
+ GUID_INIT(0x70d24161, 0x6dd5, 0x4c9e,
+ 0x80, 0x70, 0x70, 0x55, 0x31, 0x29, 0x28, 0x65),
};
/* ACPI _DSD data subnodes GUID: dbb8e3e6-5886-4ba6-8795-1319f52a966b */
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 1f15ab7eabf8..5360f1af2ed3 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -1350,12 +1350,28 @@ static void pci_acpi_set_external_facing(struct pci_dev
*dev)
dev->external_facing = 1;
}
+static void pci_acpi_check_for_dma_protection(struct pci_dev *dev)
+{
+ u8 val;
+
+ /*
+* Property also used by Microsoft Windows for same purpose,
+* (to implement DMA protection from a device, using the IOMMU).
+*/
+ if (device_property_read_u8(>dev, "DmaProperty", ))
+ return;
+
+ if (val)
+ dev->poses_dma_risk = 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_check_for_dma_protection(pci_dev);
pci_acpi_add_edr_notifier(pci_dev);
pci_acpi_add_pm_notifier(adev, pci_dev);
--
2.35.1.894.gb6a874cedc-goog
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v4 1/2] PCI: Rename "pci_dev->untrusted" to "pci_dev->poses_dma_risk"
Rename the field to make it more clear, that the device can execute DMA
attacks on the system, and thus the system may need protection from
such attacks from this device.
No functional change intended.
Signed-off-by: Rajat Jain
---
v4: Initial version, created based on comments on other patch
drivers/iommu/dma-iommu.c | 6 +++---
drivers/iommu/intel/iommu.c | 2 +-
drivers/iommu/iommu.c | 2 +-
drivers/pci/ats.c | 2 +-
drivers/pci/pci.c | 2 +-
drivers/pci/probe.c | 8
drivers/pci/quirks.c| 2 +-
include/linux/pci.h | 5 +++--
8 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index d85d54f2b549..ce10bfa86cf7 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -497,14 +497,14 @@ static int iova_reserve_iommu_regions(struct device *dev,
return ret;
}
-static bool dev_is_untrusted(struct device *dev)
+static bool dev_poses_dma_risk(struct device *dev)
{
- return dev_is_pci(dev) && to_pci_dev(dev)->untrusted;
+ return dev_is_pci(dev) && to_pci_dev(dev)->poses_dma_risk;
}
static bool dev_use_swiotlb(struct device *dev)
{
- return IS_ENABLED(CONFIG_SWIOTLB) && dev_is_untrusted(dev);
+ return IS_ENABLED(CONFIG_SWIOTLB) && dev_poses_dma_risk(dev);
}
/**
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 92fea3fbbb11..2e963a153c71 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -5570,7 +5570,7 @@ intel_iommu_enable_nesting(struct iommu_domain *domain)
*/
static bool risky_device(struct pci_dev *pdev)
{
- if (pdev->untrusted) {
+ if (pdev->poses_dma_risk) {
pci_info(pdev,
"Skipping IOMMU quirk for dev [%04X:%04X] on untrusted
PCI link\n",
pdev->vendor, pdev->device);
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 8b86406b7162..81433aab0245 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1522,7 +1522,7 @@ static int iommu_get_def_domain_type(struct device *dev)
{
const struct iommu_ops *ops = dev->bus->iommu_ops;
- if (dev_is_pci(dev) && to_pci_dev(dev)->untrusted)
+ if (dev_is_pci(dev) && to_pci_dev(dev)->poses_dma_risk)
return IOMMU_DOMAIN_DMA;
if (ops->def_domain_type)
diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index c967ad6e2626..6390fbeaaa02 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -42,7 +42,7 @@ bool pci_ats_supported(struct pci_dev *dev)
if (!dev->ats_cap)
return false;
- return (dev->untrusted == 0);
+ return (dev->poses_dma_risk == 0);
}
EXPORT_SYMBOL_GPL(pci_ats_supported);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 9ecce435fb3f..526d26f2011b 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -958,7 +958,7 @@ static void pci_std_enable_acs(struct pci_dev *dev)
ctrl |= (cap & PCI_ACS_UF);
/* Enable Translation Blocking for external devices and noats */
- if (pci_ats_disabled() || dev->external_facing || dev->untrusted)
+ if (pci_ats_disabled() || dev->external_facing || dev->poses_dma_risk)
ctrl |= (cap & PCI_ACS_TB);
pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 17a969942d37..7ae1ed312c47 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1587,7 +1587,7 @@ static void set_pcie_thunderbolt(struct pci_dev *dev)
dev->is_thunderbolt = 1;
}
-static void set_pcie_untrusted(struct pci_dev *dev)
+static void pci_check_if_dev_poses_dma_risk(struct pci_dev *dev)
{
struct pci_dev *parent;
@@ -1596,8 +1596,8 @@ static void set_pcie_untrusted(struct pci_dev *dev)
* untrusted as well.
*/
parent = pci_upstream_bridge(dev);
- if (parent && (parent->untrusted || parent->external_facing))
- dev->untrusted = true;
+ if (parent && (parent->poses_dma_risk || parent->external_facing))
+ dev->poses_dma_risk = true;
}
static void pci_set_removable(struct pci_dev *dev)
@@ -1862,7 +1862,7 @@ int pci_setup_device(struct pci_dev *dev)
/* Need to have dev->cfg_size ready */
set_pcie_thunderbolt(dev);
- set_pcie_untrusted(dev);
+ pci_check_if_dev_poses_dma_risk(dev);
/* "Unknown power state" */
dev->current_state = PCI_UNKNOWN;
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index d2dd6a6cda60..5c601c6c30bf 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -5122,7 +5122,7 @@ static int pci_quirk_enable_intel_spt_pch_acs(struct
pci_dev *dev)
ctrl |= (cap & PCI_ACS_CR);
ctrl |= (cap & PCI_ACS_UF);
- if (pci_ats_disabled() || dev->external_facing || dev->untrusted)
+ if (pci_ats_disabled() ||
Re: [PATCH v7 0/7] Fixes for dma-iommu swiotlb bounce buffers
I'm wondering why I don't see v7 on these patches on patchwork (these patches on https://lore.kernel.org/patchwork/project/lkml/list/?series==27643) ? On Sun, Aug 29, 2021 at 10:00 PM David Stevens wrote: > > This patch set includes various fixes for dma-iommu's swiotlb bounce > buffers for untrusted devices. > > The min_align_mask issue was found when running fio on an untrusted nvme > device with bs=512. The other issues were found via code inspection, so > I don't have any specific use cases where things were not working, nor > any concrete performance numbers. > > There are two issues related to min_align_mask that this patch series > does not attempt to fix. First, it does not address the case where > min_align_mask is larger than the IOVA granule. Doing so requires > changes to IOVA allocation, and is not specific to when swiotlb bounce > buffers are used. This is not a problem in practice today, since the > only driver which uses min_align_mask is nvme, which sets it to 4096. > > The second issue this series does not address is the fact that extra > swiotlb slots adjacent to a bounce buffer can be exposed to untrusted > devices whose drivers use min_align_mask. Fixing this requires being > able to allocate padding slots at the beginning of a swiotlb allocation. > This is a rather significant change that I am not comfortable making. > Without being able to handle this, there is also little point to > clearing the padding at the start of such a buffer, since we can only > clear based on (IO_TLB_SIZE - 1) instead of iova_mask. > > v6 -> v7: > - Remove unsafe attempt to clear padding at start of swiotlb buffer > - Rewrite commit message for min_align_mask commit to better explain >the problem it's fixing > - Rebase on iommu/core > - Acknowledge unsolved issues in cover letter > > v5 -> v6: > - Remove unnecessary line break > - Remove redundant config check > > v4 -> v5: > - Fix xen build error > - Move _swiotlb refactor into its own patch > > v3 -> v4: > - Fold _swiotlb functions into _page functions > - Add patch to align swiotlb buffer to iovad granule > - Combine if checks in iommu_dma_sync_sg_* functions > > v2 -> v3: > - Add new patch to address min_align_mask bug > - Set SKIP_CPU_SYNC flag after syncing in map/unmap > - Properly call arch_sync_dma_for_cpu in iommu_dma_sync_sg_for_cpu > > v1 -> v2: > - Split fixes into dedicated patches > - Less invasive changes to fix arch_sync when mapping > - Leave dev_is_untrusted check for strict iommu > > David Stevens (7): > dma-iommu: fix sync_sg with swiotlb > dma-iommu: fix arch_sync_dma for map > dma-iommu: skip extra sync during unmap w/swiotlb > dma-iommu: fold _swiotlb helpers into callers > dma-iommu: Check CONFIG_SWIOTLB more broadly > swiotlb: support aligned swiotlb buffers > dma-iommu: account for min_align_mask w/swiotlb > > drivers/iommu/dma-iommu.c | 188 +- > drivers/xen/swiotlb-xen.c | 2 +- > include/linux/swiotlb.h | 3 +- > kernel/dma/swiotlb.c | 11 ++- > 4 files changed, 93 insertions(+), 111 deletions(-) > > -- > 2.33.0.259.gc128427fd7-goog > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 24/25] iommu/dma: Factor out flush queue init
On Mon, Aug 9, 2021 at 12:59 PM Robin Murphy wrote:
>
> On 2021-08-09 20:05, Rajat Jain wrote:
> > On Wed, Aug 4, 2021 at 10:16 AM Robin Murphy wrote:
> >>
> >> Factor out flush queue setup from the initial domain init so that we
> >> can potentially trigger it from sysfs later on in a domain's lifetime.
> >>
> >> Reviewed-by: Lu Baolu
> >> Reviewed-by: John Garry
> >> Signed-off-by: Robin Murphy
> >> ---
> >> drivers/iommu/dma-iommu.c | 30 --
> >> include/linux/dma-iommu.h | 9 ++---
> >> 2 files changed, 26 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> >> index 2e19505dddf9..f51b8dc99ac6 100644
> >> --- a/drivers/iommu/dma-iommu.c
> >> +++ b/drivers/iommu/dma-iommu.c
> >> @@ -310,6 +310,25 @@ static bool dev_is_untrusted(struct device *dev)
> >> return dev_is_pci(dev) && to_pci_dev(dev)->untrusted;
> >> }
> >>
> >> +int iommu_dma_init_fq(struct iommu_domain *domain)
> >> +{
> >> + struct iommu_dma_cookie *cookie = domain->iova_cookie;
> >> +
> >> + if (domain->type != IOMMU_DOMAIN_DMA_FQ)
> >> + return -EINVAL;
> >> + if (cookie->fq_domain)
> >> + return 0;
> >> +
> >> + if (init_iova_flush_queue(>iovad,
> >> iommu_dma_flush_iotlb_all,
> >> + iommu_dma_entry_dtor)) {
> >> + pr_warn("iova flush queue initialization failed\n");
> >> + domain->type = IOMMU_DOMAIN_DMA;
> >> + return -ENODEV;
> >> + }
> >> + cookie->fq_domain = domain;
> >> + return 0;
> >> +}
> >> +
> >> /**
> >>* iommu_dma_init_domain - Initialise a DMA mapping domain
> >>* @domain: IOMMU domain previously prepared by iommu_get_dma_cookie()
> >> @@ -362,16 +381,7 @@ static int iommu_dma_init_domain(struct iommu_domain
> >> *domain, dma_addr_t base,
> >> }
> >>
> >> init_iova_domain(iovad, 1UL << order, base_pfn);
> >> -
> >> - if (domain->type == IOMMU_DOMAIN_DMA_FQ && !cookie->fq_domain) {
> >> - if (init_iova_flush_queue(iovad, iommu_dma_flush_iotlb_all,
> >> - iommu_dma_entry_dtor)) {
> >> - pr_warn("iova flush queue initialization
> >> failed\n");
> >> - domain->type = IOMMU_DOMAIN_DMA;
> >> - } else {
> >> - cookie->fq_domain = domain;
> >> - }
> >> - }
> >> + iommu_dma_init_fq(domain);
> >>
> >> return iova_reserve_iommu_regions(dev, domain);
> >> }
> >> diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
> >> index 758ca4694257..81ab647f1618 100644
> >> --- a/include/linux/dma-iommu.h
> >> +++ b/include/linux/dma-iommu.h
> >> @@ -20,6 +20,7 @@ void iommu_put_dma_cookie(struct iommu_domain *domain);
> >>
> >> /* Setup call for arch DMA mapping code */
> >> void iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64
> >> dma_limit);
> >> +int iommu_dma_init_fq(struct iommu_domain *domain);
> >>
> >> /* The DMA API isn't _quite_ the whole story, though... */
> >> /*
> >> @@ -37,9 +38,6 @@ void iommu_dma_compose_msi_msg(struct msi_desc *desc,
> >>
> >> void iommu_dma_get_resv_regions(struct device *dev, struct list_head
> >> *list);
> >>
> >> -void iommu_dma_free_cpu_cached_iovas(unsigned int cpu,
> >> - struct iommu_domain *domain);
> >> -
> >
> > This looks like an unrelated code cleanup. Should this be a separate patch?
>
> Ha, busted! Much of this was done in the "stream of consciousness" style
> where I made a big sprawling mess then split it up into patches and
> branches afterwards. TBH it was already feeling pretty tenuous having a
> separate patch just to move this one function, and it only gets more so
> with the simplification Will pointed out earlier. I think I'll squash
> iommu_dma_init_fq() into the next patch then do a thorough header sweep,
> since I've now spotted some things in iova.h which could probably go as
> well.
Thank you. I chanced upon this only because I've backported your
patchset (and some other changes that it depends on) to 5.10 which is
the kernel we currently use for our Intel platforms, and this cleanup
hunk was creating a problem (since 5.10 still uses the symbol you
removed). I'll be giving your v3 patchset a spin in my setup and
update you in case I see any issue.
Thanks,
Rajat
>
> Thanks for the poke!
>
> Robin.
>
> >
> > Thanks,
> >
> > Rajat
> >
> >
> >> extern bool iommu_dma_forcedac;
> >>
> >> #else /* CONFIG_IOMMU_DMA */
> >> @@ -54,6 +52,11 @@ static inline void iommu_setup_dma_ops(struct device
> >> *dev, u64 dma_base,
> >> {
> >> }
> >>
> >> +static inline int iommu_dma_init_fq(struct iommu_domain *domain)
> >> +{
> >> + return -EINVAL;
> >> +}
> >> +
> >> static inline int iommu_get_dma_cookie(struct iommu_domain *domain)
> >> {
> >> return -ENODEV;
>
Re: [PATCH v3 24/25] iommu/dma: Factor out flush queue init
On Wed, Aug 4, 2021 at 10:16 AM Robin Murphy wrote:
>
> Factor out flush queue setup from the initial domain init so that we
> can potentially trigger it from sysfs later on in a domain's lifetime.
>
> Reviewed-by: Lu Baolu
> Reviewed-by: John Garry
> Signed-off-by: Robin Murphy
> ---
> drivers/iommu/dma-iommu.c | 30 --
> include/linux/dma-iommu.h | 9 ++---
> 2 files changed, 26 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 2e19505dddf9..f51b8dc99ac6 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -310,6 +310,25 @@ static bool dev_is_untrusted(struct device *dev)
> return dev_is_pci(dev) && to_pci_dev(dev)->untrusted;
> }
>
> +int iommu_dma_init_fq(struct iommu_domain *domain)
> +{
> + struct iommu_dma_cookie *cookie = domain->iova_cookie;
> +
> + if (domain->type != IOMMU_DOMAIN_DMA_FQ)
> + return -EINVAL;
> + if (cookie->fq_domain)
> + return 0;
> +
> + if (init_iova_flush_queue(>iovad, iommu_dma_flush_iotlb_all,
> + iommu_dma_entry_dtor)) {
> + pr_warn("iova flush queue initialization failed\n");
> + domain->type = IOMMU_DOMAIN_DMA;
> + return -ENODEV;
> + }
> + cookie->fq_domain = domain;
> + return 0;
> +}
> +
> /**
> * iommu_dma_init_domain - Initialise a DMA mapping domain
> * @domain: IOMMU domain previously prepared by iommu_get_dma_cookie()
> @@ -362,16 +381,7 @@ static int iommu_dma_init_domain(struct iommu_domain
> *domain, dma_addr_t base,
> }
>
> init_iova_domain(iovad, 1UL << order, base_pfn);
> -
> - if (domain->type == IOMMU_DOMAIN_DMA_FQ && !cookie->fq_domain) {
> - if (init_iova_flush_queue(iovad, iommu_dma_flush_iotlb_all,
> - iommu_dma_entry_dtor)) {
> - pr_warn("iova flush queue initialization failed\n");
> - domain->type = IOMMU_DOMAIN_DMA;
> - } else {
> - cookie->fq_domain = domain;
> - }
> - }
> + iommu_dma_init_fq(domain);
>
> return iova_reserve_iommu_regions(dev, domain);
> }
> diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
> index 758ca4694257..81ab647f1618 100644
> --- a/include/linux/dma-iommu.h
> +++ b/include/linux/dma-iommu.h
> @@ -20,6 +20,7 @@ void iommu_put_dma_cookie(struct iommu_domain *domain);
>
> /* Setup call for arch DMA mapping code */
> void iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 dma_limit);
> +int iommu_dma_init_fq(struct iommu_domain *domain);
>
> /* The DMA API isn't _quite_ the whole story, though... */
> /*
> @@ -37,9 +38,6 @@ void iommu_dma_compose_msi_msg(struct msi_desc *desc,
>
> void iommu_dma_get_resv_regions(struct device *dev, struct list_head *list);
>
> -void iommu_dma_free_cpu_cached_iovas(unsigned int cpu,
> - struct iommu_domain *domain);
> -
This looks like an unrelated code cleanup. Should this be a separate patch?
Thanks,
Rajat
> extern bool iommu_dma_forcedac;
>
> #else /* CONFIG_IOMMU_DMA */
> @@ -54,6 +52,11 @@ static inline void iommu_setup_dma_ops(struct device *dev,
> u64 dma_base,
> {
> }
>
> +static inline int iommu_dma_init_fq(struct iommu_domain *domain)
> +{
> + return -EINVAL;
> +}
> +
> static inline int iommu_get_dma_cookie(struct iommu_domain *domain)
> {
> return -ENODEV;
> --
> 2.25.1
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/3] iommu: Enable non-strict DMA on QCom SD/MMC
Hi Rob, On Mon, Aug 2, 2021 at 5:09 PM Rajat Jain wrote: > > Hi Robin, Doug, > > On Wed, Jul 14, 2021 at 8:14 AM Doug Anderson wrote: > > > > Hi, > > > > On Tue, Jul 13, 2021 at 11:07 AM Robin Murphy wrote: > > > > > > On 2021-07-08 15:36, Doug Anderson wrote: > > > [...] > > > >> Or document for the users that want performance how to > > > >> change the setting, so that they can decide. > > > > > > > > Pushing this to the users can make sense for a Linux distribution but > > > > probably less sense for an embedded platform. So I'm happy to make > > > > some way for a user to override this (like via kernel command line), > > > > but I also strongly believe there should be a default that users don't > > > > have to futz with that we think is correct. > > > > > > FYI I did make progress on the "punt it to userspace" approach. I'm not > > > posting it even as an RFC yet because I still need to set up a machine > > > to try actually testing any of it (it's almost certainly broken > > > somewhere), but in the end it comes out looking surprisingly not too bad > > > overall. If you're curious to take a look in the meantime I put it here: > > > > > > https://gitlab.arm.com/linux-arm/linux-rm/-/commits/iommu/fq BTW, is there another mirror to this? I (and another colleague) are getting the following error when trying to clone it: rajatja@rajat2:~/rob_iommu$ git clone https://git.gitlab.arm.com/linux-arm/linux-rm.git Cloning into 'linux-rm'... remote: Enumerating objects: 125712, done. remote: Counting objects: 100% (125712/125712), done. remote: Compressing objects: 100% (41203/41203), done. error: RPC failed; curl 18 transfer closed with outstanding read data remaining error: 804 bytes of body are still expected fetch-pack: unexpected disconnect while reading sideband packet fatal: early EOF fatal: fetch-pack: invalid index-pack output rajatja@rajat2:~/rob_iommu$ We've tried both git and https methods. > > I was wondering if you got any closer to testing / sending it out? I > looked at the patches and am trying to understand, would they also > make it possible to convert at runtime, an existing "non-strict" > domain (for a particular device) into a "strict" domain leaving the > other devices/domains as-is? Please let me know when you think your > patches are good to be tested, and I'd also be interested in trying > them out. > > > > > Being able to change this at runtime through sysfs sounds great and it > > fills all the needs I'm aware of, thanks! In Chrome OS we can just use > > this with some udev rules and get everything we need. > > I still have another (inverse) use case where this does not work: > We have an Intel chromebook with the default domain type being > non-strict. There is an LTE modem (an internal PCI device which cannot > be marked external), which we'd like to be treated as a "Strict" DMA > domain. > > Do I understand it right that using Rob's patches, I could potentially > switch the domain to "strict" *after* booting (since we don't use > initramfs), but by that time, the driver might have already attached > to the modem device (using "non-strict" domain), and thus the damage > may have already been done? So perhaps we still need a device property > that the firmware could use to indicate "strictness" for certain > devices at boot? > > Thanks, > Rajat ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/3] iommu: Enable non-strict DMA on QCom SD/MMC
Hi Robin, Doug, On Wed, Jul 14, 2021 at 8:14 AM Doug Anderson wrote: > > Hi, > > On Tue, Jul 13, 2021 at 11:07 AM Robin Murphy wrote: > > > > On 2021-07-08 15:36, Doug Anderson wrote: > > [...] > > >> Or document for the users that want performance how to > > >> change the setting, so that they can decide. > > > > > > Pushing this to the users can make sense for a Linux distribution but > > > probably less sense for an embedded platform. So I'm happy to make > > > some way for a user to override this (like via kernel command line), > > > but I also strongly believe there should be a default that users don't > > > have to futz with that we think is correct. > > > > FYI I did make progress on the "punt it to userspace" approach. I'm not > > posting it even as an RFC yet because I still need to set up a machine > > to try actually testing any of it (it's almost certainly broken > > somewhere), but in the end it comes out looking surprisingly not too bad > > overall. If you're curious to take a look in the meantime I put it here: > > > > https://gitlab.arm.com/linux-arm/linux-rm/-/commits/iommu/fq I was wondering if you got any closer to testing / sending it out? I looked at the patches and am trying to understand, would they also make it possible to convert at runtime, an existing "non-strict" domain (for a particular device) into a "strict" domain leaving the other devices/domains as-is? Please let me know when you think your patches are good to be tested, and I'd also be interested in trying them out. > > Being able to change this at runtime through sysfs sounds great and it > fills all the needs I'm aware of, thanks! In Chrome OS we can just use > this with some udev rules and get everything we need. I still have another (inverse) use case where this does not work: We have an Intel chromebook with the default domain type being non-strict. There is an LTE modem (an internal PCI device which cannot be marked external), which we'd like to be treated as a "Strict" DMA domain. Do I understand it right that using Rob's patches, I could potentially switch the domain to "strict" *after* booting (since we don't use initramfs), but by that time, the driver might have already attached to the modem device (using "non-strict" domain), and thus the damage may have already been done? So perhaps we still need a device property that the firmware could use to indicate "strictness" for certain devices at boot? Thanks, Rajat ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 4/6] iommu: Combine device strictness requests with the global default
On Mon, Jun 21, 2021 at 4:53 PM Douglas Anderson wrote:
>
> In the patch ("drivers: base: Add bits to struct device to control
> iommu strictness") we add the ability for devices to tell us about
> their IOMMU strictness requirements. Let's now take that into account
> in the IOMMU layer.
>
> A few notes here:
> * Presumably this is always how iommu_get_dma_strict() was intended to
> behave. Had this not been the intention then it never would have
> taken a domain as a parameter.
> * The iommu_set_dma_strict() feels awfully non-symmetric now. That
> function sets the _default_ strictness globally in the system
> whereas iommu_get_dma_strict() returns the value for a given domain
> (falling back to the default). Presumably, at least, the fact that
> iommu_set_dma_strict() doesn't take a domain makes this obvious.
>
> The function iommu_get_dma_strict() should now make it super obvious
> where strictness comes from and who overides who. Though the function
> changed a bunch to make the logic clearer, the only two new rules
> should be:
> * Devices can force strictness for themselves, overriding the cmdline
> "iommu.strict=0" or a call to iommu_set_dma_strict(false)).
> * Devices can request non-strictness for themselves, assuming there
> was no cmdline "iommu.strict=1" or a call to
> iommu_set_dma_strict(true).
Along the same lines, I believe a platform (device tree / ACPI) should
also be able to have a say in this. I assume in your proposal, a
platform would expose a property in device tree which the device
driver would need to parse and then use it to set these bits in the
"struct device"?
Thanks,
Rajat
>
> Signed-off-by: Douglas Anderson
> ---
>
> drivers/iommu/iommu.c | 56 +--
> include/linux/iommu.h | 2 ++
> 2 files changed, 45 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 808ab70d5df5..0c84a4c06110 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -28,8 +28,19 @@
> static struct kset *iommu_group_kset;
> static DEFINE_IDA(iommu_group_ida);
>
> +enum iommu_strictness {
> + IOMMU_DEFAULT_STRICTNESS = -1,
> + IOMMU_NOT_STRICT = 0,
> + IOMMU_STRICT = 1,
> +};
> +static inline enum iommu_strictness bool_to_strictness(bool strictness)
> +{
> + return (enum iommu_strictness)strictness;
> +}
> +
> static unsigned int iommu_def_domain_type __read_mostly;
> -static bool iommu_dma_strict __read_mostly = true;
> +static enum iommu_strictness cmdline_dma_strict __read_mostly =
> IOMMU_DEFAULT_STRICTNESS;
> +static enum iommu_strictness driver_dma_strict __read_mostly =
> IOMMU_DEFAULT_STRICTNESS;
> static u32 iommu_cmd_line __read_mostly;
>
> struct iommu_group {
> @@ -69,7 +80,6 @@ static const char * const iommu_group_resv_type_string[] = {
> };
>
> #define IOMMU_CMD_LINE_DMA_API BIT(0)
> -#define IOMMU_CMD_LINE_STRICT BIT(1)
>
> static int iommu_alloc_default_domain(struct iommu_group *group,
> struct device *dev);
> @@ -336,25 +346,38 @@ early_param("iommu.passthrough",
> iommu_set_def_domain_type);
>
> static int __init iommu_dma_setup(char *str)
> {
> - int ret = kstrtobool(str, _dma_strict);
> + bool strict;
> + int ret = kstrtobool(str, );
>
> if (!ret)
> - iommu_cmd_line |= IOMMU_CMD_LINE_STRICT;
> + cmdline_dma_strict = bool_to_strictness(strict);
> return ret;
> }
> early_param("iommu.strict", iommu_dma_setup);
>
> void iommu_set_dma_strict(bool strict)
> {
> - if (strict || !(iommu_cmd_line & IOMMU_CMD_LINE_STRICT))
> - iommu_dma_strict = strict;
> + /* A driver can request strictness but not the other way around */
> + if (driver_dma_strict != IOMMU_STRICT)
> + driver_dma_strict = bool_to_strictness(strict);
> }
>
> bool iommu_get_dma_strict(struct iommu_domain *domain)
> {
> - /* only allow lazy flushing for DMA domains */
> - if (domain->type == IOMMU_DOMAIN_DMA)
> - return iommu_dma_strict;
> + /* Non-DMA domains or anyone forcing it to strict makes it strict */
> + if (domain->type != IOMMU_DOMAIN_DMA ||
> + cmdline_dma_strict == IOMMU_STRICT ||
> + driver_dma_strict == IOMMU_STRICT ||
> + domain->force_strict)
> + return true;
> +
> + /* Anyone requesting non-strict (if no forces) makes it non-strict */
> + if (cmdline_dma_strict == IOMMU_NOT_STRICT ||
> + driver_dma_strict == IOMMU_NOT_STRICT ||
> + domain->request_non_strict)
> + return false;
> +
> + /* Nobody said anything, so it's strict by default */
> return true;
> }
> EXPORT_SYMBOL_GPL(iommu_get_dma_strict);
> @@ -1519,7 +1542,8 @@ static int iommu_get_def_domain_type(struct device *dev)
>
> static int iommu_group_alloc_default_domain(struct
[PATCH] pci: Rename pci_dev->untrusted to pci_dev->external
The current flag name "untrusted" is not correct as it is populated
using the firmware property "external-facing" for the parent ports. In
other words, the firmware only says which ports are external facing, so
the field really identifies the devices as external (vs internal).
Only field renaming. No functional change intended.
Signed-off-by: Rajat Jain
---
drivers/iommu/dma-iommu.c | 2 +-
drivers/iommu/intel/iommu.c | 2 +-
drivers/iommu/iommu.c | 2 +-
drivers/pci/ats.c | 2 +-
drivers/pci/pci.c | 2 +-
drivers/pci/probe.c | 12 ++--
drivers/pci/quirks.c| 2 +-
include/linux/pci.h | 9 +
8 files changed, 13 insertions(+), 20 deletions(-)
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index af765c813cc8..14769c213f30 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -313,7 +313,7 @@ static void iommu_dma_flush_iotlb_all(struct iova_domain
*iovad)
static bool dev_is_untrusted(struct device *dev)
{
- return dev_is_pci(dev) && to_pci_dev(dev)->untrusted;
+ return dev_is_pci(dev) && to_pci_dev(dev)->external;
}
/**
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index ee0932307d64..965cc78e0dde 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -5511,7 +5511,7 @@ intel_iommu_domain_get_attr(struct iommu_domain *domain,
*/
static bool risky_device(struct pci_dev *pdev)
{
- if (pdev->untrusted) {
+ if (pdev->external) {
pci_info(pdev,
"Skipping IOMMU quirk for dev [%04X:%04X] on untrusted
PCI link\n",
pdev->vendor, pdev->device);
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index d0b0a15dba84..abdedc98f3c0 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1482,7 +1482,7 @@ static int iommu_get_def_domain_type(struct device *dev)
{
const struct iommu_ops *ops = dev->bus->iommu_ops;
- if (dev_is_pci(dev) && to_pci_dev(dev)->untrusted)
+ if (dev_is_pci(dev) && to_pci_dev(dev)->external)
return IOMMU_DOMAIN_DMA;
if (ops->def_domain_type)
diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index 0d3719407b8b..0d17fb4a15bf 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -42,7 +42,7 @@ bool pci_ats_supported(struct pci_dev *dev)
if (!dev->ats_cap)
return false;
- return (dev->untrusted == 0);
+ return (dev->external == 0);
}
EXPORT_SYMBOL_GPL(pci_ats_supported);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 16a17215f633..ec98892389d7 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -886,7 +886,7 @@ static void pci_std_enable_acs(struct pci_dev *dev)
ctrl |= (cap & PCI_ACS_UF);
/* Enable Translation Blocking for external devices */
- if (dev->external_facing || dev->untrusted)
+ if (dev->external_facing || dev->external)
ctrl |= (cap & PCI_ACS_TB);
pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 953f15abc850..ae4800e82914 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1562,17 +1562,17 @@ static void set_pcie_thunderbolt(struct pci_dev *dev)
}
}
-static void set_pcie_untrusted(struct pci_dev *dev)
+static void set_pcie_external(struct pci_dev *dev)
{
struct pci_dev *parent;
/*
-* If the upstream bridge is untrusted we treat this device
-* untrusted as well.
+* If the upstream bridge is external we mark this device
+* external as well.
*/
parent = pci_upstream_bridge(dev);
- if (parent && (parent->untrusted || parent->external_facing))
- dev->untrusted = true;
+ if (parent && (parent->external || parent->external_facing))
+ dev->external = true;
}
/**
@@ -1814,7 +1814,7 @@ int pci_setup_device(struct pci_dev *dev)
/* Need to have dev->cfg_size ready */
set_pcie_thunderbolt(dev);
- set_pcie_untrusted(dev);
+ set_pcie_external(dev);
/* "Unknown power state" */
dev->current_state = PCI_UNKNOWN;
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 653660e3ba9e..1054b7d9ee89 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -4942,7 +4942,7 @@ 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)
+ if (dev->external_facing || dev->external)
ctrl |= (cap & PCI_ACS_TB);
pci_write_config_dword(dev, pos + INTEL_SPT_ACS_CTRL, ctrl);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 86c799c97b77..a535e2c2b690 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -431,14 +431,7 @@ struct pci_dev
Re: [bugzilla-dae...@bugzilla.kernel.org: [Bug 209149] New: "iommu/vt-d: Enable PCI ACS for platform opt in hint" makes NVMe config space not accessible after S3]
On Wed, Sep 23, 2020 at 9:19 AM Raj, Ashok wrote:
>
> Hi Bjorn
>
>
> On Wed, Sep 23, 2020 at 11:03:27AM -0500, Bjorn Helgaas wrote:
> > [+cc IOMMU and NVMe folks]
> >
> > Sorry, I forgot to forward this to linux-pci when it was first
> > reported.
> >
> > Apparently this happens with v5.9-rc3, and may be related to
> > 50310600ebda ("iommu/vt-d: Enable PCI ACS for platform opt in hint"),
> > which appeared in v5.8-rc3.
> >
> > There are several dmesg logs and proposed patches in the bugzilla, but
> > no analysis yet of what the problem is. From the first dmesg
> > attachment (https://bugzilla.kernel.org/attachment.cgi?id=292327):
>
> We have been investigating this internally as well. It appears maybe the
> specupdate for Cometlake is missing the errata documention. The offsets
> were wrong in some of them, and if its the same issue its likely cause.
Can you please also confirm if errata applies to Tigerlake ?
Thanks,
Rajat
>
> Will nudge the hw folks to hunt that down :-(.
>
> Cheers,
> Ashok
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 4/4] PCI/ACS: Enable PCI_ACS_TB for untrusted/external-facing devices
Hello Bjorn,
On Tue, Jul 14, 2020 at 1:19 PM Rajat Jain wrote:
>
> On Sat, Jul 11, 2020 at 12:53 PM Bjorn Helgaas wrote:
> >
> > On Fri, Jul 10, 2020 at 03:53:59PM -0700, Rajat Jain wrote:
> > > On Fri, Jul 10, 2020 at 2:29 PM Raj, Ashok wrote:
> > > > On Fri, Jul 10, 2020 at 03:29:22PM -0500, Bjorn Helgaas wrote:
> > > > > On Tue, Jul 07, 2020 at 03:46:04PM -0700, Rajat Jain wrote:
> > > > > > When enabling ACS, enable translation blocking for external facing
> > > > > > ports
> > > > > > and untrusted devices.
> > > > > >
> > > > > > Signed-off-by: Rajat Jain
> > > > > > ---
> > > > > > v4: Add braces to avoid warning from kernel robot
> > > > > > print warning for only external-facing devices.
> > > > > > v3: print warning if ACS_TB not supported on
> > > > > > external-facing/untrusted ports.
> > > > > > Minor code comments fixes.
> > > > > > v2: Commit log change
> > > > > >
> > > > > > drivers/pci/pci.c| 8
> > > > > > drivers/pci/quirks.c | 15 +++
> > > > > > 2 files changed, 23 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > > > index 73a8627822140..a5a6bea7af7ce 100644
> > > > > > --- a/drivers/pci/pci.c
> > > > > > +++ b/drivers/pci/pci.c
> > > > > > @@ -876,6 +876,14 @@ static void pci_std_enable_acs(struct pci_dev
> > > > > > *dev)
> > > > > > /* Upstream Forwarding */
> > > > > > ctrl |= (cap & PCI_ACS_UF);
> > > > > >
> > > > > > + /* Enable Translation Blocking for external devices */
> > > > > > + if (dev->external_facing || dev->untrusted) {
> > > > > > + if (cap & PCI_ACS_TB)
> > > > > > + ctrl |= PCI_ACS_TB;
> > > > > > + else if (dev->external_facing)
> > > > > > + pci_warn(dev, "ACS: No Translation Blocking on
> > > > > > external-facing dev\n");
> > > > > > + }
> > > > >
> > > > > IIUC, this means that external devices can *never* use ATS and
> > > > > can never cache translations.
> > >
> > > Yes, but it already exists today (and this patch doesn't change that):
> > > 521376741b2c2 "PCI/ATS: Only enable ATS for trusted devices"
> > >
> > > IMHO any external device trying to send ATS traffic despite having ATS
> > > disabled should count as a bad intent. And this patch is trying to
> > > plug that loophole, by blocking the AT traffic from devices that we do
> > > not expect to see AT from anyway.
> >
> > Thinking about this some more, I wonder if Linux should:
> >
> > - Explicitly disable ATS for every device at enumeration-time, e.g.,
> > in pci_init_capabilities(),
> >
> > - Enable PCI_ACS_TB for every device (not just external-facing or
> > untrusted ones),
> >
> > - Disable PCI_ACS_TB for the relevant devices along the path only
> > when enabling ATS.
> >
> > One nice thing about doing that is that the "untrusted" test would be
> > only in pci_enable_ats(), and we wouldn't need one in
> > pci_std_enable_acs().
>
> Sent out v5 with this approach here:
> https://patchwork.kernel.org/patch/11663515/
Any feedback on the patch above? It has been waiting for feedback
Thanks & Best Regards,,
Rajat
>
> Thanks,
>
> Rajat
>
> >
> >
> > It's possible BIOS gives us devices with ATS enabled, and this might
> > break them, but that seems like something we'd want to find out about.
> >
> > Bjorn
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5] PCI/ACS: Enable PCI_ACS_TB and disable only when needed for ATS
Hello Bjorn,
On Mon, Aug 17, 2020 at 3:50 PM Rajat Jain wrote:
>
> Hello Bjorn,
>
>
> On Sat, Aug 1, 2020 at 5:30 PM Rajat Jain wrote:
> >
> > Hi Bjorn,
> >
> >
> > On Tue, Jul 14, 2020 at 1:24 PM Rajat Jain wrote:
> > >
> > > On Tue, Jul 14, 2020 at 1:15 PM Rajat Jain wrote:
> > > >
> > > > The ACS "Translation Blocking" bit blocks the translated addresses from
> > > > the devices. We don't expect such traffic from devices unless ATS is
> > > > enabled on them. A device sending such traffic without ATS enabled,
> > > > indicates malicious intent, and thus should be blocked.
> > > >
> > > > Enable PCI_ACS_TB by default for all devices, and it stays enabled until
> > > > atleast one of the devices downstream wants to enable ATS. It gets
> > > > disabled to enable ATS on a device downstream it, and then gets enabled
> > > > back on once all the downstream devices don't need ATS.
> > > >
> > > > Signed-off-by: Rajat Jain
> >
> > Just checking to see if you got a chance to look at this V5 patch.
>
> Any feedback on this patch?
Gentle reminder?
Thanks & Best Regards,
Rajat
>
> Thanks & Best Regards,
>
> Rajat
>
> >
> > Thanks & Best Regards,
> >
> > Rajat
> >
> > > > ---
> > > > Note that I'm ignoring the devices that require quirks to enable or
> > > > disable ACS, instead of using the standard way for ACS configuration.
> > > > The reason is that it would require adding yet another quirk table or
> > > > quirk function pointer, that I don't know how to implement for those
> > > > devices, and will neither have the devices to test that code.
> > > >
> > > > v5: Enable TB and disable ATS for all devices on boot. Disable TB later
> > > > only if needed to enable ATS on downstream devices.
> > > > v4: Add braces to avoid warning from kernel robot
> > > > print warning for only external-facing devices.
> > > > v3: print warning if ACS_TB not supported on external-facing/untrusted
> > > > ports.
> > > > Minor code comments fixes.
> > > > v2: Commit log change
> > > >
> > > > drivers/pci/ats.c | 5
> > > > drivers/pci/pci.c | 57 +
> > > > drivers/pci/pci.h | 2 ++
> > > > drivers/pci/probe.c | 2 +-
> > > > include/linux/pci.h | 2 ++
> > > > 5 files changed, 67 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> > > > index b761c1f72f67..e2ea9083f30f 100644
> > > > --- a/drivers/pci/ats.c
> > > > +++ b/drivers/pci/ats.c
> > > > @@ -28,6 +28,9 @@ void pci_ats_init(struct pci_dev *dev)
> > > > return;
> > > >
> > > > dev->ats_cap = pos;
> > > > +
> > > > + dev->ats_enabled = 1; /* To avoid WARN_ON from
> > > > pci_disable_ats() */
> > > > + pci_disable_ats(dev);
> > > > }
> > > >
> > > > /**
> > > > @@ -82,6 +85,7 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
> > > > }
> > > > pci_write_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, ctrl);
> > > >
> > > > + pci_disable_acs_trans_blocking(dev);
> > > > dev->ats_enabled = 1;
> > > > return 0;
> > > > }
> > > > @@ -102,6 +106,7 @@ void pci_disable_ats(struct pci_dev *dev)
> > > > ctrl &= ~PCI_ATS_CTRL_ENABLE;
> > > > pci_write_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, ctrl);
> > > >
> > > > + pci_enable_acs_trans_blocking(dev);
> > > > dev->ats_enabled = 0;
> > > > }
> > > > EXPORT_SYMBOL_GPL(pci_disable_ats);
> > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > index 73a862782214..614e3c1e8c56 100644
> > > > --- a/drivers/pci/pci.c
> > > > +++ b/drivers/pci/pci.c
> > > > @@ -876,6 +876,9 @@ static void pci_std_enable_acs(struct pci_dev *dev)
> > > > /* Upstream Forwarding */
> > > > ctrl |= (cap & PCI_ACS_UF);
> > > >
> > > > + /* Translation Blocking */
> > > > + ctrl |= (cap & PCI_ACS_TB);
> > > > +
> > > > pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
> > > > }
> > > >
> > > > @@ -904,6 +907,60 @@ static void pci_enable_acs(struct pci_dev *dev)
> > > > pci_disable_acs_redir(dev);
> > > > }
> > > >
> > > > +void pci_disable_acs_trans_blocking(struct pci_dev *pdev)
> > > > +{
> > > > + u16 cap, ctrl, pos;
> > > > + struct pci_dev *dev;
> > > > +
> > > > + if (!pci_acs_enable)
> > > > + return;
> > > > +
> > > > + for (dev = pdev; dev; dev = pci_upstream_bridge(pdev)) {
> > > > +
> > > > + pos = dev->acs_cap;
> > > > + if (!pos)
> > > > + continue;
> > > > +
> > > > + /*
> > > > +* Disable translation blocking when first downstream
> > > > +* device that needs it (for ATS) wants to enable ATS
> > > > +*/
> > > > + if (++dev->ats_dependencies == 1) {
> > >
> > > I am a little worried about a potential race condition here. I know
> > > that 2 PCI devices cannot be enumerating at the same time. Do
Re: [PATCH v5] PCI/ACS: Enable PCI_ACS_TB and disable only when needed for ATS
Hi Bjorn,
On Tue, Jul 14, 2020 at 1:24 PM Rajat Jain wrote:
>
> On Tue, Jul 14, 2020 at 1:15 PM Rajat Jain wrote:
> >
> > The ACS "Translation Blocking" bit blocks the translated addresses from
> > the devices. We don't expect such traffic from devices unless ATS is
> > enabled on them. A device sending such traffic without ATS enabled,
> > indicates malicious intent, and thus should be blocked.
> >
> > Enable PCI_ACS_TB by default for all devices, and it stays enabled until
> > atleast one of the devices downstream wants to enable ATS. It gets
> > disabled to enable ATS on a device downstream it, and then gets enabled
> > back on once all the downstream devices don't need ATS.
> >
> > Signed-off-by: Rajat Jain
Just checking to see if you got a chance to look at this V5 patch.
Thanks & Best Regards,
Rajat
> > ---
> > Note that I'm ignoring the devices that require quirks to enable or
> > disable ACS, instead of using the standard way for ACS configuration.
> > The reason is that it would require adding yet another quirk table or
> > quirk function pointer, that I don't know how to implement for those
> > devices, and will neither have the devices to test that code.
> >
> > v5: Enable TB and disable ATS for all devices on boot. Disable TB later
> > only if needed to enable ATS on downstream devices.
> > v4: Add braces to avoid warning from kernel robot
> > print warning for only external-facing devices.
> > v3: print warning if ACS_TB not supported on external-facing/untrusted
> > ports.
> > Minor code comments fixes.
> > v2: Commit log change
> >
> > drivers/pci/ats.c | 5
> > drivers/pci/pci.c | 57 +
> > drivers/pci/pci.h | 2 ++
> > drivers/pci/probe.c | 2 +-
> > include/linux/pci.h | 2 ++
> > 5 files changed, 67 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> > index b761c1f72f67..e2ea9083f30f 100644
> > --- a/drivers/pci/ats.c
> > +++ b/drivers/pci/ats.c
> > @@ -28,6 +28,9 @@ void pci_ats_init(struct pci_dev *dev)
> > return;
> >
> > dev->ats_cap = pos;
> > +
> > + dev->ats_enabled = 1; /* To avoid WARN_ON from pci_disable_ats() */
> > + pci_disable_ats(dev);
> > }
> >
> > /**
> > @@ -82,6 +85,7 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
> > }
> > pci_write_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, ctrl);
> >
> > + pci_disable_acs_trans_blocking(dev);
> > dev->ats_enabled = 1;
> > return 0;
> > }
> > @@ -102,6 +106,7 @@ void pci_disable_ats(struct pci_dev *dev)
> > ctrl &= ~PCI_ATS_CTRL_ENABLE;
> > pci_write_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, ctrl);
> >
> > + pci_enable_acs_trans_blocking(dev);
> > dev->ats_enabled = 0;
> > }
> > EXPORT_SYMBOL_GPL(pci_disable_ats);
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 73a862782214..614e3c1e8c56 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -876,6 +876,9 @@ static void pci_std_enable_acs(struct pci_dev *dev)
> > /* Upstream Forwarding */
> > ctrl |= (cap & PCI_ACS_UF);
> >
> > + /* Translation Blocking */
> > + ctrl |= (cap & PCI_ACS_TB);
> > +
> > pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
> > }
> >
> > @@ -904,6 +907,60 @@ static void pci_enable_acs(struct pci_dev *dev)
> > pci_disable_acs_redir(dev);
> > }
> >
> > +void pci_disable_acs_trans_blocking(struct pci_dev *pdev)
> > +{
> > + u16 cap, ctrl, pos;
> > + struct pci_dev *dev;
> > +
> > + if (!pci_acs_enable)
> > + return;
> > +
> > + for (dev = pdev; dev; dev = pci_upstream_bridge(pdev)) {
> > +
> > + pos = dev->acs_cap;
> > + if (!pos)
> > + continue;
> > +
> > + /*
> > +* Disable translation blocking when first downstream
> > +* device that needs it (for ATS) wants to enable ATS
> > +*/
> > + if (++dev->ats_dependencies == 1) {
>
> I am a little worried about a potential race condition here. I know
> that 2 PCI devices cannot be enumerating at the same time. Do we know
> if multiple pci_enable_ats() and pci_disable_ats() function calls can
> be simultaneously executing (even for different devices)? If so, we
> may need an atomic_t variable for ats_dependencies.
>
> Thanks,
>
> Rajat
>
>
> > + pci_read_config_word(dev, pos + PCI_ACS_CAP, );
> > + pci_read_config_word(dev, pos + PCI_ACS_CTRL,
> > );
> > + ctrl &= ~(cap & PCI_ACS_TB);
> > + pci_write_config_word(dev, pos + PCI_ACS_CTRL,
> > ctrl);
> > + }
> > + }
> > +}
> > +
> > +void pci_enable_acs_trans_blocking(struct pci_dev *pdev)
> > +{
> > + u16 cap, ctrl, pos;
> > + struct pci_dev *dev;
> > +
> > + if
Re: [PATCH v5] PCI/ACS: Enable PCI_ACS_TB and disable only when needed for ATS
Hi Bjorn,
On Tue, Jul 14, 2020 at 1:15 PM Rajat Jain wrote:
> The ACS "Translation Blocking" bit blocks the translated addresses from
> the devices. We don't expect such traffic from devices unless ATS is
> enabled on them. A device sending such traffic without ATS enabled,
> indicates malicious intent, and thus should be blocked.
>
> Enable PCI_ACS_TB by default for all devices, and it stays enabled until
> atleast one of the devices downstream wants to enable ATS. It gets
> disabled to enable ATS on a device downstream it, and then gets enabled
> back on once all the downstream devices don't need ATS.
>
> Signed-off-by: Rajat Jain
>
Just checking to see if you got a chance to look at this V5 patch.
Thanks & Best Regards,
Rajat
> ---
> Note that I'm ignoring the devices that require quirks to enable or
> disable ACS, instead of using the standard way for ACS configuration.
> The reason is that it would require adding yet another quirk table or
> quirk function pointer, that I don't know how to implement for those
> devices, and will neither have the devices to test that code.
>
> v5: Enable TB and disable ATS for all devices on boot. Disable TB later
> only if needed to enable ATS on downstream devices.
> v4: Add braces to avoid warning from kernel robot
> print warning for only external-facing devices.
> v3: print warning if ACS_TB not supported on external-facing/untrusted
> ports.
> Minor code comments fixes.
> v2: Commit log change
>
> drivers/pci/ats.c | 5
> drivers/pci/pci.c | 57 +
> drivers/pci/pci.h | 2 ++
> drivers/pci/probe.c | 2 +-
> include/linux/pci.h | 2 ++
> 5 files changed, 67 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index b761c1f72f67..e2ea9083f30f 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -28,6 +28,9 @@ void pci_ats_init(struct pci_dev *dev)
> return;
>
> dev->ats_cap = pos;
> +
> + dev->ats_enabled = 1; /* To avoid WARN_ON from pci_disable_ats() */
> + pci_disable_ats(dev);
> }
>
> /**
> @@ -82,6 +85,7 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
> }
> pci_write_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, ctrl);
>
> + pci_disable_acs_trans_blocking(dev);
> dev->ats_enabled = 1;
> return 0;
> }
> @@ -102,6 +106,7 @@ void pci_disable_ats(struct pci_dev *dev)
> ctrl &= ~PCI_ATS_CTRL_ENABLE;
> pci_write_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, ctrl);
>
> + pci_enable_acs_trans_blocking(dev);
> dev->ats_enabled = 0;
> }
> EXPORT_SYMBOL_GPL(pci_disable_ats);
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 73a862782214..614e3c1e8c56 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -876,6 +876,9 @@ static void pci_std_enable_acs(struct pci_dev *dev)
> /* Upstream Forwarding */
> ctrl |= (cap & PCI_ACS_UF);
>
> + /* Translation Blocking */
> + ctrl |= (cap & PCI_ACS_TB);
> +
> pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
> }
>
> @@ -904,6 +907,60 @@ static void pci_enable_acs(struct pci_dev *dev)
> pci_disable_acs_redir(dev);
> }
>
> +void pci_disable_acs_trans_blocking(struct pci_dev *pdev)
> +{
> + u16 cap, ctrl, pos;
> + struct pci_dev *dev;
> +
> + if (!pci_acs_enable)
> + return;
> +
> + for (dev = pdev; dev; dev = pci_upstream_bridge(pdev)) {
> +
> + pos = dev->acs_cap;
> + if (!pos)
> + continue;
> +
> + /*
> +* Disable translation blocking when first downstream
> +* device that needs it (for ATS) wants to enable ATS
> +*/
> + if (++dev->ats_dependencies == 1) {
> + pci_read_config_word(dev, pos + PCI_ACS_CAP, );
> + pci_read_config_word(dev, pos + PCI_ACS_CTRL,
> );
> + ctrl &= ~(cap & PCI_ACS_TB);
> + pci_write_config_word(dev, pos + PCI_ACS_CTRL,
> ctrl);
> + }
> + }
> +}
> +
> +void pci_enable_acs_trans_blocking(struct pci_dev *pdev)
> +{
> + u16 cap, ctrl, pos;
> + struct pci_dev *dev;
> +
> + if (!pci_acs_enable)
> + return;
> +
> + for (dev = pdev; dev; dev = pci_upstream_bridge(pdev)) {
> +
> + pos = dev->acs_cap;
> + if (!pos)
> + continue;
> +
> + /*
> +* Enable translation blocking when last downstream device
> +* that depends on it (for ATS), doesn't need ATS anymore
> +*/
> + if (--dev->ats_dependencies == 0) {
> + pci_read_config_word(dev, pos + PCI_ACS_CAP, );
> + pci_read_config_word(dev, pos + PCI_ACS_CTRL,
> );
> + ctrl |=
[PATCH v5] PCI/ACS: Enable PCI_ACS_TB and disable only when needed for ATS
The ACS "Translation Blocking" bit blocks the translated addresses from
the devices. We don't expect such traffic from devices unless ATS is
enabled on them. A device sending such traffic without ATS enabled,
indicates malicious intent, and thus should be blocked.
Enable PCI_ACS_TB by default for all devices, and it stays enabled until
atleast one of the devices downstream wants to enable ATS. It gets
disabled to enable ATS on a device downstream it, and then gets enabled
back on once all the downstream devices don't need ATS.
Signed-off-by: Rajat Jain
---
Note that I'm ignoring the devices that require quirks to enable or
disable ACS, instead of using the standard way for ACS configuration.
The reason is that it would require adding yet another quirk table or
quirk function pointer, that I don't know how to implement for those
devices, and will neither have the devices to test that code.
v5: Enable TB and disable ATS for all devices on boot. Disable TB later
only if needed to enable ATS on downstream devices.
v4: Add braces to avoid warning from kernel robot
print warning for only external-facing devices.
v3: print warning if ACS_TB not supported on external-facing/untrusted ports.
Minor code comments fixes.
v2: Commit log change
drivers/pci/ats.c | 5
drivers/pci/pci.c | 57 +
drivers/pci/pci.h | 2 ++
drivers/pci/probe.c | 2 +-
include/linux/pci.h | 2 ++
5 files changed, 67 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index b761c1f72f67..e2ea9083f30f 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -28,6 +28,9 @@ void pci_ats_init(struct pci_dev *dev)
return;
dev->ats_cap = pos;
+
+ dev->ats_enabled = 1; /* To avoid WARN_ON from pci_disable_ats() */
+ pci_disable_ats(dev);
}
/**
@@ -82,6 +85,7 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
}
pci_write_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, ctrl);
+ pci_disable_acs_trans_blocking(dev);
dev->ats_enabled = 1;
return 0;
}
@@ -102,6 +106,7 @@ void pci_disable_ats(struct pci_dev *dev)
ctrl &= ~PCI_ATS_CTRL_ENABLE;
pci_write_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, ctrl);
+ pci_enable_acs_trans_blocking(dev);
dev->ats_enabled = 0;
}
EXPORT_SYMBOL_GPL(pci_disable_ats);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 73a862782214..614e3c1e8c56 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -876,6 +876,9 @@ static void pci_std_enable_acs(struct pci_dev *dev)
/* Upstream Forwarding */
ctrl |= (cap & PCI_ACS_UF);
+ /* Translation Blocking */
+ ctrl |= (cap & PCI_ACS_TB);
+
pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
}
@@ -904,6 +907,60 @@ static void pci_enable_acs(struct pci_dev *dev)
pci_disable_acs_redir(dev);
}
+void pci_disable_acs_trans_blocking(struct pci_dev *pdev)
+{
+ u16 cap, ctrl, pos;
+ struct pci_dev *dev;
+
+ if (!pci_acs_enable)
+ return;
+
+ for (dev = pdev; dev; dev = pci_upstream_bridge(pdev)) {
+
+ pos = dev->acs_cap;
+ if (!pos)
+ continue;
+
+ /*
+* Disable translation blocking when first downstream
+* device that needs it (for ATS) wants to enable ATS
+*/
+ if (++dev->ats_dependencies == 1) {
+ pci_read_config_word(dev, pos + PCI_ACS_CAP, );
+ pci_read_config_word(dev, pos + PCI_ACS_CTRL, );
+ ctrl &= ~(cap & PCI_ACS_TB);
+ pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
+ }
+ }
+}
+
+void pci_enable_acs_trans_blocking(struct pci_dev *pdev)
+{
+ u16 cap, ctrl, pos;
+ struct pci_dev *dev;
+
+ if (!pci_acs_enable)
+ return;
+
+ for (dev = pdev; dev; dev = pci_upstream_bridge(pdev)) {
+
+ pos = dev->acs_cap;
+ if (!pos)
+ continue;
+
+ /*
+* Enable translation blocking when last downstream device
+* that depends on it (for ATS), doesn't need ATS anymore
+*/
+ if (--dev->ats_dependencies == 0) {
+ pci_read_config_word(dev, pos + PCI_ACS_CAP, );
+ pci_read_config_word(dev, pos + PCI_ACS_CTRL, );
+ ctrl |= (cap & PCI_ACS_TB);
+ pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
+ }
+ }
+}
+
/**
* pci_restore_bars - restore a device's BAR values (e.g. after wake-up)
* @dev: PCI device to have its BARs restored
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 12fb79fbe29d..f5d8ecb6ba96 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -552,6 +552,8 @@
Re: [PATCH v4 4/4] PCI/ACS: Enable PCI_ACS_TB for untrusted/external-facing devices
On Sat, Jul 11, 2020 at 12:53 PM Bjorn Helgaas wrote:
>
> On Fri, Jul 10, 2020 at 03:53:59PM -0700, Rajat Jain wrote:
> > On Fri, Jul 10, 2020 at 2:29 PM Raj, Ashok wrote:
> > > On Fri, Jul 10, 2020 at 03:29:22PM -0500, Bjorn Helgaas wrote:
> > > > On Tue, Jul 07, 2020 at 03:46:04PM -0700, Rajat Jain wrote:
> > > > > When enabling ACS, enable translation blocking for external facing
> > > > > ports
> > > > > and untrusted devices.
> > > > >
> > > > > Signed-off-by: Rajat Jain
> > > > > ---
> > > > > v4: Add braces to avoid warning from kernel robot
> > > > > print warning for only external-facing devices.
> > > > > v3: print warning if ACS_TB not supported on
> > > > > external-facing/untrusted ports.
> > > > > Minor code comments fixes.
> > > > > v2: Commit log change
> > > > >
> > > > > drivers/pci/pci.c| 8
> > > > > drivers/pci/quirks.c | 15 +++
> > > > > 2 files changed, 23 insertions(+)
> > > > >
> > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > > index 73a8627822140..a5a6bea7af7ce 100644
> > > > > --- a/drivers/pci/pci.c
> > > > > +++ b/drivers/pci/pci.c
> > > > > @@ -876,6 +876,14 @@ static void pci_std_enable_acs(struct pci_dev
> > > > > *dev)
> > > > > /* Upstream Forwarding */
> > > > > ctrl |= (cap & PCI_ACS_UF);
> > > > >
> > > > > + /* Enable Translation Blocking for external devices */
> > > > > + if (dev->external_facing || dev->untrusted) {
> > > > > + if (cap & PCI_ACS_TB)
> > > > > + ctrl |= PCI_ACS_TB;
> > > > > + else if (dev->external_facing)
> > > > > + pci_warn(dev, "ACS: No Translation Blocking on
> > > > > external-facing dev\n");
> > > > > + }
> > > >
> > > > IIUC, this means that external devices can *never* use ATS and
> > > > can never cache translations.
> >
> > Yes, but it already exists today (and this patch doesn't change that):
> > 521376741b2c2 "PCI/ATS: Only enable ATS for trusted devices"
> >
> > IMHO any external device trying to send ATS traffic despite having ATS
> > disabled should count as a bad intent. And this patch is trying to
> > plug that loophole, by blocking the AT traffic from devices that we do
> > not expect to see AT from anyway.
>
> Thinking about this some more, I wonder if Linux should:
>
> - Explicitly disable ATS for every device at enumeration-time, e.g.,
> in pci_init_capabilities(),
>
> - Enable PCI_ACS_TB for every device (not just external-facing or
> untrusted ones),
>
> - Disable PCI_ACS_TB for the relevant devices along the path only
> when enabling ATS.
>
> One nice thing about doing that is that the "untrusted" test would be
> only in pci_enable_ats(), and we wouldn't need one in
> pci_std_enable_acs().
Sent out v5 with this approach here:
https://patchwork.kernel.org/patch/11663515/
Thanks,
Rajat
>
>
> It's possible BIOS gives us devices with ATS enabled, and this might
> break them, but that seems like something we'd want to find out about.
>
> Bjorn
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 4/4] PCI/ACS: Enable PCI_ACS_TB for untrusted/external-facing devices
Hello,
On Fri, Jul 10, 2020 at 2:29 PM Raj, Ashok wrote:
>
> Hi Bjorn
>
>
> On Fri, Jul 10, 2020 at 03:29:22PM -0500, Bjorn Helgaas wrote:
> > On Tue, Jul 07, 2020 at 03:46:04PM -0700, Rajat Jain wrote:
> > > When enabling ACS, enable translation blocking for external facing ports
> > > and untrusted devices.
> > >
> > > Signed-off-by: Rajat Jain
> > > ---
> > > v4: Add braces to avoid warning from kernel robot
> > > print warning for only external-facing devices.
> > > v3: print warning if ACS_TB not supported on external-facing/untrusted
> > > ports.
> > > Minor code comments fixes.
> > > v2: Commit log change
> > >
> > > drivers/pci/pci.c| 8
> > > drivers/pci/quirks.c | 15 +++
> > > 2 files changed, 23 insertions(+)
> > >
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index 73a8627822140..a5a6bea7af7ce 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -876,6 +876,14 @@ static void pci_std_enable_acs(struct pci_dev *dev)
> > > /* Upstream Forwarding */
> > > ctrl |= (cap & PCI_ACS_UF);
> > >
> > > + /* Enable Translation Blocking for external devices */
> > > + if (dev->external_facing || dev->untrusted) {
> > > + if (cap & PCI_ACS_TB)
> > > + ctrl |= PCI_ACS_TB;
> > > + else if (dev->external_facing)
> > > + pci_warn(dev, "ACS: No Translation Blocking on
> > > external-facing dev\n");
> > > + }
> >
> > IIUC, this means that external devices can *never* use ATS
> and can
> > never cache translations.
Yes, but it already exists today (and this patch doesn't change that):
521376741b2c2 "PCI/ATS: Only enable ATS for trusted devices"
IMHO any external device trying to send ATS traffic despite having ATS
disabled should count as a bad intent. And this patch is trying to
plug that loophole, by blocking the AT traffic from devices that we do
not expect to see AT from anyway.
Do you see any case where this is not true?
> And (I guess, I'm not an expert) it can
> > also never use the Page Request Services?
>
> Yep, sounds like it.
Yes, from spec "Address Translation Services" Rev 1.1:
"...a device that supports ATS need not support PRI, but PRI is
dependent on ATS’s capabilities."
(So no ATS = No PRI).
>
> >
> > Is this what we want? Do we have any idea how many external devices
> > this will affect or how much of a performance impact they will see?
> >
> > Do we need some kind of override or mechanism to authenticate certain
> > devices so they can use ATS and PRI?
>
> Sounds like we would need some form of an allow-list to start with so we
> can have something in the interim.
I assume what is being referred to, is an escape hatch to enable ATS
on certain given "external-facing" ports (and devices downstream on
that port). Do we really think a *per-port* control for ATS may be
needed? I can add if there is consensus about this.
>
> I suppose a future platform might have a facilty to ensure ATS is secure and
> authenticated we could enable for all of devices in the system, in addition
> to PCI CMA/IDE.
>
> I think having a global override to enable all devices so platform can
> switch to current behavior, or maybe via a cmdline switch.. as much as we
> have a billion of those, it still gives an option in case someone needs it.
Currently:
pci.noats => No ATS on all PCI devices.
(Absense of pci.noats): ATS on all PCI devices, EXCEPT external devices.
I can look to add another parameter that is synonymous to
"trust-external-pci-devices" that can keep ATS enabled on external
ports as well. I think this is better than an allow-list of only
certain ports, because most likely an admin will trust all its
external ports, or not. Also, we can add this global override and may
be add a more granular control later, if and when really needed.
Thanks,
Rajat
>
>
>
> >
> > If we do decide this is the right thing to do, I think we need to
> > expand the commit log a bit, because this is potentially a significant
> > user-visible change.
> >
> > > pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
> > > }
> > >
> > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > > index b341628e47527..bb22b46c1d719 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_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF
> > > + *
> > > + * TODO: This quirk 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,14 @@ static int
> > > pci_quirk_enable_intel_spt_pch_acs(struct pci_dev *dev)
> > > ctrl |= (cap & PCI_ACS_CR);
> > > ctrl
[PATCH v4 2/4] PCI: Keep the ACS capability offset in device
Currently ACS capabiity 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
---
v4: No change
v3: fix commit log, remove forward declation of static function
v2: Commit log cosmetic changes
drivers/pci/p2pdma.c | 2 +-
drivers/pci/pci.c| 20
drivers/pci/pci.h| 2 +-
drivers/pci/probe.c | 2 +-
drivers/pci/quirks.c | 8
include/linux/pci.h | 1 +
6 files changed, 24 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 eec625f0e594e..73a8627822140 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -831,7 +831,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;
@@ -857,7 +857,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;
@@ -883,7 +883,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 +3362,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 +3487,18 @@ bool pci_acs_path_enabled(struct pci_dev *start,
return true;
}
+/**
+ * pci_acs_init - Initialize ACS 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
[PATCH v4 1/4] PCI: Move pci_enable_acs() and its dependencies up in pci.c
Move pci_enable_acs() and the functions it depends on, further up in the
source code to avoid having to forward declare it when we make it static
in near future (next patch).
No functional changes intended.
Signed-off-by: Rajat Jain
---
v4: Same as v3
v3: Initial version of the patch, created per Bjorn's suggestion
drivers/pci/pci.c | 254 +++---
1 file changed, 127 insertions(+), 127 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index ce096272f52b1..eec625f0e594e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -777,6 +777,133 @@ int pci_wait_for_pending(struct pci_dev *dev, int pos,
u16 mask)
return 0;
}
+static int pci_acs_enable;
+
+/**
+ * pci_request_acs - ask for ACS to be enabled if supported
+ */
+void pci_request_acs(void)
+{
+ pci_acs_enable = 1;
+}
+
+static const char *disable_acs_redir_param;
+
+/**
+ * pci_disable_acs_redir - disable ACS redirect capabilities
+ * @dev: the PCI device
+ *
+ * For only devices specified in the disable_acs_redir parameter.
+ */
+static void pci_disable_acs_redir(struct pci_dev *dev)
+{
+ int ret = 0;
+ const char *p;
+ int pos;
+ u16 ctrl;
+
+ if (!disable_acs_redir_param)
+ return;
+
+ p = disable_acs_redir_param;
+ while (*p) {
+ ret = pci_dev_str_match(dev, p, );
+ if (ret < 0) {
+ pr_info_once("PCI: Can't parse disable_acs_redir
parameter: %s\n",
+disable_acs_redir_param);
+
+ break;
+ } else if (ret == 1) {
+ /* Found a match */
+ break;
+ }
+
+ if (*p != ';' && *p != ',') {
+ /* End of param or invalid format */
+ break;
+ }
+ p++;
+ }
+
+ if (ret != 1)
+ return;
+
+ if (!pci_dev_specific_disable_acs_redir(dev))
+ return;
+
+ pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS);
+ if (!pos) {
+ pci_warn(dev, "cannot disable ACS redirect for this hardware as
it does not have ACS capabilities\n");
+ return;
+ }
+
+ pci_read_config_word(dev, pos + PCI_ACS_CTRL, );
+
+ /* P2P Request & Completion Redirect */
+ ctrl &= ~(PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC);
+
+ pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
+
+ pci_info(dev, "disabled ACS redirect\n");
+}
+
+/**
+ * pci_std_enable_acs - enable ACS on devices using standard ACS capabilities
+ * @dev: the PCI device
+ */
+static void pci_std_enable_acs(struct pci_dev *dev)
+{
+ int pos;
+ u16 cap;
+ u16 ctrl;
+
+ pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS);
+ if (!pos)
+ return;
+
+ pci_read_config_word(dev, pos + PCI_ACS_CAP, );
+ pci_read_config_word(dev, pos + PCI_ACS_CTRL, );
+
+ /* Source Validation */
+ ctrl |= (cap & PCI_ACS_SV);
+
+ /* P2P Request Redirect */
+ ctrl |= (cap & PCI_ACS_RR);
+
+ /* P2P Completion Redirect */
+ ctrl |= (cap & PCI_ACS_CR);
+
+ /* Upstream Forwarding */
+ ctrl |= (cap & PCI_ACS_UF);
+
+ pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
+}
+
+/**
+ * pci_enable_acs - enable ACS if hardware support it
+ * @dev: the PCI device
+ */
+void pci_enable_acs(struct pci_dev *dev)
+{
+ if (!pci_acs_enable)
+ goto disable_acs_redir;
+
+ if (!pci_dev_specific_enable_acs(dev))
+ goto disable_acs_redir;
+
+ pci_std_enable_acs(dev);
+
+disable_acs_redir:
+ /*
+* Note: pci_disable_acs_redir() must be called even if ACS was not
+* enabled by the kernel because it may have been enabled by
+* platform firmware. So if we are told to disable it, we should
+* always disable it after setting the kernel's default
+* preferences.
+*/
+ pci_disable_acs_redir(dev);
+}
+
/**
* pci_restore_bars - restore a device's BAR values (e.g. after wake-up)
* @dev: PCI device to have its BARs restored
@@ -3230,133 +3357,6 @@ void pci_configure_ari(struct pci_dev *dev)
}
}
-static int pci_acs_enable;
-
-/**
- * pci_request_acs - ask for ACS to be enabled if supported
- */
-void pci_request_acs(void)
-{
- pci_acs_enable = 1;
-}
-
-static const char *disable_acs_redir_param;
-
-/**
- * pci_disable_acs_redir - disable ACS redirect capabilities
- * @dev: the PCI device
- *
- * For only devices specified in the disable_acs_redir parameter.
- */
-static void pci_disable_acs_redir(struct pci_dev *dev)
-{
- int ret = 0;
- const char *p;
- int pos;
- u16 ctrl;
-
- if (!disable_acs_redir_param)
- return;
-
- p = disable_acs_redir_param;
- while (*p) {
- ret =
[PATCH v4 4/4] PCI/ACS: Enable PCI_ACS_TB for untrusted/external-facing devices
When enabling ACS, enable translation blocking for external facing ports
and untrusted devices.
Signed-off-by: Rajat Jain
---
v4: Add braces to avoid warning from kernel robot
print warning for only external-facing devices.
v3: print warning if ACS_TB not supported on external-facing/untrusted ports.
Minor code comments fixes.
v2: Commit log change
drivers/pci/pci.c| 8
drivers/pci/quirks.c | 15 +++
2 files changed, 23 insertions(+)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 73a8627822140..a5a6bea7af7ce 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -876,6 +876,14 @@ static void pci_std_enable_acs(struct pci_dev *dev)
/* Upstream Forwarding */
ctrl |= (cap & PCI_ACS_UF);
+ /* Enable Translation Blocking for external devices */
+ if (dev->external_facing || dev->untrusted) {
+ if (cap & PCI_ACS_TB)
+ ctrl |= PCI_ACS_TB;
+ else if (dev->external_facing)
+ pci_warn(dev, "ACS: No Translation Blocking on
external-facing dev\n");
+ }
+
pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
}
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index b341628e47527..bb22b46c1d719 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_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF
+ *
+ * TODO: This quirk 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,14 @@ static int pci_quirk_enable_intel_spt_pch_acs(struct
pci_dev *dev)
ctrl |= (cap & PCI_ACS_CR);
ctrl |= (cap & PCI_ACS_UF);
+ /* Enable Translation Blocking for external devices */
+ if (dev->external_facing || dev->untrusted) {
+ if (cap & PCI_ACS_TB)
+ ctrl |= PCI_ACS_TB;
+ else if (dev->external_facing)
+ pci_warn(dev, "ACS: No Translation Blocking on
external-facing dev\n");
+ }
+
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.212.ge8ba1cc988-goog
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v4 3/4] PCI: Treat "external-facing" devices as internal
The "ExternalFacingPort" devices (root ports) are internal devices that
sit on the internal system fabric. Ref:
https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports
Currently they were treated (marked as untrusted) at par with other
external devices downstream those external facing rootports.
Use the platform flag to identify the external facing devices and then
treat them at par with internal devices (don't mark them untrusted).
Any devices downstream continue to be marked as "untrusted". This was
discussed here:
https://lore.kernel.org/linux-pci/20200610230906.GA1528594@bjorn-Precision-5520/
Signed-off-by: Rajat Jain
---
v4: No change
v3: * fix commit log and minor code comment
* Don't check for "ExternalFacingPort" on PCI_EXP_TYPE_DOWNSTREAM
* Check only for pdev->external_facing in iommu.c
v2: cosmetic changes in commit log
drivers/iommu/intel/iommu.c | 6 +++---
drivers/pci/of.c| 2 +-
drivers/pci/pci-acpi.c | 10 +-
drivers/pci/probe.c | 2 +-
include/linux/pci.h | 8
5 files changed, 18 insertions(+), 10 deletions(-)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index d759e7234e982..4f0f6ee2d4aaa 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4738,12 +4738,12 @@ const struct attribute_group *intel_iommu_groups[] = {
NULL,
};
-static inline bool has_untrusted_dev(void)
+static inline bool has_external_pci(void)
{
struct pci_dev *pdev = NULL;
for_each_pci_dev(pdev)
- if (pdev->untrusted)
+ if (pdev->external_facing)
return true;
return false;
@@ -4751,7 +4751,7 @@ static inline bool has_untrusted_dev(void)
static int __init platform_optin_force_iommu(void)
{
- if (!dmar_platform_optin() || no_platform_optin || !has_untrusted_dev())
+ if (!dmar_platform_optin() || no_platform_optin || !has_external_pci())
return 0;
if (no_iommu || dmar_disabled)
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..43a5158b2b662 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -1213,7 +1213,7 @@ 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;
@@ -1223,12 +1223,12 @@ static void pci_acpi_set_untrusted(struct pci_dev *dev)
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 +1240,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 0ca39042507ce..281be857d2430 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 intuntrusted: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
+* device enumerated downstream an external-facing device, is marked
[PATCH v3 4/4] PCI/ACS: Enable PCI_ACS_TB for untrusted/external-facing devices
When enabling ACS, enable translation blocking for external facing ports
and untrusted devices.
Signed-off-by: Rajat Jain
---
v3: print warning if ACS_TB not supported on external-facing/untrusted ports.
Minor code comments fixes.
v2: Commit log change
drivers/pci/pci.c| 7 +++
drivers/pci/quirks.c | 14 ++
2 files changed, 21 insertions(+)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 73a8627822140..497ac05bf36e8 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -876,6 +876,13 @@ static void pci_std_enable_acs(struct pci_dev *dev)
/* Upstream Forwarding */
ctrl |= (cap & PCI_ACS_UF);
+ /* Enable Translation Blocking for external devices */
+ if (dev->external_facing || dev->untrusted)
+ if (cap & PCI_ACS_TB)
+ ctrl |= PCI_ACS_TB;
+ else
+ pci_warn(dev, "ACS: No Trans Blocking on ext dev\n");
+
pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
}
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index b341628e47527..9cc8c1dc215ee 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_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF
+ *
+ * TODO: This quirk 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,13 @@ static int pci_quirk_enable_intel_spt_pch_acs(struct
pci_dev *dev)
ctrl |= (cap & PCI_ACS_CR);
ctrl |= (cap & PCI_ACS_UF);
+ /* Enable Translation Blocking for external devices */
+ if (dev->external_facing || dev->untrusted)
+ if (cap & PCI_ACS_TB)
+ ctrl |= PCI_ACS_TB;
+ else
+ pci_warn(dev, "ACS: No Trans Blocking on ext dev\n");
+
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.212.ge8ba1cc988-goog
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 5/7] driver core: Add device location to "struct device" and expose it in sysfs
On Wed, Jul 1, 2020 at 10:23 PM Oliver O'Halloran wrote: > > On Thu, Jul 2, 2020 at 4:07 AM Rajat Jain wrote: > > > > *snip* > > > > > > I guess it would make sense to have an attribute for user space to > > > > write to in order to make the kernel reject device plug-in events > > > > coming from a given port or connector, but the kernel has no reliable > > > > means to determine *which* ports or connectors are "safe", and even if > > > > there was a way for it to do that, it still may not agree with user > > > > space on which ports or connectors should be regarded as "safe". > > > > > > Again, we have been doing this for USB devices for a very long time, PCI > > > shouldn't be any different. Why people keep ignoring working solutions > > > is beyond me, there's nothing "special" about PCI devices here for this > > > type of "worry" or reasoning to try to create new solutions. > > > > > > So, again, I ask, go do what USB does, and to do that, take the logic > > > out of the USB core, make it bus-agnositic, and _THEN_ add it to the PCI > > > code. Why the original submitter keeps ignoring my request to do this > > > is beyond me, I guess they like making patches that will get rejected :( > > > > IMHO I'm actually trying to precisely do what I think was the > > conclusion of our discussion, and then some changes because of the > > further feedback I received on those patches. Let's take a step back > > and please allow me to explain how I got here (my apologies but this > > spans a couple of threads, and I"m trying to tie them all together > > here): > > The previous thread had some suggestions, but no real conclusions. > That's probably why we're still arguing about it... > > > GOAL: To allow user space to control what (PCI) drivers he wants to > > allow on external (thunderbolt) ports. There was a lot of debate about > > the need for such a policy at > > https://lore.kernel.org/linux-pci/CACK8Z6GR7-wseug=ttvyrarvzx_ao2geoldnbwjtb+5y7vw...@mail.gmail.com/ > > with the final conclusion that it should be OK to implement such a > > policy in userspace, as long as the policy is not implemented in the > > kernel. The kernel only needs to expose bits & info that is needed by > > the userspace to implement such a policy, and it can be used in > > conjunction with "drivers_autoprobe" to implement this policy: > > > > > > That's an odd thing, but sure, if you want to write up such a policy for > > your systems, great. But that policy does not belong in the kernel, it > > belongs in userspace. > > > > > > 1) The post > > https://lore.kernel.org/linux-pci/20200609210400.GA1461839@bjorn-Precision-5520/ > > lists out the approach that was agreed on. Replicating it here: > > --- > > - Expose the PCI pdev->untrusted bit in sysfs. We don't expose this > > today, but doing so would be trivial. I think I would prefer a > > sysfs name like "external" so it's more descriptive and less of a > > judgment. > > > > This comes from either the DT "external-facing" property or the > > ACPI "ExternalFacingPort" property. > > > > - All devices present at boot are enumerated. Any statically built > > drivers will bind to them before any userspace code runs. > > > > If you want to keep statically built drivers from binding, you'd > > need to invent some mechanism so pci_driver_init() could clear > > drivers_autoprobe after registering pci_bus_type. > > > > - Early userspace code prevents modular drivers from automatically > > binding to PCI devices: > > > > echo 0 > /sys/bus/pci/drivers_autoprobe > > > > This prevents modular drivers from binding to all devices, whether > > present at boot or hot-added. > > > > - Userspace code uses the sysfs "bind" file to control which drivers > > are loaded and can bind to each device, e.g., > > > > echo :02:00.0 > /sys/bus/pci/drivers/nvme/bind > > I think this is a reasonable suggestion. However, as Greg pointed out > it's gratuitously different to what USB does for no real reason. > > > --- > > 2) As part of implementing the above agreed approach, when I exposed > > PCI "untrusted" attribute to userspace, it ran into discussion that > > concluded that instead of this, the device core should be enhanced > > with a location attribute. > > https://lore.kernel.org/linux-pci/20200618184621.ga446...@kroah.com/ > > --- > > ... > > 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
[PATCH v3 3/4] PCI: Treat "external-facing" devices as internal
The "ExternalFacingPort" devices (root ports) are internal devices that
sit on the internal system fabric. Ref:
https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports
Currently they were treated (marked as untrusted) at par with other
external devices downstream those external facing rootports.
Use the platform flag to identify the external facing devices and then
treat them at par with internal devices (don't mark them untrusted).
Any devices downstream continue to be marked as "untrusted". This was
discussed here:
https://lore.kernel.org/linux-pci/20200610230906.GA1528594@bjorn-Precision-5520/
Signed-off-by: Rajat Jain
---
v3: * fix commit log and minor code comment
* Don't check for "ExternalFacingPort" on PCI_EXP_TYPE_DOWNSTREAM
* Check only for pdev->external_facing in iommu.c
v2: cosmetic changes in commit log
drivers/iommu/intel/iommu.c | 6 +++---
drivers/pci/of.c| 2 +-
drivers/pci/pci-acpi.c | 10 +-
drivers/pci/probe.c | 2 +-
include/linux/pci.h | 8
5 files changed, 18 insertions(+), 10 deletions(-)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index d759e7234e982..4f0f6ee2d4aaa 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4738,12 +4738,12 @@ const struct attribute_group *intel_iommu_groups[] = {
NULL,
};
-static inline bool has_untrusted_dev(void)
+static inline bool has_external_pci(void)
{
struct pci_dev *pdev = NULL;
for_each_pci_dev(pdev)
- if (pdev->untrusted)
+ if (pdev->external_facing)
return true;
return false;
@@ -4751,7 +4751,7 @@ static inline bool has_untrusted_dev(void)
static int __init platform_optin_force_iommu(void)
{
- if (!dmar_platform_optin() || no_platform_optin || !has_untrusted_dev())
+ if (!dmar_platform_optin() || no_platform_optin || !has_external_pci())
return 0;
if (no_iommu || dmar_disabled)
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..43a5158b2b662 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -1213,7 +1213,7 @@ 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;
@@ -1223,12 +1223,12 @@ static void pci_acpi_set_untrusted(struct pci_dev *dev)
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 +1240,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 0ca39042507ce..281be857d2430 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 intuntrusted: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
+* device enumerated downstream an external-facing device, is marked
+* as
[PATCH v3 1/4] PCI: Move pci_enable_acs() and its dependencies up in pci.c
Move pci_enable_acs() and the functions it depends on, further up in the
source code to avoid having to forward declare it when we make it static
in near future (next patch).
No functional changes intended.
Signed-off-by: Rajat Jain
---
v3: Initial version of the patch, created per Bjorn's suggestion
drivers/pci/pci.c | 254 +++---
1 file changed, 127 insertions(+), 127 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index ce096272f52b1..eec625f0e594e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -777,6 +777,133 @@ int pci_wait_for_pending(struct pci_dev *dev, int pos,
u16 mask)
return 0;
}
+static int pci_acs_enable;
+
+/**
+ * pci_request_acs - ask for ACS to be enabled if supported
+ */
+void pci_request_acs(void)
+{
+ pci_acs_enable = 1;
+}
+
+static const char *disable_acs_redir_param;
+
+/**
+ * pci_disable_acs_redir - disable ACS redirect capabilities
+ * @dev: the PCI device
+ *
+ * For only devices specified in the disable_acs_redir parameter.
+ */
+static void pci_disable_acs_redir(struct pci_dev *dev)
+{
+ int ret = 0;
+ const char *p;
+ int pos;
+ u16 ctrl;
+
+ if (!disable_acs_redir_param)
+ return;
+
+ p = disable_acs_redir_param;
+ while (*p) {
+ ret = pci_dev_str_match(dev, p, );
+ if (ret < 0) {
+ pr_info_once("PCI: Can't parse disable_acs_redir
parameter: %s\n",
+disable_acs_redir_param);
+
+ break;
+ } else if (ret == 1) {
+ /* Found a match */
+ break;
+ }
+
+ if (*p != ';' && *p != ',') {
+ /* End of param or invalid format */
+ break;
+ }
+ p++;
+ }
+
+ if (ret != 1)
+ return;
+
+ if (!pci_dev_specific_disable_acs_redir(dev))
+ return;
+
+ pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS);
+ if (!pos) {
+ pci_warn(dev, "cannot disable ACS redirect for this hardware as
it does not have ACS capabilities\n");
+ return;
+ }
+
+ pci_read_config_word(dev, pos + PCI_ACS_CTRL, );
+
+ /* P2P Request & Completion Redirect */
+ ctrl &= ~(PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC);
+
+ pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
+
+ pci_info(dev, "disabled ACS redirect\n");
+}
+
+/**
+ * pci_std_enable_acs - enable ACS on devices using standard ACS capabilities
+ * @dev: the PCI device
+ */
+static void pci_std_enable_acs(struct pci_dev *dev)
+{
+ int pos;
+ u16 cap;
+ u16 ctrl;
+
+ pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS);
+ if (!pos)
+ return;
+
+ pci_read_config_word(dev, pos + PCI_ACS_CAP, );
+ pci_read_config_word(dev, pos + PCI_ACS_CTRL, );
+
+ /* Source Validation */
+ ctrl |= (cap & PCI_ACS_SV);
+
+ /* P2P Request Redirect */
+ ctrl |= (cap & PCI_ACS_RR);
+
+ /* P2P Completion Redirect */
+ ctrl |= (cap & PCI_ACS_CR);
+
+ /* Upstream Forwarding */
+ ctrl |= (cap & PCI_ACS_UF);
+
+ pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
+}
+
+/**
+ * pci_enable_acs - enable ACS if hardware support it
+ * @dev: the PCI device
+ */
+void pci_enable_acs(struct pci_dev *dev)
+{
+ if (!pci_acs_enable)
+ goto disable_acs_redir;
+
+ if (!pci_dev_specific_enable_acs(dev))
+ goto disable_acs_redir;
+
+ pci_std_enable_acs(dev);
+
+disable_acs_redir:
+ /*
+* Note: pci_disable_acs_redir() must be called even if ACS was not
+* enabled by the kernel because it may have been enabled by
+* platform firmware. So if we are told to disable it, we should
+* always disable it after setting the kernel's default
+* preferences.
+*/
+ pci_disable_acs_redir(dev);
+}
+
/**
* pci_restore_bars - restore a device's BAR values (e.g. after wake-up)
* @dev: PCI device to have its BARs restored
@@ -3230,133 +3357,6 @@ void pci_configure_ari(struct pci_dev *dev)
}
}
-static int pci_acs_enable;
-
-/**
- * pci_request_acs - ask for ACS to be enabled if supported
- */
-void pci_request_acs(void)
-{
- pci_acs_enable = 1;
-}
-
-static const char *disable_acs_redir_param;
-
-/**
- * pci_disable_acs_redir - disable ACS redirect capabilities
- * @dev: the PCI device
- *
- * For only devices specified in the disable_acs_redir parameter.
- */
-static void pci_disable_acs_redir(struct pci_dev *dev)
-{
- int ret = 0;
- const char *p;
- int pos;
- u16 ctrl;
-
- if (!disable_acs_redir_param)
- return;
-
- p = disable_acs_redir_param;
- while (*p) {
- ret = pci_dev_str_match(dev, p,
[PATCH v3 2/4] PCI: Keep the ACS capability offset in device
Currently ACS capabiity 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
---
v3: fix commit log, remove forward declation of static function
v2: Commit log cosmetic changes
drivers/pci/p2pdma.c | 2 +-
drivers/pci/pci.c| 20
drivers/pci/pci.h| 2 +-
drivers/pci/probe.c | 2 +-
drivers/pci/quirks.c | 8
include/linux/pci.h | 1 +
6 files changed, 24 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 eec625f0e594e..73a8627822140 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -831,7 +831,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;
@@ -857,7 +857,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;
@@ -883,7 +883,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 +3362,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 +3487,18 @@ bool pci_acs_path_enabled(struct pci_dev *start,
return true;
}
+/**
+ * pci_acs_init - Initialize ACS 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
Re: [PATCH v2 2/7] PCI: Set "untrusted" flag for truly external devices only
Hello Bjorn,
On Mon, Jul 6, 2020 at 4:30 PM Bjorn Helgaas wrote:
>
> On Mon, Jul 06, 2020 at 03:31:47PM -0700, Rajat Jain wrote:
> > On Mon, Jul 6, 2020 at 9:38 AM Bjorn Helgaas wrote:
> > > On Mon, Jun 29, 2020 at 09:49:38PM -0700, Rajat Jain wrote:
>
> > > > -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)
> > >
> > > This looks like a change worthy of its own patch. We used to look for
> > > "ExternalFacingPort" only on Root Ports; now we'll also do it for
> > > Switch Downstream Ports.
> >
> > Can do. (please see below)
> >
> > > Can you include DT and ACPI spec references if they exist? I found
> > > this mention:
> > > https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports
> > > which actually says it should only be implemented for Root Ports.
> >
> > I actually have no references. It seems to me that the microsoft spec
> > assumes that all external ports must be implemented on root ports, but
> > I think it would be equally fair for systems with PCIe switches to
> > implement one on one of their switch downstream ports. I don't have an
> > immediate use of this anyway, so if you think this should rather wait
> > unless someone really has this case, this can wait. Let me know.
>
> I agree that it "makes sense" to pay attention to this property no
> matter where it appears, but since that Microsoft doc went to the
> trouble to restrict it to Root Ports, I think we should leave this
> as-is and only look for it in the Root Port. Otherwise Linux will
> accept something Windows will reject, and that seems like a needless
> difference.
>
> We can at least include the above link to the Microsoft doc in the
> commit log.
Will do.
>
> > > It also mentions a "DmaProperty" that looks related. Maybe Linux
> > > should also pay attention to this?
> >
> > Interesting. Since this is not in use currently by the kernel as well
> > as not exposed by (our) BIOS, I don't have an immediate use case for
> > this. I'd like to defer this for later (as-the-need-arises).
>
> I agree, you can defer this until you see a need for it. I just
> pointed it out in case it would be useful to you.
>
> > > > + /*
> > > > + * 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.
> > >
> > > This comment has a subject/verb agreement problem.
> >
> > I assume you meant s/is/are/ in last sentence. Will do.
>
> Right. There's also something wrong with "enumerated downstream an".
I'm apparently really bad at English :-). This is what I have in my
latest patch I am about to send out:
"Thus any device enumerated downstream an external-facing device, is
marked as untrusted."
Are you suggesting s/an/a/ ? Please let me know what you would like to
see and I'd copy it as-is :-)
Thanks!
Rajat
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 4/7] PCI: Add device even if driver attach failed
On Tue, Jun 30, 2020 at 1:02 AM Greg Kroah-Hartman
wrote:
>
> On Mon, Jun 29, 2020 at 09:49:40PM -0700, Rajat Jain wrote:
> > device_attach() returning failure indicates a driver error while trying to
> > probe the device. In such a scenario, the PCI device should still be added
> > in the system and be visible to the user.
> >
> > This patch partially reverts:
> > commit ab1a187bba5c ("PCI: Check device_attach() return value always")
> >
> > Signed-off-by: Rajat Jain
> > Reviewed-by: Greg Kroah-Hartman
> > ---
> > v2: Cosmetic change in commit log.
> > Add Greg's "reviewed-by"
> >
> > drivers/pci/bus.c | 6 +-
> > 1 file changed, 1 insertion(+), 5 deletions(-)
> >
> > diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> > index 8e40b3e6da77d..3cef835b375fd 100644
> > --- a/drivers/pci/bus.c
> > +++ b/drivers/pci/bus.c
> > @@ -322,12 +322,8 @@ void pci_bus_add_device(struct pci_dev *dev)
> >
> > dev->match_driver = true;
> > retval = device_attach(>dev);
> > - if (retval < 0 && retval != -EPROBE_DEFER) {
> > + if (retval < 0 && retval != -EPROBE_DEFER)
> > pci_warn(dev, "device attach failed (%d)\n", retval);
> > - pci_proc_detach_device(dev);
> > - pci_remove_sysfs_dev_files(dev);
> > - return;
> > - }
> >
> > pci_dev_assign_added(dev, true);
> > }
>
> This should go first in the series, and cc: stable and get merged now.
> No need to tie it to this series at all.
>
> Or just an independant patch, it doesn't have much to do with this
> series, it's a bugfix.
Resent this patch as an independent patch with cc:stable here:
https://lore.kernel.org/patchwork/patch/1268456/
Thanks,
Rajat
>
> thanks,
>
> greg k-h
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH RESEND v2] PCI: Add device even if driver attach failed
device_attach() returning failure indicates a driver error while trying to
probe the device. In such a scenario, the PCI device should still be added
in the system and be visible to the user.
This patch partially reverts:
commit ab1a187bba5c ("PCI: Check device_attach() return value always")
Signed-off-by: Rajat Jain
Reviewed-by: Greg Kroah-Hartman
---
Resending to stable, independent from other patches per Greg's suggestion
v2: Add Greg's reviewed by, fix commit log
drivers/pci/bus.c | 6 +-
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 8e40b3e6da77d..3cef835b375fd 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -322,12 +322,8 @@ void pci_bus_add_device(struct pci_dev *dev)
dev->match_driver = true;
retval = device_attach(>dev);
- if (retval < 0 && retval != -EPROBE_DEFER) {
+ if (retval < 0 && retval != -EPROBE_DEFER)
pci_warn(dev, "device attach failed (%d)\n", retval);
- pci_proc_detach_device(dev);
- pci_remove_sysfs_dev_files(dev);
- return;
- }
pci_dev_assign_added(dev, true);
}
--
2.27.0.212.ge8ba1cc988-goog
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 3/7] PCI/ACS: Enable PCI_ACS_TB for untrusted/external-facing devices
On Mon, Jul 6, 2020 at 10:07 AM Bjorn Helgaas wrote:
>
> On Mon, Jun 29, 2020 at 09:49:39PM -0700, Rajat Jain wrote:
> > When enabling ACS, enable translation blocking for external facing ports
> > and untrusted devices.
> >
> > Signed-off-by: Rajat Jain
> > ---
> > v2: Commit log change
> >
> > 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
>
> Nit: Reorder these as in c8de8ed2dcaa ("PCI: Make ACS quirk
> implementations more uniform") so they match other similar lists in
> the code.
Will do.
>
> But more to the point: we have a bunch of other quirks for devices
> that do not have an ACS capability but *do* provide some ACS-like
> features. Most of them support
>
> PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF
>
> because that's what we usually want. But I bet some of them also
> actually provide the equivalent of PCI_ACS_TB.
>
> REQ_ACS_FLAGS doesn't include PCI_ACS_TB. Is there anything we need
> to do on the pci_acs_enabled() side to check for PCI_ACS_TB, and
> consequently, to update any of the quirks for devices that provide it?
I'm actually not sure.
+Alex Williamson , do you have any comments here?
Thanks,
Rajat
>
> > + *
> > + * 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.212.ge8ba1cc988-goog
> >
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 3/7] PCI/ACS: Enable PCI_ACS_TB for untrusted/external-facing devices
On Mon, Jul 6, 2020 at 9:45 AM Bjorn Helgaas wrote:
>
> On Mon, Jun 29, 2020 at 09:49:39PM -0700, Rajat Jain wrote:
> > When enabling ACS, enable translation blocking for external facing ports
> > and untrusted devices.
> >
> > Signed-off-by: Rajat Jain
> > ---
> > v2: Commit log change
> >
> > 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
>
> I don't understand this comment. Is this a "TODO"? Is there
> something more that needs to be done here?
Yes. I'll mark it as a TODO to make it more clear.
>
> After a patch is applied, a comment should describe the code as it is.
>
> > + */
> > 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.212.ge8ba1cc988-goog
> >
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/7] PCI: Set "untrusted" flag for truly external devices only
Hello,
On Mon, Jul 6, 2020 at 9:38 AM Bjorn Helgaas wrote:
>
> On Mon, Jun 29, 2020 at 09:49:38PM -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.
> >
> > 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
>
> Use the imperative mood in the commit log, as you did for 1/7. E.g.,
> instead of "This patch uses ...", say "Use the platform flag ...".
> That helps all the commit logs read nicely together.
>
> I think this patch makes two changes that should be separated:
>
> - Treat "external-facing" devices as internal.
>
> - Look for the "external-facing" or "ExternalFacing" property on
> Switch Downstream Ports as well as Root Ports.
>
> > Signed-off-by: Rajat Jain
> > ---
> > v2: cosmetic changes in commit log
> >
> > 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 d759e7234e982..1ccb224f82496 100644
> > --- a/drivers/iommu/intel/iommu.c
> > +++ b/drivers/iommu/intel/iommu.c
> > @@ -4743,7 +4743,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)
>
> I think checking pdev->external_facing is enough for this case,
> because it's impossible to have pdev->untrusted unless a parent has
> pdev->external_facing.
Agree.
>
> IIUC, this usage is asking "might we ever have an external device?"
> as opposed to the "pdev->untrusted" uses, which are asking "is *this*
> device an external device?"
Agree.
>
> > 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)
>
> This looks like a change worthy of its own patch. We used to look for
> "ExternalFacingPort" only on Root Ports; now we'll also do it for
> Switch Downstream Ports.
Can do. (please see below)
>
> Can you include DT and ACPI spec references if they exist? I found
> this mention:
> https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports
> which actually says it should only be implemented for Root Ports.
I actually have no references. It seems to me that the microsoft spec
assumes that all external ports must be implemented on root ports, but
I think it would be equally fair for systems with PCIe switches to
implement one on one of their switch downstream ports. I don't have an
immediate use of this anyway, so if you think this should rather wait
unless someone really has this case, this can wait. Let me know.
>
> It also mentions a "DmaProperty" that looks related. Maybe Linux
> should also pay attention to this?
Interesting. Since this is not in use currently by the kernel as well
as not exposed by (our) BIOS, I don't have an immediate use case for
this. I'd like to defer this for later (as-the-need-arises).
>
> If we do change this, should we use pcie_downstream_port(), which
> includes PCI-to-PCIe bridges as well?
Sure, can do that.
>
> > return;
> > if (device_property_read_u8(>dev, "ExternalFacingPort", ))
> > return;
> >
> > /*
> > - * These root ports expose PCIe (including DMA) outside of the
> > - * system so make
Re: [PATCH v2 0/7] Tighten PCI security, expose dev location in sysfs
On Sat, Jul 4, 2020 at 4:44 AM Pavel Machek wrote: > > Hi! > > > * The first 3 patches tighten the PCI security using ACS, and take care > > of a border case. > > * The 4th patch takes care of PCI bug. > > * 5th and 6th patches expose a device's location into the sysfs to allow > > admin to make decision based on that. > > I see no patch for Documentation -- new sysfs interfaces should be > documented for 5/6. Yes, sorry. The patches 5/6 have run into discussion and it looks are not acceptable at the moment. Thanks, Rajat > > Pavel > > > drivers/base/core.c | 35 +++ > > drivers/iommu/intel/iommu.c | 31 ++- > > drivers/pci/ats.c | 2 +- > > drivers/pci/bus.c | 13 ++-- > > drivers/pci/of.c| 2 +- > > drivers/pci/p2pdma.c| 2 +- > > drivers/pci/pci-acpi.c | 13 ++-- > > drivers/pci/pci-driver.c| 1 + > > drivers/pci/pci.c | 34 ++ > > drivers/pci/pci.h | 3 ++- > > drivers/pci/probe.c | 20 +++--- > > drivers/pci/quirks.c| 19 + > > include/linux/device.h | 42 + > > include/linux/device/bus.h | 8 +++ > > include/linux/pci.h | 13 ++-- > > 15 files changed, 191 insertions(+), 47 deletions(-) > > > > -- > (english) http://www.livejournal.com/~pavelmachek > (cesky, pictures) > http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 1/7] PCI: Keep the ACS capability offset in device
Hi Bjorn, Thanks for taking a look. On Mon, Jul 6, 2020 at 8:58 AM Bjorn Helgaas wrote: > > On Mon, Jun 29, 2020 at 09:49:37PM -0700, Rajat Jain wrote: > > 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. > > Write the commit log so it is complete even without the subject. > Right now, you have to read the subject to know what "this" refers to. > > The subject is like the title; the log is like the body of an article. > The title isn't *part* of the article, so the article has to make > sense all by itself. Fixed. > > > +static void pci_enable_acs(struct pci_dev *dev); > > I don't think we need this forward declaration, do we? We need it unless we move its definition further up in the file: drivers/pci/pci.c: In function ‘pci_restore_state’: drivers/pci/pci.c:1551:2: error: implicit declaration of function ‘pci_enable_acs’; did you mean ‘pci_enable_ats’? [-Werror=implicit-function-declaration] 1551 | pci_enable_acs(dev); Do you want me to move it up in the file so that we do not need the forward declaration? > > > @@ -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; > > I assume you verified that all these quirks are FINAL quirks, since > pci_init_capabilities() is called after HEADER quirks. I'll > double-check before applying this. None of these quirks are applied via DECLARE_PCI_FIXUP_*(). All these quirks are called (directly or indirectly) from either pci_enable_acs() or pci_acs_enabled(), EXCEPT pci_idt_bus_quirk(). That one is called from pci_bus_read_dev_vendor_id() which should be called only after the parent bridge has been added and setup correctly. So it looks all good to me. Thanks, Rajat > > Bjorn ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 5/7] driver core: Add device location to "struct device" and expose it in sysfs
Hello,
On Tue, Jun 30, 2020 at 10:00 AM Greg Kroah-Hartman
wrote:
>
> On Tue, Jun 30, 2020 at 06:08:31PM +0200, Rafael J. Wysocki wrote:
> > On Tue, Jun 30, 2020 at 5:38 PM Greg Kroah-Hartman
> > wrote:
> > >
> > > On Tue, Jun 30, 2020 at 03:00:34PM +0200, Rafael J. Wysocki wrote:
> > > > On Tue, Jun 30, 2020 at 2:52 PM Greg Kroah-Hartman
> > > > wrote:
> > > > >
> > > > > On Tue, Jun 30, 2020 at 01:49:48PM +0300, Heikki Krogerus wrote:
> > > > > > On Mon, Jun 29, 2020 at 09:49:41PM -0700, Rajat Jain wrote:
> > > > > > > Add a new (optional) field to denote the physical location of a
> > > > > > > device
> > > > > > > in the system, and expose it in sysfs. This was discussed here:
> > > > > > > https://lore.kernel.org/linux-acpi/20200618184621.ga446...@kroah.com/
> > > > > > >
> > > > > > > (The primary choice for attribute name i.e. "location" is already
> > > > > > > exposed as an ABI elsewhere, so settled for "site"). Individual
> > > > > > > buses
> > > > > > > that want to support this new attribute can opt-in by setting a
> > > > > > > flag in
> > > > > > > bus_type, and then populating the location of device while
> > > > > > > enumerating
> > > > > > > it.
> > > > > >
> > > > > > So why not just call it "physical_location"?
> > > > >
> > > > > That's better, and will allow us to put "3rd blue plug from the left,
> > > > > 4th row down" in there someday :)
> > > > >
> > > > > All of this is "relative" to the CPU, right? But what CPU? Again,
> > > > > how
> > > > > are the systems with drawers of PCI and CPUs and memory that can be
> > > > > added/removed at any point in time being handled here? What is
> > > > > "internal" and "external" for them?
> > > > >
> > > > > What exactly is the physical boundry here that is attempting to be
> > > > > described?
> > > >
> > > > Also, where is the "physical location" information going to come from?
> > >
> > > Who knows? :)
> > >
> > > Some BIOS seem to provide this, but do you trust that?
> > >
> > > > If that is the platform firmware (which I suspect is the anticipated
> > > > case), there may be problems with reliability related to that.
> > >
> > > s/may/will/
> > >
> > > which means making the kernel inact a policy like this patch series
> > > tries to add, will result in a lot of broken systems, which is why I
> > > keep saying that it needs to be done in userspace.
> > >
> > > It's as if some of us haven't been down this road before and just keep
> > > being ignored...
> > >
> > > {sigh}
> >
> > Well, to be honest, if you are a "vertical" vendor and you control the
> > entire stack, *including* the platform firmware, it would be kind of
> > OK for you to do that in a product kernel.
> >
> > However, this is not a practical thing to do in the mainline kernel
> > which must work for everybody, including people who happen to use
> > systems with broken or even actively unfriendly firmware on them.
> >
> > So I'm inclined to say that IMO this series "as is" would not be an
> > improvement from the mainline perspective.
>
> It can be, we have been using this for USB devices for many many years
> now, quite successfully. The key is not to trust that the platform
> firmware got it right :)
>
> > I guess it would make sense to have an attribute for user space to
> > write to in order to make the kernel reject device plug-in events
> > coming from a given port or connector, but the kernel has no reliable
> > means to determine *which* ports or connectors are "safe", and even if
> > there was a way for it to do that, it still may not agree with user
> > space on which ports or connectors should be regarded as "safe".
>
> Again, we have been doing this for USB devices for a very long time, PCI
> shouldn't be any different. Why people keep ignoring working solutions
> is beyond me, there's nothing "special" about PCI devices here for this
> type of "worry" or reasoning to try to create new solutions.
>
> So, again, I ask, go do what USB does, and to do that, take the logic
> out of the USB core, make it bus-agnositic, and _THEN_ add it to the PCI
> code. Why the original submitter keeps ignoring my request to do this
> is beyond me, I guess they like making patches that will get rejected :(
IMHO I'm actually trying to precisely do what I think was the
conclusion of our discussion, and then some changes because of the
further feedback I received on those patches. Let's take a step back
and please allow me to explain how I got here (my apologies but this
spans a couple of threads, and I"m trying to tie them all together
here):
GOAL: To allow user space to control what (PCI) drivers he wants to
allow on external (thunderbolt) ports. There was a lot of debate about
the need for such a policy at
https://lore.kernel.org/linux-pci/CACK8Z6GR7-wseug=ttvyrarvzx_ao2geoldnbwjtb+5y7vw...@mail.gmail.com/
with the final conclusion that it should be OK to implement such a
policy in userspace, as long as the policy is not implemented in the
kernel. The kernel
[PATCH v2 5/7] driver core: Add device location to "struct device" and expose it in sysfs
Add a new (optional) field to denote the physical location of a device
in the system, and expose it in sysfs. This was discussed here:
https://lore.kernel.org/linux-acpi/20200618184621.ga446...@kroah.com/
(The primary choice for attribute name i.e. "location" is already
exposed as an ABI elsewhere, so settled for "site"). Individual buses
that want to support this new attribute can opt-in by setting a flag in
bus_type, and then populating the location of device while enumerating
it.
Signed-off-by: Rajat Jain
---
v2: (Initial version)
drivers/base/core.c| 35 +++
include/linux/device.h | 42 ++
include/linux/device/bus.h | 8
3 files changed, 85 insertions(+)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 67d39a90b45c7..14c815526b7fa 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1778,6 +1778,32 @@ static ssize_t online_store(struct device *dev, struct
device_attribute *attr,
}
static DEVICE_ATTR_RW(online);
+static ssize_t site_show(struct device *dev, struct device_attribute *attr,
+char *buf)
+{
+ const char *site;
+
+ device_lock(dev);
+ switch (dev->site) {
+ case SITE_INTERNAL:
+ site = "INTERNAL";
+ break;
+ case SITE_EXTENDED:
+ site = "EXTENDED";
+ break;
+ case SITE_EXTERNAL:
+ site = "EXTERNAL";
+ break;
+ case SITE_UNKNOWN:
+ default:
+ site = "UNKNOWN";
+ break;
+ }
+ device_unlock(dev);
+ return sprintf(buf, "%s\n", site);
+}
+static DEVICE_ATTR_RO(site);
+
int device_add_groups(struct device *dev, const struct attribute_group
**groups)
{
return sysfs_create_groups(>kobj, groups);
@@ -1949,8 +1975,16 @@ static int device_add_attrs(struct device *dev)
goto err_remove_dev_groups;
}
+ if (bus_supports_site(dev->bus)) {
+ error = device_create_file(dev, _attr_site);
+ if (error)
+ goto err_remove_dev_attr_online;
+ }
+
return 0;
+ err_remove_dev_attr_online:
+ device_remove_file(dev, _attr_online);
err_remove_dev_groups:
device_remove_groups(dev, dev->groups);
err_remove_type_groups:
@@ -1968,6 +2002,7 @@ static void device_remove_attrs(struct device *dev)
struct class *class = dev->class;
const struct device_type *type = dev->type;
+ device_remove_file(dev, _attr_site);
device_remove_file(dev, _attr_online);
device_remove_groups(dev, dev->groups);
diff --git a/include/linux/device.h b/include/linux/device.h
index 15460a5ac024a..a4143735ae712 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -428,6 +428,31 @@ enum dl_dev_state {
DL_DEV_UNBINDING,
};
+/**
+ * enum device_site - Physical location of the device in the system.
+ * The semantics of values depend on subsystem / bus:
+ *
+ * @SITE_UNKNOWN: Location is Unknown (default)
+ *
+ * @SITE_INTERNAL: Device is internal to the system, and cannot be (easily)
+ * removed. E.g. SoC internal devices, onboard soldered
+ * devices, internal M.2 cards (that cannot be removed
+ * without opening the chassis).
+ * @SITE_EXTENDED: Device sits an extension of the system. E.g. devices
+ * on external PCIe trays, docking stations etc. These
+ * devices may be removable, but are generally housed
+ * internally on an extension board, so they are removed
+ * only when that whole extension board is removed.
+ * @SITE_EXTERNAL: Devices truly external to the system (i.e. plugged on
+ * an external port) that may be removed or added frequently.
+ */
+enum device_site {
+ SITE_UNKNOWN = 0,
+ SITE_INTERNAL,
+ SITE_EXTENDED,
+ SITE_EXTERNAL,
+};
+
/**
* struct dev_links_info - Device data related to device links.
* @suppliers: List of links to supplier devices.
@@ -513,6 +538,7 @@ struct dev_links_info {
* device (i.e. the bus driver that discovered the device).
* @iommu_group: IOMMU group the device belongs to.
* @iommu: Per device generic IOMMU runtime data
+ * @site: Physical location of the device w.r.t. the system
*
* @offline_disabled: If set, the device is permanently online.
* @offline: Set after successful invocation of bus type's .offline().
@@ -613,6 +639,8 @@ struct device {
struct iommu_group *iommu_group;
struct dev_iommu*iommu;
+ enum device_sitesite; /* Device physical location */
+
booloffline_disabled:1;
booloffline:1;
boolof_node_reused:1;
@@ -806,6 +834,20 @@ static inline bool dev_has_sync_state(struct
[PATCH v2 7/7] PCI: Add parameter to disable attaching external devices
Introduce a PCI parameter that disables the automatic attachment of
external devices to their drivers.
This is needed to allow an admin to control which drivers he wants to
allow on external ports. For more context, see threads at:
https://lore.kernel.org/linux-pci/20200609210400.GA1461839@bjorn-Precision-5520/
https://lore.kernel.org/linux-pci/cack8z6h-dzqybmqtu5_h5ttwwn35q7yysm9a7wj0twfqp8q...@mail.gmail.com/
drivers_autoprobe can only be disabled after userspace comes up. So
any external devices that were plugged in before boot may still bind
to drivers before userspace gets a chance to clear drivers_autoprobe.
Another problem is that even with drivers_autoprobe=0, the hot-added
PCI devices are bound to drivers because PCI explicitly calls
device_attach() asking driver core to find and attach a driver. This
patch helps with both of these problems.
Signed-off-by: Rajat Jain
---
v2: Use the newly introduced dev_is_external() from device core
commit log elaborated
drivers/pci/bus.c | 11 ---
drivers/pci/pci.c | 9 +
drivers/pci/pci.h | 1 +
3 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 3cef835b375fd..c11725bccffb0 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -321,9 +321,14 @@ void pci_bus_add_device(struct pci_dev *dev)
pci_bridge_d3_update(dev);
dev->match_driver = true;
- retval = device_attach(>dev);
- if (retval < 0 && retval != -EPROBE_DEFER)
- pci_warn(dev, "device attach failed (%d)\n", retval);
+
+ if (pci_dont_attach_external_devs && dev_is_external(>dev)) {
+ pci_info(dev, "not attaching external device\n");
+ } else {
+ retval = device_attach(>dev);
+ if (retval < 0 && retval != -EPROBE_DEFER)
+ pci_warn(dev, "device attach failed (%d)\n", retval);
+ }
pci_dev_assign_added(dev, true);
}
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 35f25ac39167b..3ebcfa8b33178 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -128,6 +128,13 @@ static bool pcie_ats_disabled;
/* If set, the PCI config space of each device is printed during boot. */
bool pci_early_dump;
+/*
+ * If set, the devices behind external-facing bridges (as marked by firmware)
+ * shall not be attached automatically. Userspace will need to attach them
+ * manually: echo > /sys/bus/pci/drivers//bind
+ */
+bool pci_dont_attach_external_devs;
+
bool pci_ats_disabled(void)
{
return pcie_ats_disabled;
@@ -6539,6 +6546,8 @@ static int __init pci_setup(char *str)
pci_add_flags(PCI_SCAN_ALL_PCIE_DEVS);
} else if (!strncmp(str, "disable_acs_redir=", 18)) {
disable_acs_redir_param = str + 18;
+ } else if (!strcmp(str, "dont_attach_external_devs")) {
+ pci_dont_attach_external_devs = true;
} else {
pr_err("PCI: Unknown option `%s'\n", str);
}
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 12fb79fbe29d3..875fecb9b2612 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -13,6 +13,7 @@
extern const unsigned char pcie_link_speed[];
extern bool pci_early_dump;
+extern bool pci_dont_attach_external_devs;
bool pcie_cap_has_lnkctl(const struct pci_dev *dev);
bool pcie_cap_has_rtctl(const struct pci_dev *dev);
--
2.27.0.212.ge8ba1cc988-goog
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 3/7] PCI/ACS: Enable PCI_ACS_TB for untrusted/external-facing devices
When enabling ACS, enable translation blocking for external facing ports
and untrusted devices.
Signed-off-by: Rajat Jain
---
v2: Commit log change
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.212.ge8ba1cc988-goog
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 4/7] PCI: Add device even if driver attach failed
device_attach() returning failure indicates a driver error while trying to
probe the device. In such a scenario, the PCI device should still be added
in the system and be visible to the user.
This patch partially reverts:
commit ab1a187bba5c ("PCI: Check device_attach() return value always")
Signed-off-by: Rajat Jain
Reviewed-by: Greg Kroah-Hartman
---
v2: Cosmetic change in commit log.
Add Greg's "reviewed-by"
drivers/pci/bus.c | 6 +-
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 8e40b3e6da77d..3cef835b375fd 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -322,12 +322,8 @@ void pci_bus_add_device(struct pci_dev *dev)
dev->match_driver = true;
retval = device_attach(>dev);
- if (retval < 0 && retval != -EPROBE_DEFER) {
+ if (retval < 0 && retval != -EPROBE_DEFER)
pci_warn(dev, "device attach failed (%d)\n", retval);
- pci_proc_detach_device(dev);
- pci_remove_sysfs_dev_files(dev);
- return;
- }
pci_dev_assign_added(dev, true);
}
--
2.27.0.212.ge8ba1cc988-goog
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 6/7] PCI: Move pci_dev->untrusted logic to use device location instead
The firmware was provinding "ExternalFacing" attribute on PCI root ports,
to allow the kernel to mark devices behind it as external. Note that the
firmware provides an immutable, read-only property, i.e. the location of
the device.
The use of (external) device location as hint for (dis)trust, is a
decision that IOMMU drivers have taken, so we should call it out
explicitly.
This patch removes the pci_dev->untrusted and changes the users of it to
use device core provided device location instead. This location is
populated by PCI using the same "ExternalFacing" firmware info. Any
device not behind the "ExternalFacing" bridges are marked internal and
the ones behind such bridges are markes external.
Signed-off-by: Rajat Jain
---
v2: (Initial version)
drivers/iommu/intel/iommu.c | 31 +--
drivers/pci/ats.c | 2 +-
drivers/pci/pci-driver.c| 1 +
drivers/pci/pci.c | 2 +-
drivers/pci/probe.c | 18 --
drivers/pci/quirks.c| 2 +-
include/linux/pci.h | 10 +-
7 files changed, 38 insertions(+), 28 deletions(-)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 1ccb224f82496..ca66a196f5e97 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -168,6 +168,22 @@ static inline unsigned long virt_to_dma_pfn(void *p)
return page_to_dma_pfn(virt_to_page(p));
}
+static inline bool untrusted_dev(struct device *dev)
+{
+ /*
+* Treat all external PCI devices as untrusted devices. These are the
+* devices behing marked behind external-facing bridges as marked by
+* the firmware. The untrusted devices are the ones that can potentially
+* execute DMA attacks and similar. They are typically connected through
+* external thunderbolt ports. When an IOMMU is enabled they should be
+* getting full mappings to ensure they cannot access arbitrary memory.
+*/
+ if (dev_is_pci(dev) && dev_is_external(dev))
+ return true;
+
+ return false;
+}
+
/* global iommu list, set NULL for ignored DMAR units */
static struct intel_iommu **g_iommus;
@@ -383,8 +399,7 @@ struct device_domain_info *get_domain_info(struct device
*dev)
DEFINE_SPINLOCK(device_domain_lock);
static LIST_HEAD(device_domain_list);
-#define device_needs_bounce(d) (!intel_no_bounce && dev_is_pci(d) && \
- to_pci_dev(d)->untrusted)
+#define device_needs_bounce(d) (!intel_no_bounce && untrusted_dev(d))
/*
* Iterate over elements in device_domain_list and call the specified
@@ -2830,7 +2845,7 @@ static int device_def_domain_type(struct device *dev)
* Prevent any device marked as untrusted from getting
* placed into the statically identity mapping domain.
*/
- if (pdev->untrusted)
+ if (untrusted_dev(dev))
return IOMMU_DOMAIN_DMA;
if ((iommu_identity_mapping & IDENTMAP_AZALIA) &&
IS_AZALIA(pdev))
@@ -3464,7 +3479,6 @@ static void intel_unmap(struct device *dev, dma_addr_t
dev_addr, size_t size)
unsigned long iova_pfn;
struct intel_iommu *iommu;
struct page *freelist;
- struct pci_dev *pdev = NULL;
domain = find_domain(dev);
BUG_ON(!domain);
@@ -3477,11 +3491,8 @@ static void intel_unmap(struct device *dev, dma_addr_t
dev_addr, size_t size)
start_pfn = mm_to_dma_pfn(iova_pfn);
last_pfn = start_pfn + nrpages - 1;
- if (dev_is_pci(dev))
- pdev = to_pci_dev(dev);
-
freelist = domain_unmap(domain, start_pfn, last_pfn);
- if (intel_iommu_strict || (pdev && pdev->untrusted) ||
+ if (intel_iommu_strict || untrusted_dev(dev) ||
!has_iova_flush_queue(>iovad)) {
iommu_flush_iotlb_psi(iommu, domain, start_pfn,
nrpages, !freelist, 0);
@@ -4743,7 +4754,7 @@ static inline bool has_untrusted_dev(void)
struct pci_dev *pdev = NULL;
for_each_pci_dev(pdev)
- if (pdev->untrusted || pdev->external_facing)
+ if (pdev->external_facing || untrusted_dev(>dev))
return true;
return false;
@@ -6036,7 +6047,7 @@ intel_iommu_domain_set_attr(struct iommu_domain *domain,
*/
static bool risky_device(struct pci_dev *pdev)
{
- if (pdev->untrusted) {
+ if (untrusted_dev(>dev)) {
pci_info(pdev,
"Skipping IOMMU quirk for dev [%04X:%04X] on untrusted
PCI link\n",
pdev->vendor, pdev->device);
diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index b761c1f72f672..ebd370f4d5b06 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -42,7 +42,7 @@ bool pci_ats_supported(struct pci_dev *dev)
if (!dev->ats_cap)
return false;
-
[PATCH v2 2/7] PCI: Set "untrusted" flag for truly external devices only
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.
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
---
v2: cosmetic changes in commit log
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 d759e7234e982..1ccb224f82496 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4743,7 +4743,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, "ExternalFacingPort", ))
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 intuntrusted: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 intexternal_facing:1;
unsigned intbroken_intx_masking:1; /* INTx masking can't be used */
unsigned intio_window_1k:1; /* Intel bridge 1K I/O windows
*/
unsigned intirq_managed:1;
--
2.27.0.212.ge8ba1cc988-goog
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 1/7] PCI: Keep the ACS capability offset in device
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
---
v2: Commit log cosmetic changes
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,
[PATCH v2 0/7] Tighten PCI security, expose dev location in sysfs
This is a set of loosely related patches most of whom emerged out of discussion in the following threads. In a nutshell the goal was to allow an administrator to specify which driver he wants to allow on external ports, and a strategy was chalked out: https://lore.kernel.org/linux-pci/20200609210400.GA1461839@bjorn-Precision-5520/ https://lore.kernel.org/linux-pci/20200618184621.ga446...@kroah.com/ https://lore.kernel.org/linux-pci/20200627050225.ga226...@kroah.com/ * The first 3 patches tighten the PCI security using ACS, and take care of a border case. * The 4th patch takes care of PCI bug. * 5th and 6th patches expose a device's location into the sysfs to allow admin to make decision based on that. * 7th patch is to ensure that the external devices don't bind to drivers during boot. Rajat Jain (7): PCI: Keep the ACS capability offset in device PCI: Set "untrusted" flag for truly external devices only PCI/ACS: Enable PCI_ACS_TB for untrusted/external-facing devices PCI: Add device even if driver attach failed driver core: Add device location to "struct device" and expose it in sysfs PCI: Move pci_dev->untrusted logic to use device location instead PCI: Add parameter to disable attaching external devices drivers/base/core.c | 35 +++ drivers/iommu/intel/iommu.c | 31 ++- drivers/pci/ats.c | 2 +- drivers/pci/bus.c | 13 ++-- drivers/pci/of.c| 2 +- drivers/pci/p2pdma.c| 2 +- drivers/pci/pci-acpi.c | 13 ++-- drivers/pci/pci-driver.c| 1 + drivers/pci/pci.c | 34 ++ drivers/pci/pci.h | 3 ++- drivers/pci/probe.c | 20 +++--- drivers/pci/quirks.c| 19 + include/linux/device.h | 42 + include/linux/device/bus.h | 8 +++ include/linux/pci.h | 13 ++-- 15 files changed, 191 insertions(+), 47 deletions(-) -- 2.27.0.212.ge8ba1cc988-goog ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/2] pci: Add pci device even if the driver failed to attach
On Fri, Jun 26, 2020 at 8:39 AM Bjorn Helgaas wrote:
>
> Nit: when you update these patches, can you run "git log --oneline
> drivers/pci/bus.c" and make your subject lines match the convention?
Sorry, will do.
> E.g.,
>
> PCI: Add device even if driver attach failed
>
> On Thu, Jun 25, 2020 at 05:27:09PM -0700, Rajat Jain wrote:
> > device_attach() returning failure indicates a driver error
> > while trying to probe the device. In such a scenario, the PCI
> > device should still be added in the system and be visible to
> > the user.
>
> Nit: please wrap logs to fill 75 characters. "git log" adds 4 spaces
> at the beginning, so 75+4 still fits nicely in 80 columns without
> wrapping.
Sorry, will do.
>
> > This patch partially reverts:
> > commit ab1a187bba5c ("PCI: Check device_attach() return value always")
> >
> > Signed-off-by: Rajat Jain
> > ---
> > drivers/pci/bus.c | 6 +-
> > 1 file changed, 1 insertion(+), 5 deletions(-)
> >
> > diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> > index 8e40b3e6da77d..3cef835b375fd 100644
> > --- a/drivers/pci/bus.c
> > +++ b/drivers/pci/bus.c
> > @@ -322,12 +322,8 @@ void pci_bus_add_device(struct pci_dev *dev)
> >
> > dev->match_driver = true;
> > retval = device_attach(>dev);
> > - if (retval < 0 && retval != -EPROBE_DEFER) {
> > + if (retval < 0 && retval != -EPROBE_DEFER)
> > pci_warn(dev, "device attach failed (%d)\n", retval);
> > - pci_proc_detach_device(dev);
> > - pci_remove_sysfs_dev_files(dev);
>
> Thanks for catching my bug!
>
> > - return;
> > - }
> >
> > pci_dev_assign_added(dev, true);
> > }
> > --
> > 2.27.0.212.ge8ba1cc988-goog
> >
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 2/2] pci: Add parameter to disable attaching untrusted devices
Hello,
Thanks for taking a look.
On Fri, Jun 26, 2020 at 7:18 AM Greg Kroah-Hartman
wrote:
>
> On Thu, Jun 25, 2020 at 05:27:10PM -0700, Rajat Jain wrote:
> > Introduce a PCI parameter that disables the automatic attachment of
> > untrusted devices to their drivers.
> >
> > Signed-off-by: Rajat Jain
> > ---
> > Context:
> >
> > I set out to implement the approach outlined in
> > https://lkml.org/lkml/2020/6/9/1331
> > https://lkml.org/lkml/2020/6/15/1453
> >
> > But to my surprise, I found that the new hotplugged PCI devices
> > were getting automatically attached to drivers even though
> > /sys/bus/pci/drivers_autoprobe was set to 0.
> >
> > I realized that the device core's "drivers_autoprobe":
> >
> > * only disables the *initial* probe of the device (i.e. from
> > device_add()). If a subsystem calls device_attach() explicitly
> > for its devices like PCI subsystem does, the drivers_autoprobe
> > setting does not matter. The core will attach device to the driver.
> > This looks like correct semantic behavior to me because PCI is
> > explicitly calling device_attach(), which is a way to explicitly
> > ask the core to find and attach a driver for a device.
> >
> > * "drivers_autoprobe" cannot be controlled at boot time (to restrict
> > any drivers before userspace comes up).
> >
> > The options I considered were:
> >
> > 1) Change device_attach() so that it takes into consideration the
> > drivers_autoprobe property. Not sure if this is semantically correct
> > thing to do though. If I do this, then the only way a driver can
> > be attached to the drivers would be via userspace
> > (/sys/bus/pci/drivers/bind) (Good for our use case though!).
>
> This is the correct thing to do here, haven't I been asking you do move
> this logic into the driver core so that all busses can use it?
(please see below)
>
> > 2) Make the drivers_autoprobe property available to PCI to use
> > (currently it is private to device core). The PCI could use this
> > to determine whether or not to call device_attach(). This still
> > leaves the other problem (of not being able to set
> > drivers_autoprobe via command line open).
>
> Ick, command lines are horrible, don't do that if at all possible. On
> some systems they are not able to be changed which can be good or bad...
(please see below)
>
> > 3) I found the pci_dev->match_driver, which seemed similar to what I
> > am trying to do, but can't be controlled from userspace. I considered
> > populating that field based on drivers_autoprobe (still need (2)).
> > But the problem is that there is the AMD IOMMU driver which is setting
> > this independently, so setting the match_driver based on
> > drivers_autoprobe may not be a good idea. May be we can populate it
> > for untrusted devicesi, based on the parameter that I'm introducing?
> >
> > 4) This patch was my option 4 that helps fix both the problems for me.
>
> I suggest putting some of the above text in the changelog, as it has a
> lot of good context, while your existing changelog is pretty sparse and
> does not explain anything...
Will do.
>
>
> >
> > drivers/pci/bus.c | 11 ---
> > drivers/pci/pci.c | 9 +
> > drivers/pci/pci.h | 1 +
> > 3 files changed, 18 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> > index 3cef835b375fd..336aeeb4c4ebf 100644
> > --- a/drivers/pci/bus.c
> > +++ b/drivers/pci/bus.c
> > @@ -321,9 +321,14 @@ void pci_bus_add_device(struct pci_dev *dev)
> > pci_bridge_d3_update(dev);
> >
> > dev->match_driver = true;
> > - retval = device_attach(>dev);
> > - if (retval < 0 && retval != -EPROBE_DEFER)
> > - pci_warn(dev, "device attach failed (%d)\n", retval);
> > +
> > + if (dev->untrusted && pci_dont_attach_untrusted_devs) {
> > + pci_info(dev, "not attaching untrusted device\n");
> > + } else {
> > + retval = device_attach(>dev);
> > + if (retval < 0 && retval != -EPROBE_DEFER)
> > + pci_warn(dev, "device attach failed (%d)\n", retval);
> > + }
> >
> > pci_dev_assign_added(dev, true);
> > }
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index ce096272f52b1..dec1f9ef27d71 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -127,6 +127,13 @@ static bool pcie_ats_disabled;
> > /* If set, the PCI config space of each device is printed during boot. */
> > bool pci_early_dump;
> >
> > +/*
> > + * If set, the devices with "untrusted" flag shall not be attached
> > automatically
> > + * Userspace will need to attach them manually:
> > + * echo > /sys/bus/pci/drivers//bind
> > + */
> > +bool pci_dont_attach_untrusted_devs;
> > +
> > bool pci_ats_disabled(void)
> > {
> > return pcie_ats_disabled;
> > @@ -6522,6 +6529,8 @@ static int __init pci_setup(char *str)
> >
[PATCH 2/2] pci: Add parameter to disable attaching untrusted devices
Introduce a PCI parameter that disables the automatic attachment of
untrusted devices to their drivers.
Signed-off-by: Rajat Jain
---
Context:
I set out to implement the approach outlined in
https://lkml.org/lkml/2020/6/9/1331
https://lkml.org/lkml/2020/6/15/1453
But to my surprise, I found that the new hotplugged PCI devices
were getting automatically attached to drivers even though
/sys/bus/pci/drivers_autoprobe was set to 0.
I realized that the device core's "drivers_autoprobe":
* only disables the *initial* probe of the device (i.e. from
device_add()). If a subsystem calls device_attach() explicitly
for its devices like PCI subsystem does, the drivers_autoprobe
setting does not matter. The core will attach device to the driver.
This looks like correct semantic behavior to me because PCI is
explicitly calling device_attach(), which is a way to explicitly
ask the core to find and attach a driver for a device.
* "drivers_autoprobe" cannot be controlled at boot time (to restrict
any drivers before userspace comes up).
The options I considered were:
1) Change device_attach() so that it takes into consideration the
drivers_autoprobe property. Not sure if this is semantically correct
thing to do though. If I do this, then the only way a driver can
be attached to the drivers would be via userspace
(/sys/bus/pci/drivers/bind) (Good for our use case though!).
2) Make the drivers_autoprobe property available to PCI to use
(currently it is private to device core). The PCI could use this
to determine whether or not to call device_attach(). This still
leaves the other problem (of not being able to set
drivers_autoprobe via command line open).
3) I found the pci_dev->match_driver, which seemed similar to what I
am trying to do, but can't be controlled from userspace. I considered
populating that field based on drivers_autoprobe (still need (2)).
But the problem is that there is the AMD IOMMU driver which is setting
this independently, so setting the match_driver based on
drivers_autoprobe may not be a good idea. May be we can populate it
for untrusted devicesi, based on the parameter that I'm introducing?
4) This patch was my option 4 that helps fix both the problems for me.
drivers/pci/bus.c | 11 ---
drivers/pci/pci.c | 9 +
drivers/pci/pci.h | 1 +
3 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 3cef835b375fd..336aeeb4c4ebf 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -321,9 +321,14 @@ void pci_bus_add_device(struct pci_dev *dev)
pci_bridge_d3_update(dev);
dev->match_driver = true;
- retval = device_attach(>dev);
- if (retval < 0 && retval != -EPROBE_DEFER)
- pci_warn(dev, "device attach failed (%d)\n", retval);
+
+ if (dev->untrusted && pci_dont_attach_untrusted_devs) {
+ pci_info(dev, "not attaching untrusted device\n");
+ } else {
+ retval = device_attach(>dev);
+ if (retval < 0 && retval != -EPROBE_DEFER)
+ pci_warn(dev, "device attach failed (%d)\n", retval);
+ }
pci_dev_assign_added(dev, true);
}
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index ce096272f52b1..dec1f9ef27d71 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -127,6 +127,13 @@ static bool pcie_ats_disabled;
/* If set, the PCI config space of each device is printed during boot. */
bool pci_early_dump;
+/*
+ * If set, the devices with "untrusted" flag shall not be attached
automatically
+ * Userspace will need to attach them manually:
+ * echo > /sys/bus/pci/drivers//bind
+ */
+bool pci_dont_attach_untrusted_devs;
+
bool pci_ats_disabled(void)
{
return pcie_ats_disabled;
@@ -6522,6 +6529,8 @@ static int __init pci_setup(char *str)
pci_add_flags(PCI_SCAN_ALL_PCIE_DEVS);
} else if (!strncmp(str, "disable_acs_redir=", 18)) {
disable_acs_redir_param = str + 18;
+ } else if (!strcmp(str, "dont_attach_untrusted_devs")) {
+ pci_dont_attach_untrusted_devs = true;
} else {
pr_err("PCI: Unknown option `%s'\n", str);
}
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 6d3f758671064..30ffad047d926 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -13,6 +13,7 @@
extern const unsigned char pcie_link_speed[];
extern bool pci_early_dump;
+extern bool pci_dont_attach_untrusted_devs;
bool pcie_cap_has_lnkctl(const struct pci_dev *dev);
bool pcie_cap_has_rtctl(const struct pci_dev *dev);
--
2.27.0.212.ge8ba1cc988-goog
___
iommu mailing list
iommu@lists.linux-foundation.org
[PATCH 1/2] pci: Add pci device even if the driver failed to attach
device_attach() returning failure indicates a driver error
while trying to probe the device. In such a scenario, the PCI
device should still be added in the system and be visible to
the user.
This patch partially reverts:
commit ab1a187bba5c ("PCI: Check device_attach() return value always")
Signed-off-by: Rajat Jain
---
drivers/pci/bus.c | 6 +-
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 8e40b3e6da77d..3cef835b375fd 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -322,12 +322,8 @@ void pci_bus_add_device(struct pci_dev *dev)
dev->match_driver = true;
retval = device_attach(>dev);
- if (retval < 0 && retval != -EPROBE_DEFER) {
+ if (retval < 0 && retval != -EPROBE_DEFER)
pci_warn(dev, "device attach failed (%d)\n", retval);
- pci_proc_detach_device(dev);
- pci_remove_sysfs_dev_files(dev);
- return;
- }
pci_dev_assign_added(dev, true);
}
--
2.27.0.212.ge8ba1cc988-goog
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/4] pci: acs: Enable PCI_ACS_TB for untrusted/external-facing devices
Hi Ashok,
On Fri, Jun 19, 2020 at 9:10 AM Raj, Ashok 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
> > ---
> > 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
> >
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 4/4] pci: export untrusted attribute in sysfs
Hi Bjorn, All,
Thank you so much for your helpful review and inputs.
On Mon, Jun 15, 2020 at 6:17 PM 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
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
> _attr_driver_override.attr,
> _attr_ari_enabled.attr,
> + _attr_untrusted.attr,
> NULL,
> };
>
> --
> 2.27.0.290.gba653c62da-goog
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 4/4] pci: export untrusted attribute in sysfs
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 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
> > > wrote:
> > > >
> > > > On Thu, Jun 18, 2020 at 11:36 AM Greg Kroah-Hartman
> > > > 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
> > > > > > wrote:
> > > > > > > On Wed, Jun 17, 2020 at 12:31 AM Christoph Hellwig
> > > > > > > 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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 4/4] pci: export untrusted attribute in sysfs
Hello,
On Thu, Jun 18, 2020 at 2:14 AM Andy Shevchenko
wrote:
>
> On Thu, Jun 18, 2020 at 11:36 AM Greg Kroah-Hartman
> 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 wrote:
> > > > On Wed, Jun 17, 2020 at 12:31 AM Christoph Hellwig
> > > > 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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 4/4] pci: export untrusted attribute in sysfs
Hi Greg, Christoph, On Wed, Jun 17, 2020 at 12:31 AM Christoph Hellwig 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? (_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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 4/4] pci: export untrusted attribute in sysfs
On Tue, Jun 16, 2020 at 12:32 AM Christoph Hellwig 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? (_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. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 2/4] pci: set "untrusted" flag for truly external devices only
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
---
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, "ExternalFacingPort", ))
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 intuntrusted: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 intexternal_facing:1;
unsigned intbroken_intx_masking:1; /* INTx masking can't be used */
unsigned intio_window_1k:1; /* Intel bridge 1K I/O windows
*/
unsigned intirq_managed:1;
--
2.27.0.290.gba653c62da-goog
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 4/4] pci: export untrusted attribute in sysfs
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
---
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
_attr_driver_override.attr,
_attr_ari_enabled.attr,
+ _attr_untrusted.attr,
NULL,
};
--
2.27.0.290.gba653c62da-goog
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 1/4] pci: Keep the ACS capability offset in device
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
---
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 =
[PATCH 3/4] pci: acs: Enable PCI_ACS_TB for untrusted/external-facing devices
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
---
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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v4] iommu/vt-d: Don't apply gfx quirks to untrusted devices
Currently, an external malicious PCI device can masquerade the VID:PID
of faulty gfx devices, and thus apply iommu quirks to effectively
disable the IOMMU restrictions for itself.
Thus we need to ensure that the device we are applying quirks to, is
indeed an internal trusted device.
Signed-off-by: Rajat Jain
Acked-by: Lu Baolu
Reviewed-by: Ashok Raj
---
v4: - Add Ashok Raj's "Reviewed-by"
- Use pci_info() and split debug print cleanly into 2 statements.
v3: - Separate out the warning mesage in a function to be called from
other places. Change the warning string as suggested.
v2: - Change the warning print strings.
- Add Lu Baolu's acknowledgement.
drivers/iommu/intel-iommu.c | 37 +
1 file changed, 37 insertions(+)
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index ef0a5246700e5..efd1e5de947b9 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -6185,6 +6185,23 @@ intel_iommu_domain_set_attr(struct iommu_domain *domain,
return ret;
}
+/*
+ * Check that the device does not live on an external facing PCI port that is
+ * marked as untrusted. Such devices should not be able to apply quirks and
+ * thus not be able to bypass the IOMMU restrictions.
+ */
+static bool risky_device(struct pci_dev *pdev)
+{
+ if (pdev->untrusted) {
+ pci_info(pdev,
+"Skipping IOMMU quirk for dev [%04X:%04X] on untrusted
PCI link\n",
+pdev->vendor, pdev->device);
+ pci_info(pdev, "Please check with your BIOS/Platform vendor
about this\n");
+ return true;
+ }
+ return false;
+}
+
const struct iommu_ops intel_iommu_ops = {
.capable= intel_iommu_capable,
.domain_alloc = intel_iommu_domain_alloc,
@@ -6214,6 +6231,9 @@ const struct iommu_ops intel_iommu_ops = {
static void quirk_iommu_igfx(struct pci_dev *dev)
{
+ if (risky_device(dev))
+ return;
+
pci_info(dev, "Disabling IOMMU for graphics on this chipset\n");
dmar_map_gfx = 0;
}
@@ -6255,6 +6275,9 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x163D,
quirk_iommu_igfx);
static void quirk_iommu_rwbf(struct pci_dev *dev)
{
+ if (risky_device(dev))
+ return;
+
/*
* Mobile 4 Series Chipset neglects to set RWBF capability,
* but needs it. Same seems to hold for the desktop versions.
@@ -6285,6 +6308,9 @@ static void quirk_calpella_no_shadow_gtt(struct pci_dev
*dev)
{
unsigned short ggc;
+ if (risky_device(dev))
+ return;
+
if (pci_read_config_word(dev, GGC, ))
return;
@@ -6318,6 +6344,12 @@ static void __init check_tylersburg_isoch(void)
pdev = pci_get_device(PCI_VENDOR_ID_INTEL, 0x3a3e, NULL);
if (!pdev)
return;
+
+ if (risky_device(pdev)) {
+ pci_dev_put(pdev);
+ return;
+ }
+
pci_dev_put(pdev);
/* System Management Registers. Might be hidden, in which case
@@ -6327,6 +6359,11 @@ static void __init check_tylersburg_isoch(void)
if (!pdev)
return;
+ if (risky_device(pdev)) {
+ pci_dev_put(pdev);
+ return;
+ }
+
if (pci_read_config_dword(pdev, 0x188, )) {
pci_dev_put(pdev);
return;
--
2.27.0.rc2.251.g90737beb825-goog
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3] iommu/vt-d: Don't apply gfx quirks to untrusted devices
On Tue, Jun 2, 2020 at 10:30 PM Mika Westerberg
wrote:
>
> On Tue, Jun 02, 2020 at 04:26:02PM -0700, Rajat Jain wrote:
> > +static bool risky_device(struct pci_dev *pdev)
> > +{
> > + if (pdev->untrusted) {
> > + pci_warn(pdev,
> > + "Skipping IOMMU quirk for dev (%04X:%04X) on
> > untrusted"
> > + " PCI link. Please check with your BIOS/Platform"
> > + " vendor about this\n", pdev->vendor, pdev->device);
>
> You should not break user visible strings like this. It makes grepping
> for them harder (see also CodingStyle). You can write it like this instead:
>
> pci_info(pdev, "Skipping IOMMU quirk for dev (%04X:%04X) on untrusted
> PCI link\n",
> pdev->vendor, pdev->device);
> pci_info(pdev, "Please check with your BIOS/Platform vendor about
> this\n");
>
> Also I guess pci_info() might be better here after all. Your call :)
Done, sent the updated patch.
Thanks,
Rajat
>
> Rest of the patch looks good to me.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v3] iommu/vt-d: Don't apply gfx quirks to untrusted devices
Currently, an external malicious PCI device can masquerade the VID:PID
of faulty gfx devices, and thus apply iommu quirks to effectively
disable the IOMMU restrictions for itself.
Thus we need to ensure that the device we are applying quirks to, is
indeed an internal trusted device.
Signed-off-by: Rajat Jain
Acked-by: Lu Baolu
---
v3: - Separate out the warning mesage in a function to be called from
other places. Change the warning string as suggested.
v2: - Change the warning print strings.
- Add Lu Baolu's acknowledgement.
drivers/iommu/intel-iommu.c | 37 +
1 file changed, 37 insertions(+)
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index ef0a5246700e5..dc859f02985a0 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -6185,6 +6185,23 @@ intel_iommu_domain_set_attr(struct iommu_domain *domain,
return ret;
}
+/*
+ * Check that the device does not live on an external facing PCI port that is
+ * marked as untrusted. Such devices should not be able to apply quirks and
+ * thus not be able to bypass the IOMMU restrictions.
+ */
+static bool risky_device(struct pci_dev *pdev)
+{
+ if (pdev->untrusted) {
+ pci_warn(pdev,
+"Skipping IOMMU quirk for dev (%04X:%04X) on untrusted"
+" PCI link. Please check with your BIOS/Platform"
+" vendor about this\n", pdev->vendor, pdev->device);
+ return true;
+ }
+ return false;
+}
+
const struct iommu_ops intel_iommu_ops = {
.capable= intel_iommu_capable,
.domain_alloc = intel_iommu_domain_alloc,
@@ -6214,6 +6231,9 @@ const struct iommu_ops intel_iommu_ops = {
static void quirk_iommu_igfx(struct pci_dev *dev)
{
+ if (risky_device(dev))
+ return;
+
pci_info(dev, "Disabling IOMMU for graphics on this chipset\n");
dmar_map_gfx = 0;
}
@@ -6255,6 +6275,9 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x163D,
quirk_iommu_igfx);
static void quirk_iommu_rwbf(struct pci_dev *dev)
{
+ if (risky_device(dev))
+ return;
+
/*
* Mobile 4 Series Chipset neglects to set RWBF capability,
* but needs it. Same seems to hold for the desktop versions.
@@ -6285,6 +6308,9 @@ static void quirk_calpella_no_shadow_gtt(struct pci_dev
*dev)
{
unsigned short ggc;
+ if (risky_device(dev))
+ return;
+
if (pci_read_config_word(dev, GGC, ))
return;
@@ -6318,6 +6344,12 @@ static void __init check_tylersburg_isoch(void)
pdev = pci_get_device(PCI_VENDOR_ID_INTEL, 0x3a3e, NULL);
if (!pdev)
return;
+
+ if (risky_device(pdev)) {
+ pci_dev_put(pdev);
+ return;
+ }
+
pci_dev_put(pdev);
/* System Management Registers. Might be hidden, in which case
@@ -6327,6 +6359,11 @@ static void __init check_tylersburg_isoch(void)
if (!pdev)
return;
+ if (risky_device(pdev)) {
+ pci_dev_put(pdev);
+ return;
+ }
+
if (pci_read_config_dword(pdev, 0x188, )) {
pci_dev_put(pdev);
return;
--
2.27.0.rc2.251.g90737beb825-goog
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/vt-d: Don't apply gfx quirks to untrusted devices
Hi MIka,
Thanks for taking a look.
On Tue, Jun 2, 2020 at 2:50 AM Mika Westerberg
wrote:
>
> On Mon, Jun 01, 2020 at 10:45:17PM -0700, Rajat Jain wrote:
> > Currently, an external malicious PCI device can masquerade the VID:PID
> > of faulty gfx devices, and thus apply iommu quirks to effectively
> > disable the IOMMU restrictions for itself.
> >
> > Thus we need to ensure that the device we are applying quirks to, is
> > indeed an internal trusted device.
> >
> > Signed-off-by: Rajat Jain
> > ---
> > drivers/iommu/intel-iommu.c | 28
> > 1 file changed, 28 insertions(+)
> >
> > diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> > index ef0a5246700e5..f2a480168a02f 100644
> > --- a/drivers/iommu/intel-iommu.c
> > +++ b/drivers/iommu/intel-iommu.c
> > @@ -6214,6 +6214,11 @@ const struct iommu_ops intel_iommu_ops = {
> >
> > static void quirk_iommu_igfx(struct pci_dev *dev)
> > {
> > + if (dev->untrusted) {
> > + pci_warn(dev, "skipping iommu quirk for untrusted gfx dev\n");
>
> I think you should be consistent with other messages. For example iommu
> should be spelled IOMMU as done below.
>
> Also this is visible to users so maybe put bit more information there:
>
> pci_warn(dev, "Will not apply IOMMU quirk for untrusted graphics device\n");
>
> Ditto for all the other places. Also is "untrusted" good word here? If
> an ordinary user sees this will it trigger some sort of panic reaction.
> Perhaps we should call it "potentially untrusted" or something like
> that?
Fixed it, posted new patch at
https://lkml.org/lkml/2020/6/2/822
Thanks,
Rajat
>
> > + return;
> > + }
> > +
> > pci_info(dev, "Disabling IOMMU for graphics on this chipset\n");
> > dmar_map_gfx = 0;
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2] iommu/vt-d: Don't apply gfx quirks to untrusted devices
Currently, an external malicious PCI device can masquerade the VID:PID
of faulty gfx devices, and thus apply iommu quirks to effectively
disable the IOMMU restrictions for itself.
Thus we need to ensure that the device we are applying quirks to, is
indeed an internal trusted device.
Signed-off-by: Rajat Jain
Acked-by: Lu Baolu
---
V2: - Change the warning print strings.
- Add Lu Baolu's acknowledgement.
drivers/iommu/intel-iommu.c | 38 +
1 file changed, 38 insertions(+)
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index ef0a5246700e5..fdfbea4ff8cb3 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -6214,6 +6214,13 @@ const struct iommu_ops intel_iommu_ops = {
static void quirk_iommu_igfx(struct pci_dev *dev)
{
+ if (dev->untrusted) {
+ pci_warn(dev,
+"Skipping IOMMU quirk %s() for potentially untrusted
device\n",
+__func__);
+ return;
+ }
+
pci_info(dev, "Disabling IOMMU for graphics on this chipset\n");
dmar_map_gfx = 0;
}
@@ -6255,6 +6262,13 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x163D,
quirk_iommu_igfx);
static void quirk_iommu_rwbf(struct pci_dev *dev)
{
+ if (dev->untrusted) {
+ pci_warn(dev,
+"Skipping IOMMU quirk %s() for potentially untrusted
device\n",
+__func__);
+ return;
+ }
+
/*
* Mobile 4 Series Chipset neglects to set RWBF capability,
* but needs it. Same seems to hold for the desktop versions.
@@ -6285,6 +6299,13 @@ static void quirk_calpella_no_shadow_gtt(struct pci_dev
*dev)
{
unsigned short ggc;
+ if (dev->untrusted) {
+ pci_warn(dev,
+"Skipping IOMMU quirk %s() for potentially untrusted
device\n",
+__func__);
+ return;
+ }
+
if (pci_read_config_word(dev, GGC, ))
return;
@@ -6318,6 +6339,15 @@ static void __init check_tylersburg_isoch(void)
pdev = pci_get_device(PCI_VENDOR_ID_INTEL, 0x3a3e, NULL);
if (!pdev)
return;
+
+ if (pdev->untrusted) {
+ pci_warn(pdev,
+"Skipping IOMMU quirk %s() for potentially untrusted
device\n",
+__func__);
+ pci_dev_put(pdev);
+ return;
+ }
+
pci_dev_put(pdev);
/* System Management Registers. Might be hidden, in which case
@@ -6327,6 +6357,14 @@ static void __init check_tylersburg_isoch(void)
if (!pdev)
return;
+ if (pdev->untrusted) {
+ pci_warn(pdev,
+"Skipping IOMMU quirk %s() for potentially untrusted
device\n",
+__func__);
+ pci_dev_put(pdev);
+ return;
+ }
+
if (pci_read_config_dword(pdev, 0x188, )) {
pci_dev_put(pdev);
return;
--
2.27.0.rc2.251.g90737beb825-goog
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] iommu/vt-d: Don't apply gfx quirks to untrusted devices
Currently, an external malicious PCI device can masquerade the VID:PID
of faulty gfx devices, and thus apply iommu quirks to effectively
disable the IOMMU restrictions for itself.
Thus we need to ensure that the device we are applying quirks to, is
indeed an internal trusted device.
Signed-off-by: Rajat Jain
---
drivers/iommu/intel-iommu.c | 28
1 file changed, 28 insertions(+)
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index ef0a5246700e5..f2a480168a02f 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -6214,6 +6214,11 @@ const struct iommu_ops intel_iommu_ops = {
static void quirk_iommu_igfx(struct pci_dev *dev)
{
+ if (dev->untrusted) {
+ pci_warn(dev, "skipping iommu quirk for untrusted gfx dev\n");
+ return;
+ }
+
pci_info(dev, "Disabling IOMMU for graphics on this chipset\n");
dmar_map_gfx = 0;
}
@@ -6255,6 +6260,11 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x163D,
quirk_iommu_igfx);
static void quirk_iommu_rwbf(struct pci_dev *dev)
{
+ if (dev->untrusted) {
+ pci_warn(dev, "skipping iommu quirk for untrusted dev\n");
+ return;
+ }
+
/*
* Mobile 4 Series Chipset neglects to set RWBF capability,
* but needs it. Same seems to hold for the desktop versions.
@@ -6285,6 +6295,11 @@ static void quirk_calpella_no_shadow_gtt(struct pci_dev
*dev)
{
unsigned short ggc;
+ if (dev->untrusted) {
+ pci_warn(dev, "skipping iommu quirk for untrusted gfx dev\n");
+ return;
+ }
+
if (pci_read_config_word(dev, GGC, ))
return;
@@ -6318,6 +6333,13 @@ static void __init check_tylersburg_isoch(void)
pdev = pci_get_device(PCI_VENDOR_ID_INTEL, 0x3a3e, NULL);
if (!pdev)
return;
+
+ if (pdev->untrusted) {
+ pci_warn(pdev, "skipping iommu quirk due to untrusted dev\n");
+ pci_dev_put(pdev);
+ return;
+ }
+
pci_dev_put(pdev);
/* System Management Registers. Might be hidden, in which case
@@ -6327,6 +6349,12 @@ static void __init check_tylersburg_isoch(void)
if (!pdev)
return;
+ if (pdev->untrusted) {
+ pci_warn(pdev, "skipping iommu quirk due to untrusted dev\n");
+ pci_dev_put(pdev);
+ return;
+ }
+
if (pci_read_config_dword(pdev, 0x188, )) {
pci_dev_put(pdev);
return;
--
2.27.0.rc2.251.g90737beb825-goog
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
