1. Patchew
  2. QEMU
  3. View series

[PATCH v2] hw/display/vmware_vga: Don't allow guest to trigger long running loop in host

Thomas Huth posted 1 patch 16 hours ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20260723124432.353969-1-thuth@redhat.com
Maintainers: Dmitry Fleytman <dmitry.fleytman@gmail.com>
hw/display/vmware_vga.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
[PATCH v2] hw/display/vmware_vga: Don't allow guest to trigger long running loop in host
Posted by Thomas Huth 16 hours ago 
From: Thomas Huth <thuth@redhat.com>

The code in the SVGA_CMD_DEFINE_ALPHA_CURSOR handler in vmsvga_fifo_run()
basically does:

            x = vmsvga_fifo_read(s);
            y = vmsvga_fifo_read(s);
            args = x * y;
            goto badcmd;
            ...
badcmd:
            len -= args;
            if (len < 0) {
                goto rewind;
            }
            while (args--) {
                vmsvga_fifo_read(s);
            }

Thus by supplying huge values for x and y that overflow the result of
the multiplication, the guest can trigger a long-running loop here
that burns the host's CPU cycles.

Add some sanity checks so that this cannot happen anymore.

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3782
Reported-by: Feifan Qian <bea1e@proton.me>
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4026
Reported-by: Tristan Madani <tristan@talencesecurity.com>
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4076
Reported-by: Sunday Jiang
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 v2: Use SVGA_MAX_WIDTH and SVGA_MAX_HEIGHT instead of an arbitrary value

 hw/display/vmware_vga.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index f6f9edfd1d9..567806f0e47 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -737,6 +737,10 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
             vmsvga_fifo_read(s);
             x = vmsvga_fifo_read(s);
             y = vmsvga_fifo_read(s);
+            if (x < 0 || x >= SVGA_MAX_WIDTH ||
+                y < 0 || y >= SVGA_MAX_HEIGHT) {
+                goto rewind;
+            }
             args = x * y;
             goto badcmd;
         case SVGA_CMD_RECT_ROP_FILL:
@@ -776,7 +780,7 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
             if (len < 0) {
                 goto rewind;
             }
