[PATCH] iommu/virtio: bound the probe-property walk to the probe buffer

[PATCH] iommu/virtio: bound the probe-property walk to the probe buffer

[Yehyeong Lee]

viommu_probe_endpoint() walks the properties the device returns in the
PROBE request, advancing a cursor by each property's device-provided
length.  The loop only checks that the start of a property header lies
within probe_size, but viommu_add_resv_mem() then dereferences a full
struct virtio_iommu_probe_resv_mem, reading mem->start and mem->end,
before its own length check runs.  A property placed near the end of the
buffer, or a probe_size smaller than the property, makes that read run
past the end of the probe allocation.

A malicious or buggy device triggers this with a RESV_MEM property at the
tail of the buffer.  KASAN reports a slab-out-of-bounds read, and the
out-of-bounds bytes become the start and end of a reserved region that is
then exposed to userspace through
/sys/kernel/iommu_groups/*/reserved_regions:

[    0.894779] BUG: KASAN: slab-out-of-bounds in viommu_probe_device+0x8b4/0xb20
[    0.894788] Read of size 8 at addr ffff88800a2eba4c by task swapper/0/1
[    0.894791]
[    0.894794] CPU: 1 UID: 0 PID: 1 Comm: swapper/0 Not tainted 7.3.0-rc6-00063-g0c2669a9f4a1 #1 PREEMPT(lazy)
[    0.894800] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.17.0-0-gb52ca86e094d-prebuilt.qemu.org 04/01/2014
[    0.894802] Call Trace:
[    0.894805]  <TASK>
[    0.894807]  dump_stack_lvl+0x66/0xa0
[    0.894821]  print_report+0xd0/0x630
[    0.894827]  ? viommu_probe_device+0x8b4/0xb20
[    0.894831]  ? __virt_addr_valid+0x209/0x3f0
[    0.894837]  ? viommu_probe_device+0x8b4/0xb20
[    0.894840]  kasan_report+0xe4/0x120
[    0.894844]  ? viommu_probe_device+0x8b4/0xb20
[    0.894849]  viommu_probe_device+0x8b4/0xb20
[    0.894855]  __iommu_probe_device+0x298/0x12f0
[    0.894860]  ? __pfx___iommu_probe_device+0x10/0x10
[    0.894863]  ? lockdep_hardirqs_on_prepare+0xdc/0x190
[    0.894869]  ? trace_hardirqs_on+0x18/0x160
[    0.894875]  ? __pfx_probe_iommu_group+0x10/0x10
[    0.894879]  probe_iommu_group+0x25/0x50
[    0.894882]  bus_for_each_dev+0x103/0x180
[    0.894887]  ? __pfx_bus_for_each_dev+0x10/0x10
[    0.894890]  ? iommu_device_register+0x195/0x680
[    0.894893]  ? lock_release+0xc6/0x280
[    0.894899]  iommu_device_register+0x1d7/0x680
[    0.894903]  ? __pfx_iommu_device_register+0x10/0x10
[    0.894907]  ? __asan_memset+0x23/0x50
[    0.894913]  viommu_probe+0x1001/0x1460
[    0.894919]  ? __pfx_viommu_probe+0x10/0x10
[    0.894924]  ? lock_acquire+0x18d/0x300
[    0.894929]  ? __pfx_viommu_event_handler+0x10/0x10
[    0.894933]  ? vp_modern_set_extended_features+0x85/0xb0
[    0.894938]  ? vp_set_status+0x20/0x70
[    0.894944]  virtio_dev_probe+0x569/0xbe0
[    0.894950]  ? kernfs_add_one+0xeb/0x7a0
[    0.894956]  ? __pfx_virtio_dev_probe+0x10/0x10
[    0.894961]  ? kernfs_create_link+0x169/0x230
[    0.894965]  ? kernfs_put+0x17/0x30
[    0.894970]  ? sysfs_do_create_link_sd+0x89/0x100
[    0.894976]  really_probe+0x1c3/0x6a0
[    0.894981]  __driver_probe_device+0x19e/0x3b0
[    0.894985]  ? trace_hardirqs_on+0x18/0x160
[    0.894990]  driver_probe_device+0x45/0xd0
[    0.894994]  __driver_attach+0x149/0x3e0
[    0.894998]  ? __pfx___driver_attach+0x10/0x10
[    0.895002]  bus_for_each_dev+0x103/0x180
[    0.895006]  ? __pfx_bus_for_each_dev+0x10/0x10
[    0.895008]  ? bus_add_driver+0x1d1/0x500
[    0.895012]  ? lock_release+0xc6/0x280
[    0.895018]  bus_add_driver+0x20b/0x500
[    0.895022]  driver_register+0x12f/0x450
[    0.895027]  ? __pfx_virtio_iommu_drv_init+0x10/0x10
[    0.895032]  do_one_initcall+0xc9/0x440
[    0.895037]  ? __pfx_do_one_initcall+0x10/0x10
[    0.895042]  ? __kmalloc_noprof+0x40d/0x680
[    0.895047]  ? kernel_init_freeable+0x385/0x8e0
[    0.895053]  kernel_init_freeable+0x4c3/0x8e0
[    0.895057]  ? __pfx_kernel_init+0x10/0x10
[    0.895063]  kernel_init+0x1f/0x1e0
[    0.895068]  ? _raw_spin_unlock_irq+0x23/0x40
[    0.895072]  ? __pfx_kernel_init+0x10/0x10
[    0.895077]  ret_from_fork+0x52e/0x780
[    0.895081]  ? __pfx_ret_from_fork+0x10/0x10
[    0.895085]  ? __switch_to+0x572/0xde0
[    0.895091]  ? __pfx_kernel_init+0x10/0x10
[    0.895096]  ret_from_fork_asm+0x1a/0x30
[    0.895103]  </TASK>
[    0.895104]
[    0.895105] Allocated by task 1:
[    0.895108]  kasan_save_stack+0x33/0x60
[    0.895111]  kasan_save_track+0x14/0x30
[    0.895114]  __kasan_kmalloc+0x8f/0xa0
[    0.895117]  __kmalloc_noprof+0x264/0x680
[    0.895120]  viommu_probe_device+0x2e3/0xb20
[    0.895123]  __iommu_probe_device+0x298/0x12f0
[    0.895126]  probe_iommu_group+0x25/0x50
[    0.895129]  bus_for_each_dev+0x103/0x180
[    0.895131]  iommu_device_register+0x1d7/0x680
[    0.895134]  viommu_probe+0x1001/0x1460
[    0.895137]  virtio_dev_probe+0x569/0xbe0
[    0.895141]  really_probe+0x1c3/0x6a0
[    0.895144]  __driver_probe_device+0x19e/0x3b0
[    0.895148]  driver_probe_device+0x45/0xd0
[    0.895151]  __driver_attach+0x149/0x3e0
[    0.895154]  bus_for_each_dev+0x103/0x180
[    0.895160]  bus_add_driver+0x20b/0x500
[    0.895163]  driver_register+0x12f/0x450
[    0.895166]  do_one_initcall+0xc9/0x440
[    0.895166]  kernel_init_freeable+0x4c3/0x8e0
[    0.895166]  kernel_init+0x1f/0x1e0
[    0.895166]  ret_from_fork+0x52e/0x780
[    0.895166]  ret_from_fork_asm+0x1a/0x30
[    0.895166]
[    0.895166] The buggy address belongs to the object at ffff88800a2eb800
[    0.895166]  which belongs to the cache kmalloc-1k of size 1024
[    0.895166] The buggy address is located 0 bytes to the right of
[    0.895166]  allocated 588-byte region [ffff88800a2eb800, ffff88800a2eba4c)
[    0.895166]
[    0.895166] The buggy address belongs to the physical page:
[    0.895166] page: refcount:0 mapcount:0 mapping:0000000000000000 index:0xffff88800a2ee000 pfn:0xa2e8
[    0.895166] head: order:3 mapcount:0 entire_mapcount:0 nr_pages_mapped:0 pincount:0
[    0.895166] flags: 0x100000000000240(workingset|head|node=0|zone=1)
[    0.895166] page_type: f5(slab)
[    0.895166] raw: 0100000000000240 ffff888008c41dc0 ffff888008c40888 ffff888008c40888
[    0.895166] raw: ffff88800a2ee000 000000000010000c 00000000f5000000 0000000000000000
[    0.895166] head: 0100000000000240 ffff888008c41dc0 ffff888008c40888 ffff888008c40888
[    0.895166] head: ffff88800a2ee000 000000000010000c 00000000f5000000 0000000000000000
[    0.895166] head: 0100000000000003 fffffffffffffe01 00000000ffffffff 00000000ffffffff
[    0.895166] head: 0000000000000000 0000000000000000 00000000ffffffff 0000000000000000
[    0.895166] page dumped because: kasan: bad access detected
[    0.895166]
[    0.895166] Memory state around the buggy address:
[    0.895166]  ffff88800a2eb900: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[    0.895166]  ffff88800a2eb980: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[    0.895166] >ffff88800a2eba00: 00 00 00 00 00 00 00 00 00 04 fc fc fc fc fc fc
[    0.895166]                                               ^
[    0.895166]  ffff88800a2eba80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[    0.895166]  ffff88800a2ebb00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[    0.895166] ==================================================================

Bound the walk so a property cannot extend past probe_size, and move the
length check in viommu_add_resv_mem() ahead of the dereference.  The
property length is device-controlled, so widen it so the addition cannot
wrap.

Fixes: 2a5a314874450d ("iommu/virtio: Add probe request")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/iommu/virtio-iommu.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index 587fc13197f12..dbc997a6e3568 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -489,6 +489,9 @@ static int viommu_add_resv_mem(struct viommu_endpoint *vdev,
 	struct iommu_resv_region *region = NULL, *next;
 	unsigned long prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
 
+	if (len < sizeof(*mem))
+		return -EINVAL;
+
 	start = start64 = le64_to_cpu(mem->start);
 	end = end64 = le64_to_cpu(mem->end);
 	size = end64 - start64 + 1;
@@ -497,9 +500,6 @@ static int viommu_add_resv_mem(struct viommu_endpoint *vdev,
 	if (start != start64 || end != end64 || size < end64 - start64)
 		return -EOVERFLOW;
 
-	if (len < sizeof(*mem))
-		return -EINVAL;
-
 	switch (mem->subtype) {
 	default:
 		dev_warn(vdev->dev, "unknown resv mem subtype 0x%x\n",
@@ -531,7 +531,8 @@ static int viommu_add_resv_mem(struct viommu_endpoint *vdev,
 static int viommu_probe_endpoint(struct viommu_dev *viommu, struct device *dev)
 {
 	int ret;
-	u16 type, len;
+	u16 type;
+	unsigned int len;
 	size_t cur = 0;
 	size_t probe_len;
 	struct virtio_iommu_req_probe *probe;
@@ -563,8 +564,10 @@ static int viommu_probe_endpoint(struct viommu_dev *viommu, struct device *dev)
 	type = le16_to_cpu(prop->type) & VIRTIO_IOMMU_PROBE_T_MASK;
 
 	while (type != VIRTIO_IOMMU_PROBE_T_NONE &&
-	       cur < viommu->probe_size) {
+	       cur + sizeof(*prop) <= viommu->probe_size) {
 		len = le16_to_cpu(prop->length) + sizeof(*prop);
+		if (len > viommu->probe_size - cur)
+			break;
 
 		switch (type) {
 		case VIRTIO_IOMMU_PROBE_T_RESV_MEM:
-- 
2.43.0

Re: [PATCH] iommu/virtio: bound the probe-property walk to the probe buffer

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] iommu/virtio: unaligned 64-bit access due to unvalidated property length
--

commit 1966676bc77a49f0f207bc2272bebf67d2bd8f06
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

iommu/virtio: bound the probe-property walk to the probe buffer

This commit prevents an out-of-bounds read in viommu_probe_endpoint() by
checking if the property fits within the probe allocation before
dereferencing it. It also moves the length check in viommu_add_resv_mem()
ahead of the memory accesses to avoid reading past the end of the
allocation.

> diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> index 587fc13197f12..dbc997a6e3568 100644
> --- a/drivers/iommu/virtio-iommu.c
> +++ b/drivers/iommu/virtio-iommu.c
> @@ -489,6 +489,9 @@ static int viommu_add_resv_mem(struct viommu_endpoint *vdev,
>  	struct iommu_resv_region *region = NULL, *next;
>  	unsigned long prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
>  
> +	if (len < sizeof(*mem))
> +		return -EINVAL;
> +
>  	start = start64 = le64_to_cpu(mem->start);
>  	end = end64 = le64_to_cpu(mem->end);

[Severity: High]
If a buggy or malicious device supplies an unaligned property length earlier
in the parsing loop, could the mem pointer passed to viommu_add_resv_mem()
be unaligned?

Because the virtio_iommu_probe_resv_mem structure is not packed, the
compiler assumes natural alignment. On architectures with strict alignment
requirements (such as ARM or SPARC), wouldn't these direct 64-bit
dereferences via le64_to_cpu() trigger a hardware alignment fault and lead
to a kernel panic?

[ ... ]

> @@ -563,8 +564,10 @@ static int viommu_probe_endpoint(struct viommu_dev *viommu, struct device *dev)
>  	type = le16_to_cpu(prop->type) & VIRTIO_IOMMU_PROBE_T_MASK;
>  
>  	while (type != VIRTIO_IOMMU_PROBE_T_NONE &&
> -	       cur < viommu->probe_size) {
> +	       cur + sizeof(*prop) <= viommu->probe_size) {
>  		len = le16_to_cpu(prop->length) + sizeof(*prop);
> +		if (len > viommu->probe_size - cur)
> +			break;

[Severity: High]
The virtio-iommu specification requires property lengths to be 8-byte
aligned, but this length validation does not enforce alignment.

Can an unaligned property length from the untrusted device advance the cur
offset incorrectly?

viommu_probe_endpoint()
    ...
    cur += len;

Wouldn't this cause subsequent property structures to be mapped to unaligned
memory addresses?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008094240.1082031-1-yhlee@isslab.korea.ac.kr?part=1

Re: [PATCH] iommu/virtio: bound the probe-property walk to the probe buffer

[Yehyeong Lee]

Thanks for the review.  Both points are the same concern: an unaligned property
making mem->start and mem->end unaligned.

This is pre-existing.  viommu_add_resv_mem() already performed the le64_to_cpu()
reads of mem->start and mem->end before this patch; the patch only adds the
bounds check in the walk and moves the existing length check ahead of those
reads, so it neither introduces nor widens the unaligned access.

And it does not fault on the architectures that build this driver.  VIRTIO_IOMMU
depends on (ARM64 || X86) (drivers/iommu/Kconfig), and both select
HAVE_EFFICIENT_UNALIGNED_ACCESS and read unaligned normal memory without
trapping.  I tested this on x86-64: with the RESV_MEM property forced to an odd
offset so mem->start and mem->end are misaligned, the values are read back
correctly with no alignment fault, both with and without this patch (I have not
run the same on arm64).  The 8-byte property alignment is a device-side
requirement; the driver does not need to rely on it, since an unaligned property
is still read correctly here.

If you would prefer the parser to be alignment-safe regardless, for instance for
a UBSAN_ALIGNMENT build, I can convert the device-provided reads to
get_unaligned_le64() as a follow-up.

Thanks,
Yehyeong

Re: [PATCH] iommu/virtio: bound the probe-property walk to the probe buffer

[Michael S. Tsirkin]

On Thu, Oct 08, 2026 at 09:56:03AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] iommu/virtio: unaligned 64-bit access due to unvalidated property length
> --
> 
> commit 1966676bc77a49f0f207bc2272bebf67d2bd8f06
> Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
> 
> iommu/virtio: bound the probe-property walk to the probe buffer
> 
> This commit prevents an out-of-bounds read in viommu_probe_endpoint() by
> checking if the property fits within the probe allocation before
> dereferencing it. It also moves the length check in viommu_add_resv_mem()
> ahead of the memory accesses to avoid reading past the end of the
> allocation.
> 
> > diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> > index 587fc13197f12..dbc997a6e3568 100644
> > --- a/drivers/iommu/virtio-iommu.c
> > +++ b/drivers/iommu/virtio-iommu.c
> > @@ -489,6 +489,9 @@ static int viommu_add_resv_mem(struct viommu_endpoint *vdev,
> >  	struct iommu_resv_region *region = NULL, *next;
> >  	unsigned long prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> >  
> > +	if (len < sizeof(*mem))
> > +		return -EINVAL;
> > +
> >  	start = start64 = le64_to_cpu(mem->start);
> >  	end = end64 = le64_to_cpu(mem->end);
> 
> [Severity: High]
> If a buggy or malicious device supplies an unaligned property length earlier
> in the parsing loop, could the mem pointer passed to viommu_add_resv_mem()
> be unaligned?
> 
> Because the virtio_iommu_probe_resv_mem structure is not packed, the
> compiler assumes natural alignment. On architectures with strict alignment
> requirements (such as ARM or SPARC), wouldn't these direct 64-bit
> dereferences via le64_to_cpu() trigger a hardware alignment fault and lead
> to a kernel panic?
> 
> [ ... ]
> 
> > @@ -563,8 +564,10 @@ static int viommu_probe_endpoint(struct viommu_dev *viommu, struct device *dev)
> >  	type = le16_to_cpu(prop->type) & VIRTIO_IOMMU_PROBE_T_MASK;
> >  
> >  	while (type != VIRTIO_IOMMU_PROBE_T_NONE &&
> > -	       cur < viommu->probe_size) {
> > +	       cur + sizeof(*prop) <= viommu->probe_size) {
> >  		len = le16_to_cpu(prop->length) + sizeof(*prop);
> > +		if (len > viommu->probe_size - cur)
> > +			break;
> 
> [Severity: High]
> The virtio-iommu specification requires property lengths to be 8-byte
> aligned,

Does it? Where in the spec?

> but this length validation does not enforce alignment.

it really shouldn't since it's not in the spec.

> Can an unaligned property length from the untrusted device advance the cur
> offset incorrectly?
> 
> viommu_probe_endpoint()
>     ...
>     cur += len;
> 
> Wouldn't this cause subsequent property structures to be mapped to unaligned
> memory addresses?
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261008094240.1082031-1-yhlee@isslab.korea.ac.kr?part=1


Indeed, it would be cleaner to use unaligned APIs, or memcpy the
structure. Pre-existing and not part of this patch.

-- 
MST