On 5/6/26 11:19, Junjie Cao wrote:
An 8-byte guest access to a 32-bit-only VT-d register hit
assert(size == 4) and aborted QEMU. Remove all 25 asserts.
21 sit at non-8-byte-aligned offsets and are rejected by
memory_region_access_valid() before reaching the handler -- dead
code, simply deleted.
The remaining 4 guard 8-byte-aligned 32-bit registers reachable
by a well-formed 8-byte access (FECTL 0x38, IECTL 0xa0, IEADDR
0xa8, PECTL 0xe0).
could you share how you identify the 4 registers among all the
registers. The offset of such registers are multiple of 8? is it? Just
curious why only 4 registers.
Writes fall through to vtd_set_long(), which
takes uint32_t and implicitly truncates, and log a guest error.
Reads fall through to the default vtd_get_quad() -- equivalent
to two 4-byte reads and therefore harmless, no warn needed.
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 <[email protected]>
Signed-off-by: Junjie Cao <[email protected]>
---
hw/i386/intel_iommu.c | 41 ++++++++++++++++-------------------------
1 file changed, 16 insertions(+), 25 deletions(-)
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index f395fa248c..0fb89332f9 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3713,7 +3713,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 +3727,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 +3776,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 +3791,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 +3809,27 @@ 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);
+ if (size != 4) {
+ error_report_once("%s: invalid %u-byte access to 32-bit reg "
+ "addr=0x%" PRIx64, __func__, size, addr);
+ }
Since such writes are harmless, I don't think it is really helpful to
have this error log. Instead, we should have a comment here to avoid
somebody break it in future.
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 +3848,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 +3866,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 +3880,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 +3895,41 @@ 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);
+ if (size != 4) {
+ error_report_once("%s: invalid %u-byte access to 32-bit reg "
+ "addr=0x%" PRIx64, __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);
+ if (size != 4) {
+ error_report_once("%s: invalid %u-byte access to 32-bit reg "
+ "addr=0x%" PRIx64, __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 +3943,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 +3957,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 +3971,19 @@ 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);
+ if (size != 4) {
+ error_report_once("%s: invalid %u-byte access to 32-bit reg "
+ "addr=0x%" PRIx64, __func__, size, addr);
+ }
vtd_set_long(s, addr, val);
vtd_handle_pectl_write(s);
break;
