[PATCH] amd_iommu: fix infinite loop

[PATCH] amd_iommu: fix infinite loop

[Paolo Bonzini]

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

[Alejandro Jimenez]

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

[Paolo Bonzini]

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

[Michael S. Tsirkin]

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

[Alejandro Jimenez]

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
>