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

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 <[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.
> 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 :|