Re: [PATCH v4 0/2] intel_iommu: fix guest-triggerable assert in MMIO handlers

Re: [PATCH v4 0/2] intel_iommu: fix guest-triggerable assert in MMIO handlers

[Yi Liu]

On 5/15/26 02:07, Junjie Cao wrote:
> An 8-byte guest access to a 32-bit-only VT-d register hits
> assert(size == 4) and aborts QEMU.  Found by generic-fuzz.
> 
> v1: https://lore.kernel.org/all/20260420170523.17908-1-junjie.cao@intel.com/
> v2: https://lore.kernel.org/all/20260424201842.176953-1-junjie.cao@intel.com/
> v3: https://lore.kernel.org/all/20260506031942.251335-1-junjie.cao@intel.com/
> 
> Changes in v4:
>    - Switch the guest-error log from error_report_once() to
>      qemu_log_mask(LOG_GUEST_ERROR, ...) so it is surfaced only
>      under -d guest_errors (Zhenzhong).
>    - Add a block comment at each of the 4 reachable sites
>      (FECTL 0x38, IECTL 0xa0, IEADDR 0xa8, PECTL 0xe0)
>      explaining why the check must stay, so future readers do
>      not delete it as "harmless" (Yi).
>    - No functional change beyond the logging-API swap.
> 
> Changes in v3:
>    - Drop v2's min_access_size=8 approach: per Zhenzhong, it
>      silently zero-extends 4-byte guest writes, wiping upper
>      wmask bits of 64-bit registers and firing triggers gated
>      on size==8.
>    - Keep min_access_size=4.  Remove the 25 assert(size == 4)
>      sites: 21 are unreachable (non-8-aligned), the 4 reachable
>      (FECTL 0x38, IECTL 0xa0, IEADDR 0xa8, PECTL 0xe0) fall
>      through to vtd_set_long() and log a guest error.
> 
> Junjie Cao (2):
>    intel_iommu: fix guest-triggerable abort on oversized MMIO access
>    tests/qtest: add 8-byte MMIO access sweep for intel-iommu
> 
>   hw/i386/intel_iommu.c          | 74 ++++++++++++++++++++++------------
>   tests/qtest/intel-iommu-test.c | 30 ++++++++++++++
>   2 files changed, 79 insertions(+), 25 deletions(-)
> 
> 
> base-commit: 5e61afe211e82a9af15a8794a0bd29bb574e953b

LGTM. Thanks.

Reviewed-by: Yi Liu <yi.l.liu@intel.com>

[PATCH v4 0/2] intel_iommu: fix guest-triggerable assert in MMIO handlers

[Junjie Cao]

An 8-byte guest access to a 32-bit-only VT-d register hits
assert(size == 4) and aborts QEMU.  Found by generic-fuzz.

v1: https://lore.kernel.org/all/20260420170523.17908-1-junjie.cao@intel.com/
v2: https://lore.kernel.org/all/20260424201842.176953-1-junjie.cao@intel.com/
v3: https://lore.kernel.org/all/20260506031942.251335-1-junjie.cao@intel.com/

Changes in v4:
  - Switch the guest-error log from error_report_once() to
    qemu_log_mask(LOG_GUEST_ERROR, ...) so it is surfaced only
    under -d guest_errors (Zhenzhong).
  - Add a block comment at each of the 4 reachable sites
    (FECTL 0x38, IECTL 0xa0, IEADDR 0xa8, PECTL 0xe0)
    explaining why the check must stay, so future readers do
    not delete it as "harmless" (Yi).
  - No functional change beyond the logging-API swap.

Changes in v3:
  - Drop v2's min_access_size=8 approach: per Zhenzhong, it
    silently zero-extends 4-byte guest writes, wiping upper
    wmask bits of 64-bit registers and firing triggers gated
    on size==8.
  - Keep min_access_size=4.  Remove the 25 assert(size == 4)
    sites: 21 are unreachable (non-8-aligned), the 4 reachable
    (FECTL 0x38, IECTL 0xa0, IEADDR 0xa8, PECTL 0xe0) fall
    through to vtd_set_long() and log a guest error.

Junjie Cao (2):
  intel_iommu: fix guest-triggerable abort on oversized MMIO access
  tests/qtest: add 8-byte MMIO access sweep for intel-iommu

 hw/i386/intel_iommu.c          | 74 ++++++++++++++++++++++------------
 tests/qtest/intel-iommu-test.c | 30 ++++++++++++++
 2 files changed, 79 insertions(+), 25 deletions(-)


base-commit: 5e61afe211e82a9af15a8794a0bd29bb574e953b
-- 
2.43.0

[PATCH v4 1/2] intel_iommu: fix guest-triggerable abort on oversized MMIO access

[Junjie Cao]

An 8-byte guest access to a 32-bit-only VT-d register hit
assert(size == 4) and aborted QEMU.  Remove all 25 asserts.

