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

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 ?

  Thomas