All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] iommu: intel: Apply the device scope length check to every device scope walk
@ 2026-10-12  8:41 Shirong Zhao
  2026-10-16  0:45 ` Baolu Lu
  0 siblings, 1 reply; 2+ messages in thread
From: Shirong Zhao @ 2026-10-12  8:41 UTC (permalink / raw)
  To: dwmw2, baolu.lu, joro, will; +Cc: robin.murphy, iommu, linux-kernel

dmar_walk_remapping_entries() already refuses to step by a zero length:

	next = (void *)iter + iter->length;
	if (iter->length == 0) {
		/* Avoid looping forever on bad ACPI tables */
		pr_debug(FW_BUG "Invalid 0-length structure\n");
		break;
	}

Five walks over the same DMAR structures do not:

  dmar_alloc_dev_scope()        - counts namespace/endpoint/bridge scopes
  dmar_insert_dev_scope()       - attaches a scope to a PCI path
  dmar_acpi_insert_dev_scope()  - same, by ANDD enumeration id
  dmar_acpi_dev_scope_init()    - the ANDD entries at the end of the table
  ir_parse_ioapic_hpet_scope()  - IOAPIC and HPET scopes

Each advances its cursor by a length read out of the DMAR table, and
nothing constrains it.  A zero length leaves the cursor where it is and
the walk never ends; a short one steps through table payload a byte at a
time, reinterpreting it as scopes.  dmar_alloc_dev_scope() and
dmar_acpi_dev_scope_init() run on every boot with the IOMMU enabled, and a
short length in the latter hands andd->device_name to acpi_get_handle().

Use the check dmar_walk_remapping_entries() already uses.

Checked against torvalds/linux at HEAD; both files are unmodified there.

Found by EFSM.R17, which reports a loop whose cursor advances by a step
memory can supply as zero while the exit condition depends on that cursor.
Signed-off-by: Shirong Zhao <shxzhaosr@163.com>

---
 drivers/iommu/intel/dmar.c          | 16 ++++++++++++++++
 drivers/iommu/intel/irq_remapping.c |  5 +++++
 2 files changed, 21 insertions(+)

diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
index ba675b0..1615ef6 100644
--- a/drivers/iommu/intel/dmar.c
+++ b/drivers/iommu/intel/dmar.c
@@ -94,6 +94,10 @@ void *dmar_alloc_dev_scope(void *start, void *end, int *cnt)
 			scope->entry_type != ACPI_DMAR_SCOPE_TYPE_HPET) {
 			pr_warn("Unsupported device scope\n");
 		}
+		if (!scope->length) {
+			pr_warn(FW_BUG "Invalid 0-length device scope\n");
+			break;
+		}
 		start += scope->length;
 	}
 	if (*cnt == 0)
@@ -227,6 +231,10 @@ int dmar_insert_dev_scope(struct dmar_pci_notify_info *info,
 
 	for (; start < end; start += scope->length) {
 		scope = start;
+		if (!scope->length) {
+			pr_warn(FW_BUG "Invalid 0-length device scope\n");
+			break;
+		}
 		if (scope->entry_type != ACPI_DMAR_SCOPE_TYPE_ENDPOINT &&
 		    scope->entry_type != ACPI_DMAR_SCOPE_TYPE_BRIDGE)
 			continue;
@@ -743,6 +751,10 @@ static void __init dmar_acpi_insert_dev_scope(u8 device_number,
 		for (scope = (void *)(drhd + 1);
 		     (unsigned long)scope < ((unsigned long)drhd) + drhd->header.length;
 		     scope = ((void *)scope) + scope->length) {
+			if (!scope->length) {
+				pr_warn(FW_BUG "Invalid 0-length device scope\n");
+				break;
+			}
 			if (scope->entry_type != ACPI_DMAR_SCOPE_TYPE_NAMESPACE)
 				continue;
 			if (scope->enumeration_id != device_number)
@@ -778,6 +790,10 @@ static int __init dmar_acpi_dev_scope_init(void)
 	for (andd = (void *)dmar_tbl + sizeof(struct acpi_table_dmar);
 	     ((unsigned long)andd) < ((unsigned long)dmar_tbl) + dmar_tbl->length;
 	     andd = ((void *)andd) + andd->header.length) {
+		if (!andd->header.length) {
+			pr_warn(FW_BUG "Invalid 0-length namespace entry\n");
+			break;
+		}
 		if (andd->header.type == ACPI_DMAR_TYPE_NAMESPACE) {
 			acpi_handle h;
 			struct acpi_device *adev;
diff --git a/drivers/iommu/intel/irq_remapping.c b/drivers/iommu/intel/irq_remapping.c
index 25c26f7..884347c 100644
--- a/drivers/iommu/intel/irq_remapping.c
+++ b/drivers/iommu/intel/irq_remapping.c
@@ -933,6 +933,11 @@ static int ir_parse_ioapic_hpet_scope(struct acpi_dmar_header *header,
 			ret = ir_parse_one_ioapic_scope(scope, iommu, drhd);
 		else if (scope->entry_type == ACPI_DMAR_SCOPE_TYPE_HPET)
 			ret = ir_parse_one_hpet_scope(scope, iommu, drhd);
+		if (!scope->length) {
+			pr_warn(FW_BUG "Invalid 0-length device scope\n");
+			ret = -EINVAL;
+			break;
+		}
 		start += scope->length;
 	}
 


^ permalink raw reply related	[flat|nested] 2+ messages in thread

* Re: [PATCH] iommu: intel: Apply the device scope length check to every device scope walk
  2026-10-12  8:41 [PATCH] iommu: intel: Apply the device scope length check to every device scope walk Shirong Zhao
@ 2026-10-16  0:45 ` Baolu Lu
  0 siblings, 0 replies; 2+ messages in thread