All 3 read-side and 18 of 22 write-side asserts are at
non-8-aligned offsets (unreachable, rejected by
memory_region_access_valid()) -- simply deleted.

The remaining 4, all writes at 8-aligned offsets, are
reachable: FECTL 0x38, IECTL 0xa0, IEADDR 0xa8, PECTL 0xe0.
Truncating the high half via vtd_set_long() matches prior
behavior; log under -d guest_errors since the VT-d spec is
silent on oversized accesses to 32-bit registers, and add a
comment so future maintainers don't delete the check as
"harmless".  min_access_size stays 4, so all size-based
branches on 64-bit register pairs are preserved.

Found by generic-fuzz (24 distinct crash seeds, all fixed).

Suggested-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Signed-off-by: Junjie Cao <junjie.cao@intel.com>
---
 hw/i386/intel_iommu.c | 74 ++++++++++++++++++++++++++++---------------
 1 file changed, 49 insertions(+), 25 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index b784c5f10a..17f897177d 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -21,6 +21,7 @@
 
 #include "qemu/osdep.h"
 #include "qemu/error-report.h"
+#include "qemu/log.h"
 #include "qemu/main-loop.h"
 #include "qapi/error.h"
 #include "hw/core/sysbus.h"
@@ -3713,7 +3714,6 @@ static uint64_t vtd_mem_read(void *opaque, hwaddr addr, unsigned size)
         break;
 
     case DMAR_RTADDR_REG_HI:
-        assert(size == 4);
         val = vtd_get_quad_raw(s, DMAR_RTADDR_REG) >> 32;
         break;
 
@@ -3728,12 +3728,10 @@ static uint64_t vtd_mem_read(void *opaque, hwaddr addr, unsigned size)
         break;
 
     case DMAR_IQA_REG_HI:
-        assert(size == 4);
         val = s->iq >> 32;
         break;
 
     case DMAR_PEUADDR_REG:
-        assert(size == 4);
         val = vtd_get_long_raw(s, DMAR_PEUADDR_REG);
         break;
 
@@ -3779,7 +3777,6 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
         break;
 
     case DMAR_CCMD_REG_HI:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         vtd_handle_ccmd_write(s);
         break;
@@ -3795,13 +3792,11 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
         break;
 
     case DMAR_IOTLB_REG_HI:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         vtd_handle_iotlb_write(s);
         break;
 
     case DMAR_PEUADDR_REG:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         break;
 
@@ -3815,27 +3810,35 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
         break;
 
     case DMAR_IVA_REG_HI:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         break;
 
     /* Fault Status Register, 32-bit */
     case DMAR_FSTS_REG:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         vtd_handle_fsts_write(s);
         break;
 
     /* Fault Event Control Register, 32-bit */
     case DMAR_FECTL_REG:
-        assert(size == 4);
+        /*
+         * 32-bit register at an 8-byte-aligned offset: a well-formed
+         * 8-byte guest access reaches this handler.  vtd_set_long()
+         * takes uint32_t and truncates the high half -- undefined per
+         * the VT-d spec but harmless here.  Flag it under
+         * -d guest_errors so the guest-side bug surfaces.
+         */
+        if (size != 4) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "%s: invalid %u-byte access to 32-bit reg "
+                          "addr=0x%" PRIx64 "\n", __func__, size, addr);
+        }
         vtd_set_long(s, addr, val);
         vtd_handle_fectl_write(s);
         break;
 
     /* Fault Event Data Register, 32-bit */
     case DMAR_FEDATA_REG:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         break;
 
@@ -3854,13 +3857,11 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
 
     /* Fault Event Upper Address Register, 32-bit */
     case DMAR_FEUADDR_REG:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         break;
 
     /* Protected Memory Enable Register, 32-bit */
     case DMAR_PMEN_REG:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         break;
 
@@ -3874,7 +3875,6 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
         break;
 
     case DMAR_RTADDR_REG_HI:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         break;
 
@@ -3889,7 +3889,6 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
         break;
 
     case DMAR_IQT_REG_HI:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         /* 19:63 of IQT_REG is RsvdZ, do nothing here */
         break;
@@ -3905,39 +3904,57 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
         break;
 
     case DMAR_IQA_REG_HI:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         break;
 
     /* Invalidation Completion Status Register, 32-bit */
     case DMAR_ICS_REG:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         vtd_handle_ics_write(s);
         break;
 
     /* Invalidation Event Control Register, 32-bit */
     case DMAR_IECTL_REG:
-        assert(size == 4);
+        /*
+         * 32-bit register at an 8-byte-aligned offset: a well-formed
+         * 8-byte guest access reaches this handler.  vtd_set_long()
+         * takes uint32_t and truncates the high half -- undefined per
+         * the VT-d spec but harmless here.  Flag it under
+         * -d guest_errors so the guest-side bug surfaces.
+         */
+        if (size != 4) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "%s: invalid %u-byte access to 32-bit reg "
+                          "addr=0x%" PRIx64 "\n", __func__, size, addr);
+        }
         vtd_set_long(s, addr, val);
         vtd_handle_iectl_write(s);
         break;
 
     /* Invalidation Event Data Register, 32-bit */
     case DMAR_IEDATA_REG:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         break;
 
     /* Invalidation Event Address Register, 32-bit */
     case DMAR_IEADDR_REG:
