1. Patchew
  2. QEMU
  3. View series

[PATCH] amd_iommu: fix infinite loop

Paolo Bonzini posted 1 patch 2 weeks, 5 days ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20260511113923.2478812-1-pbonzini@redhat.com
Maintainers: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>, Sairaj Kodilkar <sarunkod@amd.com>, Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, "Michael S. Tsirkin" <mst@redhat.com>
hw/i386/amd_iommu.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
[PATCH] amd_iommu: fix infinite loop
Posted by Paolo Bonzini 2 weeks, 5 days ago 
The AMD IOMMU command buffer is a ring buffer of cmdbuf_len (a power
of two) entries.  Each entries is 16 bytes and the head pointer cycles
through the set:

The tail pointer is written by the guest through the COMMAND_TAIL MMIO
register (offset 0x2008); the while loop in amdvi_cmdbuf_run() only
terminates when head == tail.  If tail is set to a value higher than
cmdbuf_len * 16, head will cycle through all the elements of the ring
buffer indefinitely, without ever matching tail.  Fix this by further
masking tail (and head, for consistency) against the size of the
ring buffer.

Reported-by: Yunhe Wang <yunhewwww@163.com>
Cc: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/i386/amd_iommu.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 789e09d6f2b..197e452e3c3 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1578,7 +1578,8 @@ static inline void amdvi_handle_devtab_write(AMDVIState *s)
 static inline void amdvi_handle_cmdhead_write(AMDVIState *s)
 {
     s->cmdbuf_head = amdvi_readq(s, AMDVI_MMIO_COMMAND_HEAD)
-                     & AMDVI_MMIO_CMDBUF_HEAD_MASK;
+                     & AMDVI_MMIO_CMDBUF_HEAD_MASK
+                     & (s->cmdbuf_len * AMDVI_COMMAND_SIZE - 1);
     amdvi_cmdbuf_run(s);
 }
 
@@ -1594,7 +1595,8 @@ static inline void amdvi_handle_cmdbase_write(AMDVIState *s)
 static inline void amdvi_handle_cmdtail_write(AMDVIState *s)
 {
     s->cmdbuf_tail = amdvi_readq(s, AMDVI_MMIO_COMMAND_TAIL)
-                     & AMDVI_MMIO_CMDBUF_TAIL_MASK;
+                     & AMDVI_MMIO_CMDBUF_TAIL_MASK
+                     & (s->cmdbuf_len * AMDVI_COMMAND_SIZE - 1);
     amdvi_cmdbuf_run(s);
 }
 
-- 
2.54.0
Re: [PATCH] amd_iommu: fix infinite loop
Posted by Michael S. Tsirkin 3 days, 4 hours ago 
On Mon, May 11, 2026 at 01:39:22PM +0200, Paolo Bonzini wrote:
> The AMD IOMMU command buffer is a ring buffer of cmdbuf_len (a power
> of two) entries.  Each entries is 16 bytes and the head pointer cycles
> through the set:
> 
> The tail pointer is written by the guest through the COMMAND_TAIL MMIO
> register (offset 0x2008); the while loop in amdvi_cmdbuf_run() only
> terminates when head == tail.  If tail is set to a value higher than
> cmdbuf_len * 16, head will cycle through all the elements of the ring
> buffer indefinitely, without ever matching tail.  Fix this by further
> masking tail (and head, for consistency) against the size of the
> ring buffer.
> 
> Reported-by: Yunhe Wang <yunhewwww@163.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

in my tree now.