From: Baolu Lu @ 2026-10-16  0:45 UTC (permalink / raw)
  To: Shirong Zhao, dwmw2, joro, will
  Cc: baolu.lu, robin.murphy, iommu, linux-kernel

On 10/12/2026 4:41 PM, Shirong Zhao wrote:
> dmar_walk_remapping_entries() already refuses to step by a zero length:
> 
> 	next = (void *)iter + iter->length;
> 	if (iter->length == 0) {
> 		/* Avoid looping forever on bad ACPI tables */
> 		pr_debug(FW_BUG "Invalid 0-length structure\n");
> 		break;
> 	}
> 
> Five walks over the same DMAR structures do not:
> 
>    dmar_alloc_dev_scope()        - counts namespace/endpoint/bridge scopes
>    dmar_insert_dev_scope()       - attaches a scope to a PCI path
>    dmar_acpi_insert_dev_scope()  - same, by ANDD enumeration id
>    dmar_acpi_dev_scope_init()    - the ANDD entries at the end of the table
>    ir_parse_ioapic_hpet_scope()  - IOAPIC and HPET scopes
> 
> Each advances its cursor by a length read out of the DMAR table, and
> nothing constrains it.  A zero length leaves the cursor where it is and
> the walk never ends; a short one steps through table payload a byte at a
> time, reinterpreting it as scopes.  dmar_alloc_dev_scope() and
> dmar_acpi_dev_scope_init() run on every boot with the IOMMU enabled, and a
> short length in the latter hands andd->device_name to acpi_get_handle().
> 
> Use the check dmar_walk_remapping_entries() already uses.
> 
> Checked against torvalds/linux at HEAD; both files are unmodified there.
> 
> Found by EFSM.R17, which reports a loop whose cursor advances by a step
> memory can supply as zero while the exit condition depends on that cursor.
> Signed-off-by: Shirong Zhao<shxzhaosr@163.com>
> 
> ---
>   drivers/iommu/intel/dmar.c          | 16 ++++++++++++++++
>   drivers/iommu/intel/irq_remapping.c |  5 +++++
>   2 files changed, 21 insertions(+)

A zero length is never valid under the VT-d spec, so such a DMAR table
is a firmware bug.

The existing check in dmar_walk_remapping_entries() already rejects a
zero-length top-level structure before any of these walks run, and I'm
not aware of any shipping platform with a malformed device scope.

Without a real-world report, I'd rather not add more checks for this.
Thanks for looking at this, though.

Thanks,
baolu

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2026-10-16  0:45 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-12  8:41 [PATCH] iommu: intel: Apply the device scope length check to every device scope walk Shirong Zhao
2026-10-16  0:45 ` Baolu Lu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.