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

On 23/7/26 18:18, 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 ...

Ah I missed that, sorry for the troubles.

> 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
> 
>