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

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 <[email protected]>
> > 
> > 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 <[email protected]>
> > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4026
> > Reported-by: Tristan Madani <[email protected]>
> > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4076
> > Reported-by: Sunday Jiang
> > Signed-off-by: Thomas Huth <[email protected]>
> > ---
> >   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é <[email protected]>
> 
> > +                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 :|