hw/i386/amd_iommu: Avoid undefined behavior in amdvi_setevent_bits()

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

Thomas Huth posted 1 patch 1 month, 4 weeks ago

Download mbox

Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20260731140229.259272-1-thuth@redhat.com

Maintainers: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>, Sairaj Kodilkar <sarunkod@amd.com>, "Michael S. Tsirkin" <mst@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>

hw/i386/amd_iommu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

Expand all Fold all

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

Posted by Thomas Huth 1 month, 4 weeks ago

From: Thomas Huth <thuth@redhat.com>

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).

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3633
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 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

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

Posted by Alejandro Jimenez 1 month, 3 weeks ago

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 <thuth@redhat.com>
> 
> 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 <thuth@redhat.com>

Reviewed-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>

> ---
>  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
>