> ---
>  hw/i386/amd_iommu.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 789e09d6f2b..197e452e3c3 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -1578,7 +1578,8 @@ static inline void amdvi_handle_devtab_write(AMDVIState *s)
>  static inline void amdvi_handle_cmdhead_write(AMDVIState *s)
>  {
>      s->cmdbuf_head = amdvi_readq(s, AMDVI_MMIO_COMMAND_HEAD)
> -                     & AMDVI_MMIO_CMDBUF_HEAD_MASK;
> +                     & AMDVI_MMIO_CMDBUF_HEAD_MASK
> +                     & (s->cmdbuf_len * AMDVI_COMMAND_SIZE - 1);
>      amdvi_cmdbuf_run(s);
>  }
>  
> @@ -1594,7 +1595,8 @@ static inline void amdvi_handle_cmdbase_write(AMDVIState *s)
>  static inline void amdvi_handle_cmdtail_write(AMDVIState *s)
>  {
>      s->cmdbuf_tail = amdvi_readq(s, AMDVI_MMIO_COMMAND_TAIL)
> -                     & AMDVI_MMIO_CMDBUF_TAIL_MASK;
> +                     & AMDVI_MMIO_CMDBUF_TAIL_MASK
> +                     & (s->cmdbuf_len * AMDVI_COMMAND_SIZE - 1);
>      amdvi_cmdbuf_run(s);
>  }
>  
> -- 
> 2.54.0
Re: [PATCH] amd_iommu: fix infinite loop
Posted by Alejandro Jimenez 2 weeks, 4 days ago 
Hi Paolo,

A couple of comments/questions..

I'd suggest replacing the "fix infinite loop" commit subject with something
a bit more informative e.g.
"restrict command buffer head/tail ranges to ring size"

On 5/11/26 7:39 AM, Paolo Bonzini wrote:
> The AMD IOMMU command buffer is a ring buffer of cmdbuf_len (a power
> of two) entries.  Each entries is 16 bytes and the head pointer cycles
> through the set:
> 
Not sure if the ":" above meant the intention was to give an example here
with a series of offsets e.g.:
[0, 16, 32, ..., (cmdbuf_len - 1) * AMDVI_COMMAND_SIZE]

or just end the sentence there. I can fix it either way.

> The tail pointer is written by the guest through the COMMAND_TAIL MMIO
> register (offset 0x2008); the while loop in amdvi_cmdbuf_run() only
> terminates when head == tail.  If tail is set to a value higher than
> cmdbuf_len * 16, head will cycle through all the elements of the ring
> buffer indefinitely, without ever matching tail.  Fix this by further
> masking tail (and head, for consistency) against the size of the
> ring buffer.
> 
> Reported-by: Yunhe Wang <yunhewwww@163.com>

Was this reported in a public thread? I have not been able to find an
earlier discussion. I will send a follow up change patching an issue with
the head pointer handling in amdvi_cmdbuf_run(), so I'd like to check if I
missed any other similar issues.

Thank you,
Alejandro

> Cc: qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/i386/amd_iommu.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 789e09d6f2b..197e452e3c3 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -1578,7 +1578,8 @@ static inline void amdvi_handle_devtab_write(AMDVIState *s)
>  static inline void amdvi_handle_cmdhead_write(AMDVIState *s)
>  {
>      s->cmdbuf_head = amdvi_readq(s, AMDVI_MMIO_COMMAND_HEAD)
> -                     & AMDVI_MMIO_CMDBUF_HEAD_MASK;
> +                     & AMDVI_MMIO_CMDBUF_HEAD_MASK
> +                     & (s->cmdbuf_len * AMDVI_COMMAND_SIZE - 1);
>      amdvi_cmdbuf_run(s);
>  }
>  
> @@ -1594,7 +1595,8 @@ static inline void amdvi_handle_cmdbase_write(AMDVIState *s)
>  static inline void amdvi_handle_cmdtail_write(AMDVIState *s)
>  {
>      s->cmdbuf_tail = amdvi_readq(s, AMDVI_MMIO_COMMAND_TAIL)
> -                     & AMDVI_MMIO_CMDBUF_TAIL_MASK;
> +                     & AMDVI_MMIO_CMDBUF_TAIL_MASK
> +                     & (s->cmdbuf_len * AMDVI_COMMAND_SIZE - 1);
>      amdvi_cmdbuf_run(s);
>  }
>
Re: [PATCH] amd_iommu: fix infinite loop
Posted by Paolo Bonzini 3 days, 7 hours ago 
On 5/12/26 02:13, Alejandro Jimenez wrote:
> Hi Paolo,
> 
> A couple of comments/questions..
> 
> I'd suggest replacing the "fix infinite loop" commit subject with something
> a bit more informative e.g.
> "restrict command buffer head/tail ranges to ring size"

Ok.

