Re: [PATCH] hw/i386/amd_iommu: Avoid undefined behavior in amdvi_setevent_bits()

Hi Thomas,

nit: The commit subject prefix should simply be "amd_iommu".

On 7/31/26 10:02 AM, Thomas Huth wrote:
> From: Thomas Huth <[email protected]>
> 
> The code in amdvi_encode_event() calls amdvi_setevent_bits() with
> start = 64:
> 
>     amdvi_setevent_bits(evt, addr, 64, 64);
> 
> and amdvi_setevent_bits() then calculates:
> 
>     uint64_t mask = MAKE_64BIT_MASK(start, length);
> 
> but this MAKE_64BIT_MASK() macro shifts a value left by "start" bit
> positions. Shifting left by more than 63 is undefined behavior and
> could have unexpected results with different compilers / architectures.
> 
> Fix it by using "bitpos" instead, which was likely the original
> intended behavior anyway. (bitpos is calculated as bitpos = start % 64).
>


Fixes: 1d5b128cbeea ("hw/iommu: Fix problems reported by Coverity scan")

> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3633
> Signed-off-by: Thomas Huth <[email protected]>

Reviewed-by: Alejandro Jimenez <[email protected]>

> ---
>  hw/i386/amd_iommu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 90252c52af4..578c27ccbef 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -323,7 +323,7 @@ static void amdvi_setevent_bits(uint64_t *buffer, uint64_t value, int start,
>                                  int length)
>  {
>      int index = start / 64, bitpos = start % 64;
> -    uint64_t mask = MAKE_64BIT_MASK(start, length);
> +    uint64_t mask = MAKE_64BIT_MASK(bitpos, length);
>      buffer[index] &= ~mask;
>      buffer[index] |= (value << bitpos) & mask;
>  }
> -- 
> 2.55.0
>