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

[Yi Liu <yi.l.liu@intel.com>]

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 <zhenzhong.duan@intel.com>
> Signed-off-by: Junjie Cao <junjie.cao@intel.com>
> ---
>   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;