-            while (args--) {
+            while (args-- > 0) {
                 vmsvga_fifo_read(s);
             }
             printf("%s: Unknown command 0x%02x in SVGA command FIFO\n",
-- 
2.55.0
Re: [PATCH v2] hw/display/vmware_vga: Don't allow guest to trigger long running loop in host
Posted by Michael Tokarev 13 hours ago 
On 7/23/26 15:44, Thomas Huth wrote:
> From: Thomas Huth <thuth@redhat.com>
> 
> The code in the SVGA_CMD_DEFINE_ALPHA_CURSOR handler in vmsvga_fifo_run()
> basically does:
> 
>              x = vmsvga_fifo_read(s);
>              y = vmsvga_fifo_read(s);
>              args = x * y;
>              goto badcmd;
>              ...
> badcmd:
>              len -= args;
>              if (len < 0) {
>                  goto rewind;
>              }
>              while (args--) {
>                  vmsvga_fifo_read(s);
>              }
> 
> Thus by supplying huge values for x and y that overflow the result of
> the multiplication, the guest can trigger a long-running loop here
> that burns the host's CPU cycles.
> 
> Add some sanity checks so that this cannot happen anymore.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3782
> Reported-by: Feifan Qian <bea1e@proton.me>
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4026
> Reported-by: Tristan Madani <tristan@talencesecurity.com>
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4076
> Reported-by: Sunday Jiang
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   v2: Use SVGA_MAX_WIDTH and SVGA_MAX_HEIGHT instead of an arbitrary value
> 
>   hw/display/vmware_vga.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
> index f6f9edfd1d9..567806f0e47 100644
> --- a/hw/display/vmware_vga.c
> +++ b/hw/display/vmware_vga.c
> @@ -737,6 +737,10 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
>               vmsvga_fifo_read(s);
>               x = vmsvga_fifo_read(s);
>               y = vmsvga_fifo_read(s);
> +            if (x < 0 || x >= SVGA_MAX_WIDTH ||
> +                y < 0 || y >= SVGA_MAX_HEIGHT) {

Applied to the trivial-patches tree, dropping <0 conditions here.

Thanks,

/mjt

> +                goto rewind;
> +            }
>               args = x * y;
>               goto badcmd;
>           case SVGA_CMD_RECT_ROP_FILL:
> @@ -776,7 +780,7 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
>               if (len < 0) {
>                   goto rewind;
>               }
> -            while (args--) {
> +            while (args-- > 0) {
>                   vmsvga_fifo_read(s);
>               }
>               printf("%s: Unknown command 0x%02x in SVGA command FIFO\n",
Re: [PATCH v2] hw/display/vmware_vga: Don't allow guest to trigger long running loop in host
Posted by Thomas Huth 12 hours ago 
On 23/07/2026 17.05, Michael Tokarev wrote:
> On 7/23/26 15:44, Thomas Huth wrote:
>> From: Thomas Huth <thuth@redhat.com>
>>
>> The code in the SVGA_CMD_DEFINE_ALPHA_CURSOR handler in vmsvga_fifo_run()
>> basically does:
>>
>>              x = vmsvga_fifo_read(s);
>>              y = vmsvga_fifo_read(s);
>>              args = x * y;
>>              goto badcmd;
>>              ...
>> badcmd:
>>              len -= args;
>>              if (len < 0) {
>>                  goto rewind;
>>              }
>>              while (args--) {
>>                  vmsvga_fifo_read(s);
>>              }
>>
>> Thus by supplying huge values for x and y that overflow the result of
>> the multiplication, the guest can trigger a long-running loop here
>> that burns the host's CPU cycles.
>>
>> Add some sanity checks so that this cannot happen anymore.
>>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3782
>> Reported-by: Feifan Qian <bea1e@proton.me>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4026
>> Reported-by: Tristan Madani <tristan@talencesecurity.com>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4076
>> Reported-by: Sunday Jiang
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   v2: Use SVGA_MAX_WIDTH and SVGA_MAX_HEIGHT instead of an arbitrary value
>>
>>   hw/display/vmware_vga.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
>> index f6f9edfd1d9..567806f0e47 100644
>> --- a/hw/display/vmware_vga.c
>> +++ b/hw/display/vmware_vga.c
>> @@ -737,6 +737,10 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
>>               vmsvga_fifo_read(s);
>>               x = vmsvga_fifo_read(s);
>>               y = vmsvga_fifo_read(s);
>> +            if (x < 0 || x >= SVGA_MAX_WIDTH ||
>> +                y < 0 || y >= SVGA_MAX_HEIGHT) {
> 
> Applied to the trivial-patches tree, dropping <0 conditions here.

Please don't drop the "< 0" ! x and y are signed variables, so we should 
better check this here.

  Thanks,
   Thomas
Re: [PATCH v2] hw/display/vmware_vga: Don't allow guest to trigger long running loop in host
Posted by Philippe Mathieu-Daudé 14 hours ago 
On 23/7/26 14:44, Thomas Huth wrote:
> From: Thomas Huth <thuth@redhat.com>
> 
> The code in the SVGA_CMD_DEFINE_ALPHA_CURSOR handler in vmsvga_fifo_run()
> basically does:
> 
>              x = vmsvga_fifo_read(s);
>              y = vmsvga_fifo_read(s);
>              args = x * y;
>              goto badcmd;
>              ...
> badcmd:
>              len -= args;
>              if (len < 0) {
>                  goto rewind;
>              }
>              while (args--) {
>                  vmsvga_fifo_read(s);
>              }
> 
> Thus by supplying huge values for x and y that overflow the result of
> the multiplication, the guest can trigger a long-running loop here
> that burns the host's CPU cycles.
> 
> Add some sanity checks so that this cannot happen anymore.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3782
> Reported-by: Feifan Qian <bea1e@proton.me>
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4026
> Reported-by: Tristan Madani <tristan@talencesecurity.com>
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4076
> Reported-by: Sunday Jiang
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   v2: Use SVGA_MAX_WIDTH and SVGA_MAX_HEIGHT instead of an arbitrary value
> 
>   hw/display/vmware_vga.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
> index f6f9edfd1d9..567806f0e47 100644
> --- a/hw/display/vmware_vga.c
> +++ b/hw/display/vmware_vga.c
> @@ -737,6 +737,10 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
>               vmsvga_fifo_read(s);
>               x = vmsvga_fifo_read(s);
>               y = vmsvga_fifo_read(s);
> +            if (x < 0 || x >= SVGA_MAX_WIDTH ||
> +                y < 0 || y >= SVGA_MAX_HEIGHT) {

vmsvga_fifo_read() returns unsigned... otherwise:
Reviewed-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>

> +                goto rewind;
> +            }
>               args = x * y;
>               goto badcmd;
>           case SVGA_CMD_RECT_ROP_FILL:
> @@ -776,7 +780,7 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
>               if (len < 0) {
>                   goto rewind;
>               }
> -            while (args--) {
> +            while (args-- > 0) {
>                   vmsvga_fifo_read(s);
>               }
>               printf("%s: Unknown command 0x%02x in SVGA command FIFO\n",
Re: [PATCH v2] hw/display/vmware_vga: Don't allow guest to trigger long running loop in host
Posted by Daniel P. Berrangé 13 hours ago 
On Thu, Jul 23, 2026 at 03:54:41PM +0200, Philippe Mathieu-Daudé wrote:
> On 23/7/26 14:44, Thomas Huth wrote:
> > From: Thomas Huth <thuth@redhat.com>
> > 
> > The code in the SVGA_CMD_DEFINE_ALPHA_CURSOR handler in vmsvga_fifo_run()
> > basically does:
> > 
> >              x = vmsvga_fifo_read(s);
> >              y = vmsvga_fifo_read(s);
> >              args = x * y;
> >              goto badcmd;
> >              ...
> > badcmd:
> >              len -= args;
> >              if (len < 0) {
> >                  goto rewind;
> >              }
> >              while (args--) {
> >                  vmsvga_fifo_read(s);
> >              }
> > 
> > Thus by supplying huge values for x and y that overflow the result of
> > the multiplication, the guest can trigger a long-running loop here
> > that burns the host's CPU cycles.
> > 
> > Add some sanity checks so that this cannot happen anymore.
> > 
> > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3782
> > Reported-by: Feifan Qian <bea1e@proton.me>
> > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4026
> > Reported-by: Tristan Madani <tristan@talencesecurity.com>
> > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4076
> > Reported-by: Sunday Jiang
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> >   v2: Use SVGA_MAX_WIDTH and SVGA_MAX_HEIGHT instead of an arbitrary value
> > 
> >   hw/display/vmware_vga.c | 6 +++++-
> >   1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
> > index f6f9edfd1d9..567806f0e47 100644
> > --- a/hw/display/vmware_vga.c
> > +++ b/hw/display/vmware_vga.c
> > @@ -737,6 +737,10 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
> >               vmsvga_fifo_read(s);
> >               x = vmsvga_fifo_read(s);
> >               y = vmsvga_fifo_read(s);
> > +            if (x < 0 || x >= SVGA_MAX_WIDTH ||
> > +                y < 0 || y >= SVGA_MAX_HEIGHT) {
> 
> vmsvga_fifo_read() returns unsigned... otherwise:

But x & y  are declared 'int', so at least on 32-bit builds
a large uint32_t value would wrap and become negative.
Although we dropped support for 32-bit platforms, IMHO it would
be better to use uint32_t for 'x' and 'y' too rather than
assuming the 'int' value won't be negative.

> Reviewed-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>
> 
> > +                goto rewind;
> > +            }
> >               args = x * y;
> >               goto badcmd;
> >           case SVGA_CMD_RECT_ROP_FILL:
> > @@ -776,7 +780,7 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
> >               if (len < 0) {
> >                   goto rewind;
> >               }
> > -            while (args--) {
> > +            while (args-- > 0) {
> >                   vmsvga_fifo_read(s);
> >               }
> >               printf("%s: Unknown command 0x%02x in SVGA command FIFO\n",
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|
Re: [PATCH v2] hw/display/vmware_vga: Don't allow guest to trigger long running loop in host
Posted by Thomas Huth 12 hours ago 
On 23/07/2026 17.16, Daniel P. Berrangé wrote:
> On Thu, Jul 23, 2026 at 03:54:41PM +0200, Philippe Mathieu-Daudé wrote:
>> On 23/7/26 14:44, Thomas Huth wrote:
>>> From: Thomas Huth <thuth@redhat.com>
>>>
>>> The code in the SVGA_CMD_DEFINE_ALPHA_CURSOR handler in vmsvga_fifo_run()
>>> basically does:
>>>
>>>               x = vmsvga_fifo_read(s);
>>>               y = vmsvga_fifo_read(s);
>>>               args = x * y;
>>>               goto badcmd;
>>>               ...
>>> badcmd:
>>>               len -= args;
>>>               if (len < 0) {
>>>                   goto rewind;
>>>               }
>>>               while (args--) {
>>>                   vmsvga_fifo_read(s);
>>>               }
>>>
>>> Thus by supplying huge values for x and y that overflow the result of
>>> the multiplication, the guest can trigger a long-running loop here
>>> that burns the host's CPU cycles.
>>>
>>> Add some sanity checks so that this cannot happen anymore.
>>>
>>> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3782
>>> Reported-by: Feifan Qian <bea1e@proton.me>
>>> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4026
>>> Reported-by: Tristan Madani <tristan@talencesecurity.com>
>>> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4076
>>> Reported-by: Sunday Jiang
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>    v2: Use SVGA_MAX_WIDTH and SVGA_MAX_HEIGHT instead of an arbitrary value
>>>
>>>    hw/display/vmware_vga.c | 6 +++++-
>>>    1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
>>> index f6f9edfd1d9..567806f0e47 100644
>>> --- a/hw/display/vmware_vga.c
>>> +++ b/hw/display/vmware_vga.c
>>> @@ -737,6 +737,10 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
>>>                vmsvga_fifo_read(s);
>>>                x = vmsvga_fifo_read(s);
>>>                y = vmsvga_fifo_read(s);
>>> +            if (x < 0 || x >= SVGA_MAX_WIDTH ||
>>> +                y < 0 || y >= SVGA_MAX_HEIGHT) {
>>
>> vmsvga_fifo_read() returns unsigned... otherwise:
> 
> But x & y  are declared 'int', so at least on 32-bit builds
> a large uint32_t value would wrap and become negative.
> Although we dropped support for 32-bit platforms, IMHO it would
> be better to use uint32_t for 'x' and 'y' too rather than
> assuming the 'int' value won't be negative.
x and y are used as signed int all over the place here ... so reworking that 
goes way beyond fixing this problem. Could we please get this patch merged 
first for 11.1, and if someone then still feels like reworking the code, 
this could be done for 11.2 ?

  Thomas
Re: [PATCH v2] hw/display/vmware_vga: Don't allow guest to trigger long running loop in host
Posted by Daniel P. Berrangé 12 hours ago 
On Thu, Jul 23, 2026 at 06:18:28PM +0200, Thomas Huth wrote:
> On 23/07/2026 17.16, Daniel P. Berrangé wrote:
> > On Thu, Jul 23, 2026 at 03:54:41PM +0200, Philippe Mathieu-Daudé wrote:
> > > On 23/7/26 14:44, Thomas Huth wrote:
> > > > From: Thomas Huth <thuth@redhat.com>
> > > > 
> > > > The code in the SVGA_CMD_DEFINE_ALPHA_CURSOR handler in vmsvga_fifo_run()
> > > > basically does:
> > > > 
> > > >               x = vmsvga_fifo_read(s);
> > > >               y = vmsvga_fifo_read(s);
> > > >               args = x * y;
> > > >               goto badcmd;
> > > >               ...
> > > > badcmd:
> > > >               len -= args;
> > > >               if (len < 0) {
> > > >                   goto rewind;
> > > >               }
> > > >               while (args--) {
> > > >                   vmsvga_fifo_read(s);
> > > >               }
> > > > 
> > > > Thus by supplying huge values for x and y that overflow the result of
> > > > the multiplication, the guest can trigger a long-running loop here
> > > > that burns the host's CPU cycles.
> > > > 
> > > > Add some sanity checks so that this cannot happen anymore.
> > > > 
> > > > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3782
> > > > Reported-by: Feifan Qian <bea1e@proton.me>
> > > > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4026
> > > > Reported-by: Tristan Madani <tristan@talencesecurity.com>
> > > > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4076
> > > > Reported-by: Sunday Jiang
> > > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > > > ---
> > > >    v2: Use SVGA_MAX_WIDTH and SVGA_MAX_HEIGHT instead of an arbitrary value
> > > > 
> > > >    hw/display/vmware_vga.c | 6 +++++-
> > > >    1 file changed, 5 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
> > > > index f6f9edfd1d9..567806f0e47 100644
> > > > --- a/hw/display/vmware_vga.c
> > > > +++ b/hw/display/vmware_vga.c
> > > > @@ -737,6 +737,10 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
> > > >                vmsvga_fifo_read(s);
> > > >                x = vmsvga_fifo_read(s);
> > > >                y = vmsvga_fifo_read(s);
> > > > +            if (x < 0 || x >= SVGA_MAX_WIDTH ||
> > > > +                y < 0 || y >= SVGA_MAX_HEIGHT) {
> > > 
> > > vmsvga_fifo_read() returns unsigned... otherwise:
> > 
> > But x & y  are declared 'int', so at least on 32-bit builds
> > a large uint32_t value would wrap and become negative.
> > Although we dropped support for 32-bit platforms, IMHO it would
> > be better to use uint32_t for 'x' and 'y' too rather than
> > assuming the 'int' value won't be negative.
> x and y are used as signed int all over the place here ... so reworking that
> goes way beyond fixing this problem. Could we please get this patch merged
> first for 11.1, and if someone then still feels like reworking the code,
> this could be done for 11.2 ?

Ok, then the < 0 checks should be kept.

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.