>-----Original Message----- >From: Liu, Yi L <[email protected]> >Subject: Re: [PATCH v3 1/2] intel_iommu: fix guest-triggerable abort on >oversized >MMIO access > >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.
Maybe to take it as guest error and log it: qemu_log_mask(LOG_GUEST_ERROR,...), then people could catch it and fix guest code? Thanks Zhenzhong
