From e461b8c991b9202b007ea2059d953e264240b0c9 Mon Sep 17 00:00:00 2001 From: Alexander Monakov Date: Mon, 11 May 2020 10:23:52 +0000 Subject: iommu/amd: Fix over-read of ACPI UID from IVRS table IVRS parsing code always tries to read 255 bytes from memory when retrieving ACPI device path, and makes an assumption that firmware provides a zero-terminated string. Both of those are bugs: the entry is likely to be shorter than 255 bytes, and zero-termination is not guaranteed. With Acer SF314-42 firmware these issues manifest visibly in dmesg: AMD-Vi: ivrs, add hid:AMDI0020, uid:\_SB.FUR0\xf0\xa5, rdevid:160 AMD-Vi: ivrs, add hid:AMDI0020, uid:\_SB.FUR1\xf0\xa5, rdevid:160 AMD-Vi: ivrs, add hid:AMDI0020, uid:\_SB.FUR2\xf0\xa5, rdevid:160 AMD-Vi: ivrs, add hid:AMDI0020, uid:\_SB.FUR3>\x83e\x8d\x9a\xd1... The first three lines show how the code over-reads adjacent table entries into the UID, and in the last line it even reads garbage data beyond the end of the IVRS table itself. Since each entry has the length of the UID (uidl member of ivhd_entry struct), use that for memcpy, and manually add a zero terminator. Avoid zero-filling hid and uid arrays up front, and instead ensure the uid array is always zero-terminated. No change needed for the hid array, as it was already properly zero-terminated. Fixes: 2a0cb4e2d423c ("iommu/amd: Add new map for storing IVHD dev entry type HID") Signed-off-by: Alexander Monakov Cc: Joerg Roedel Cc: iommu@lists.linux-foundation.org Link: https://lore.kernel.org/r/20200511102352.1831-1-amonakov@ispras.ru Signed-off-by: Joerg Roedel --- drivers/iommu/amd_iommu_init.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c index 2b9a67ecc6ac..5b81fd16f5fa 100644 --- a/drivers/iommu/amd_iommu_init.c +++ b/drivers/iommu/amd_iommu_init.c @@ -1329,8 +1329,8 @@ static int __init init_iommu_from_acpi(struct amd_iommu *iommu, } case IVHD_DEV_ACPI_HID: { u16 devid; - u8 hid[ACPIHID_HID_LEN] = {0}; - u8 uid[ACPIHID_UID_LEN] = {0}; + u8 hid[ACPIHID_HID_LEN]; + u8 uid[ACPIHID_UID_LEN]; int ret; if (h->type != 0x40) { @@ -1347,6 +1347,7 @@ static int __init init_iommu_from_acpi(struct amd_iommu *iommu, break; } + uid[0] = '\0'; switch (e->uidf) { case UID_NOT_PRESENT: @@ -1361,8 +1362,8 @@ static int __init init_iommu_from_acpi(struct amd_iommu *iommu, break; case UID_IS_CHARACTER: - memcpy(uid, (u8 *)(&e->uid), ACPIHID_UID_LEN - 1); - uid[ACPIHID_UID_LEN - 1] = '\0'; + memcpy(uid, &e->uid, e->uidl); + uid[e->uidl] = '\0'; break; default: -- cgit v1.2.3 From ea90228c7b2ae6646bb6381385229aabb6f14cd2 Mon Sep 17 00:00:00 2001 From: Raul E Rangel Date: Mon, 11 May 2020 10:33:36 -0600 Subject: iommu/amd: Fix get_acpihid_device_id() acpi_dev_hid_uid_match() expects a null pointer for UID if it doesn't exist. The acpihid_map_entry contains a char buffer for holding the UID. If no UID was provided in the IVRS table, this buffer will be zeroed. If we pass in a null string, acpi_dev_hid_uid_match() will return false because it will try and match an empty string to the ACPI UID of the device. Fixes: ae5e6c6439c3 ("iommu/amd: Switch to use acpi_dev_hid_uid_match()") Suggested-by: Andy Shevchenko Signed-off-by: Raul E Rangel Reviewed-by: Andy Shevchenko Link: https://lore.kernel.org/r/20200511103229.v2.1.I6f1b6f973ee6c8af1348611370c73a0ec0ea53f1@changeid Signed-off-by: Joerg Roedel --- drivers/iommu/amd_iommu.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c index 1dc3718560d0..2883ac389abb 100644 --- a/drivers/iommu/amd_iommu.c +++ b/drivers/iommu/amd_iommu.c @@ -127,7 +127,8 @@ static inline int get_acpihid_device_id(struct device *dev, return -ENODEV; list_for_each_entry(p, &acpihid_map, list) { - if (acpi_dev_hid_uid_match(adev, p->hid, p->uid)) { + if (acpi_dev_hid_uid_match(adev, p->hid, + p->uid[0] ? p->uid : NULL)) { if (entry) *entry = p; return p->devid; -- cgit v1.2.3 From bd421264ed307dd296eab036851221b225071a32 Mon Sep 17 00:00:00 2001 From: Joerg Roedel Date: Tue, 19 May 2020 15:03:40 +0200 Subject: iommu: Fix deferred domain attachment The IOMMU core code has support for deferring the attachment of a domain to a device. This is needed in kdump kernels where the new domain must not be attached to a device before the device driver takes it over. When the AMD IOMMU driver got converted to use the dma-iommu implementation, the deferred attaching got lost. The code in dma-iommu.c has support for deferred attaching, but it calls into iommu_attach_device() to actually do it. But iommu_attach_device() will check if the device should be deferred in it code-path and do nothing, breaking deferred attachment. Move the is_deferred_attach() check out of the attach_device path and into iommu_group_add_device() to make deferred attaching work from the dma-iommu code. Fixes: 795bbbb9b6f8 ("iommu/dma-iommu: Handle deferred devices") Reported-by: Jerry Snitselaar Suggested-by: Robin Murphy Signed-off-by: Joerg Roedel Tested-by: Jerry Snitselaar Cc: Jerry Snitselaar Cc: Tom Murphy Cc: Robin Murphy Link: https://lore.kernel.org/r/20200519130340.14564-1-joro@8bytes.org --- drivers/iommu/iommu.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 7b375421afba..1faa08c8bbb4 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -693,6 +693,15 @@ out: return ret; } +static bool iommu_is_attach_deferred(struct iommu_domain *domain, + struct device *dev) +{ + if (domain->ops->is_attach_deferred) + return domain->ops->is_attach_deferred(domain, dev); + + return false; +} + /** * iommu_group_add_device - add a device to an iommu group * @group: the group into which to add the device (reference should be held) @@ -747,7 +756,7 @@ rename: mutex_lock(&group->mutex); list_add_tail(&device->list, &group->devices); - if (group->domain) + if (group->domain && !iommu_is_attach_deferred(group->domain, dev)) ret = __iommu_attach_device(group->domain, dev); mutex_unlock(&group->mutex); if (ret) @@ -1653,9 +1662,6 @@ static int __iommu_attach_device(struct iommu_domain *domain, struct device *dev) { int ret; - if ((domain->ops->is_attach_deferred != NULL) && - domain->ops->is_attach_deferred(domain, dev)) - return 0; if (unlikely(domain->ops->attach_dev == NULL)) return -ENODEV; @@ -1727,8 +1733,7 @@ EXPORT_SYMBOL_GPL(iommu_sva_unbind_gpasid); static void __iommu_detach_device(struct iommu_domain *domain, struct device *dev) { - if ((domain->ops->is_attach_deferred != NULL) && - domain->ops->is_attach_deferred(domain, dev)) + if (iommu_is_attach_deferred(domain, dev)) return; if (unlikely(domain->ops->detach_dev == NULL)) -- cgit v1.2.3