| Message ID | 20200515104359.1178606-3-jean-philippe@linaro.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | PCI, iommu: Factor 'untrusted' check for ATS enablement | expand |
On Fri, May 15, 2020 at 12:44:00PM +0200, Jean-Philippe Brucker wrote: > The pci_ats_supported() function checks if a device supports ATS and is > allowed to use it. In addition to checking that the device has an ATS > capability and that the global pci=noats is not set > (pci_ats_disabled()), it also checks if a device is untrusted. Hmm, but per patch 1, pci_ats_supported() does not check pci_ats_disabled(), or do I miss something? Joerg
On Fri, May 15, 2020 at 02:01:50PM +0200, Joerg Roedel wrote: > On Fri, May 15, 2020 at 12:44:00PM +0200, Jean-Philippe Brucker wrote: > > The pci_ats_supported() function checks if a device supports ATS and is > > allowed to use it. In addition to checking that the device has an ATS > > capability and that the global pci=noats is not set > > (pci_ats_disabled()), it also checks if a device is untrusted. > > Hmm, but per patch 1, pci_ats_supported() does not check > pci_ats_disabled(), or do I miss something? The commit message isn't clear. pci_ats_init() sets dev->ats_cap only if !pci_ats_disabled(), so checking dev->ats_cap in pci_ats_supported() takes pci_ats_disabled() into account. Thanks, Jean
On Fri, May 15, 2020 at 02:11:24PM +0200, Jean-Philippe Brucker wrote: > On Fri, May 15, 2020 at 02:01:50PM +0200, Joerg Roedel wrote: > > Hmm, but per patch 1, pci_ats_supported() does not check > > pci_ats_disabled(), or do I miss something? > > The commit message isn't clear. pci_ats_init() sets dev->ats_cap only if > !pci_ats_disabled(), so checking dev->ats_cap in pci_ats_supported() > takes pci_ats_disabled() into account. Right, so the patch is fine: Reviewed-by: Joerg Roedel <jroedel@suse.de>
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c index 1dc3718560d0e8..8b7a9e811d33a6 100644 --- a/drivers/iommu/amd_iommu.c +++ b/drivers/iommu/amd_iommu.c @@ -313,16 +313,15 @@ static struct iommu_group *acpihid_device_group(struct device *dev) static bool pci_iommuv2_capable(struct pci_dev *pdev) { static const int caps[] = { - PCI_EXT_CAP_ID_ATS, PCI_EXT_CAP_ID_PRI, PCI_EXT_CAP_ID_PASID, }; int i, pos; - if (pci_ats_disabled()) + if (!pci_ats_supported(pdev)) return false; - for (i = 0; i < 3; ++i) { + for (i = 0; i < 2; ++i) { pos = pci_find_ext_capability(pdev, caps[i]); if (pos == 0) return false; @@ -3150,11 +3149,8 @@ int amd_iommu_device_info(struct pci_dev *pdev, memset(info, 0, sizeof(*info)); - if (!pci_ats_disabled()) { - pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ATS); - if (pos) - info->flags |= AMD_IOMMU_DEVICE_FLAG_ATS_SUP; - } + if (pci_ats_supported(pdev)) + info->flags |= AMD_IOMMU_DEVICE_FLAG_ATS_SUP; pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI); if (pos)
The pci_ats_supported() function checks if a device supports ATS and is allowed to use it. In addition to checking that the device has an ATS capability and that the global pci=noats is not set (pci_ats_disabled()), it also checks if a device is untrusted. A device is untrusted if it is plugged into an external-facing port such as Thunderbolt and could be spoofing an existing device to exploit weaknesses in the IOMMU configuration. By calling pci_ats_supported() we keep DTE[I]=0 for untrusted devices and abort transactions with Pretranslated Addresses. Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> --- drivers/iommu/amd_iommu.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)