> On 5/11/26 7:39 AM, Paolo Bonzini wrote:
>> The AMD IOMMU command buffer is a ring buffer of cmdbuf_len (a power
>> of two) entries.  Each entries is 16 bytes and the head pointer cycles
>> through the set:
>>
> Not sure if the ":" above meant the intention was to give an example here
> with a series of offsets e.g.:
> [0, 16, 32, ..., (cmdbuf_len - 1) * AMDVI_COMMAND_SIZE]
> 
> or just end the sentence there. I can fix it either way.

Yes, listing the offsets would work.

>> The tail pointer is written by the guest through the COMMAND_TAIL MMIO
>> register (offset 0x2008); the while loop in amdvi_cmdbuf_run() only
>> terminates when head == tail.  If tail is set to a value higher than
>> cmdbuf_len * 16, head will cycle through all the elements of the ring
>> buffer indefinitely, without ever matching tail.  Fix this by further
>> masking tail (and head, for consistency) against the size of the
>> ring buffer.
>>
>> Reported-by: Yunhe Wang <yunhewwww@163.com>
> 
> Was this reported in a public thread? I have not been able to find an
> earlier discussion. I will send a follow up change patching an issue with
> the head pointer handling in amdvi_cmdbuf_run(), so I'd like to check if I
> missed any other similar issues.

No, it was sent to the QEMU security mailing list.

I haven't seen the patch yet so please let me know if you'd like me to 
merge anything first.

Thanks,

Paolo
Re: [PATCH] amd_iommu: fix infinite loop
Posted by Alejandro Jimenez 3 days, 2 hours ago 

On 5/27/26 5:05 AM, Paolo Bonzini wrote:
> On 5/12/26 02:13, Alejandro Jimenez wrote:
>> Hi Paolo,
>>
>> A couple of comments/questions..
>>
>> I'd suggest replacing the "fix infinite loop" commit subject with something
>> a bit more informative e.g.
>> "restrict command buffer head/tail ranges to ring size"
> 
> Ok.
> 
>> On 5/11/26 7:39 AM, Paolo Bonzini wrote:
>>> The AMD IOMMU command buffer is a ring buffer of cmdbuf_len (a power
>>> of two) entries.  Each entries is 16 bytes and the head pointer cycles
>>> through the set:
>>>
>> Not sure if the ":" above meant the intention was to give an example here
>> with a series of offsets e.g.:
>> [0, 16, 32, ..., (cmdbuf_len - 1) * AMDVI_COMMAND_SIZE]
>>
>> or just end the sentence there. I can fix it either way.
> 
> Yes, listing the offsets would work.
> 

Sounds good. I'll make the changes in the commit message then.

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

>>> The tail pointer is written by the guest through the COMMAND_TAIL MMIO
>>> register (offset 0x2008); the while loop in amdvi_cmdbuf_run() only
>>> terminates when head == tail.  If tail is set to a value higher than
>>> cmdbuf_len * 16, head will cycle through all the elements of the ring
>>> buffer indefinitely, without ever matching tail.  Fix this by further
>>> masking tail (and head, for consistency) against the size of the
>>> ring buffer.
>>>
>>> Reported-by: Yunhe Wang <yunhewwww@163.com>
>>
>> Was this reported in a public thread? I have not been able to find an
>> earlier discussion. I will send a follow up change patching an issue with
>> the head pointer handling in amdvi_cmdbuf_run(), so I'd like to check if I
>> missed any other similar issues.
> 
> No, it was sent to the QEMU security mailing list.
> 
> I haven't seen the patch yet so please let me know if you'd like me to
> merge anything first.
> 
There is no dependency. I sent the follow up a while back:

https://lore.kernel.org/qemu-devel/20260512150044.334867-1-alejandro.j.jimenez@oracle.com/

and it was reviewed already.

Then Costas (CC'd here) found and fixed the same issue a few days later:

https://lore.kernel.org/qemu-devel/20260517192314.355499-1-costas.argyris@amd.com/

but they also added a test which I am going to review hopefully by EOW,
then I will add the change to my tree.

Thank you,
Alejandro

> Thanks,
> 
> Paolo
>

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.