-        assert(size == 4);
+        /*
+         * 32-bit register at an 8-byte-aligned offset: a well-formed
+         * 8-byte guest access reaches this handler.  vtd_set_long()
+         * takes uint32_t and truncates the high half -- undefined per
+         * the VT-d spec but harmless here.  Flag it under
+         * -d guest_errors so the guest-side bug surfaces.
+         */
+        if (size != 4) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "%s: invalid %u-byte access to 32-bit reg "
+                          "addr=0x%" PRIx64 "\n", __func__, size, addr);
+        }
         vtd_set_long(s, addr, val);
         break;
 
     /* Invalidation Event Upper Address Register, 32-bit */
     case DMAR_IEUADDR_REG:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         break;
 
@@ -3951,7 +3968,6 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
         break;
 
     case DMAR_FRCD_REG_0_1:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         break;
 
@@ -3966,7 +3982,6 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
         break;
 
     case DMAR_FRCD_REG_0_3:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         /* May clear bit 127 (Fault), update PPF */
         vtd_update_fsts_ppf(s);
@@ -3981,18 +3996,27 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
         break;
 
     case DMAR_IRTA_REG_HI:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         break;
 
     case DMAR_PRS_REG:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         vtd_handle_prs_write(s);
         break;
 
     case DMAR_PECTL_REG:
-        assert(size == 4);
+        /*
+         * 32-bit register at an 8-byte-aligned offset: a well-formed
+         * 8-byte guest access reaches this handler.  vtd_set_long()
+         * takes uint32_t and truncates the high half -- undefined per
+         * the VT-d spec but harmless here.  Flag it under
+         * -d guest_errors so the guest-side bug surfaces.
+         */
+        if (size != 4) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "%s: invalid %u-byte access to 32-bit reg "
+                          "addr=0x%" PRIx64 "\n", __func__, size, addr);
+        }
         vtd_set_long(s, addr, val);
         vtd_handle_pectl_write(s);
         break;
-- 
2.43.0

[PATCH v4 2/2] tests/qtest: add 8-byte MMIO access sweep for intel-iommu

[Junjie Cao]

Sweep every 4-byte-aligned offset in the VT-d MMIO register space
with 8-byte reads and writes to verify that no register handler
aborts on an oversized access.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Junjie Cao <junjie.cao@intel.com>
---
 tests/qtest/intel-iommu-test.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/tests/qtest/intel-iommu-test.c b/tests/qtest/intel-iommu-test.c
index e5cc6acaf0..b1763ed294 100644
--- a/tests/qtest/intel-iommu-test.c
+++ b/tests/qtest/intel-iommu-test.c
@@ -17,11 +17,39 @@
 #define ECAP_STAGE_1_FIXED1   (VTD_ECAP_QI |  VTD_ECAP_IR | VTD_ECAP_IRO | \
                               VTD_ECAP_MHMV | VTD_ECAP_SMTS | VTD_ECAP_FSTS)
 
+static inline uint32_t vtd_reg_readl(QTestState *s, uint64_t offset)
+{
+    return qtest_readl(s, Q35_HOST_BRIDGE_IOMMU_ADDR + offset);
+}
+
 static inline uint64_t vtd_reg_readq(QTestState *s, uint64_t offset)
 {
     return qtest_readq(s, Q35_HOST_BRIDGE_IOMMU_ADDR + offset);
 }
 
+static inline void vtd_reg_writeq(QTestState *s, uint64_t offset,
+                                  uint64_t value)
+{
+    qtest_writeq(s, Q35_HOST_BRIDGE_IOMMU_ADDR + offset, value);
+}
+
+static void test_intel_iommu_8byte_access(void)
+{
+    QTestState *s;
+    uint64_t off;
+
+    s = qtest_init("-M q35 -device intel-iommu");
+
+    for (off = 0; off < DMAR_REG_SIZE; off += 4) {
+        vtd_reg_readq(s, off);
+        vtd_reg_writeq(s, off, 0);
+    }
+
+    g_assert_cmpuint(vtd_reg_readl(s, DMAR_VER_REG), !=, 0);
+
+    qtest_quit(s);
+}
+
 static void test_intel_iommu_stage_1(void)
 {
     uint8_t init_csr[DMAR_REG_SIZE];     /* register values */
@@ -58,6 +86,8 @@ static void test_intel_iommu_stage_1(void)
 int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
+    qtest_add_func("/q35/intel-iommu/8byte-access",
+                   test_intel_iommu_8byte_access);
     qtest_add_func("/q35/intel-iommu/stage-1", test_intel_iommu_stage_1);
 
     return g_test_run();
-- 
2.43.0