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

On 23/7/26 14:35, Thomas Huth wrote:
> On 23/07/2026 14.27, Philippe Mathieu-Daudé wrote:
>> Hi Thomas,
>>
>> On 23/7/26 14:00, 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]>
>>> ---
>>>   hw/display/vmware_vga.c | 5 ++++-
>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
>>> index f6f9edfd1d9..04cc438cc02 100644
>>> --- a/hw/display/vmware_vga.c
>>> +++ b/hw/display/vmware_vga.c
>>> @@ -737,6 +737,9 @@ 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 >= 32768 || y < 0 || y >= 32768) {
>>
>> Maybe add a self-decribing definition instead of magic number?
>>
>>    #define ALPHA_CURSOR_SAFE_LIMIT 32768 /* Arbitrary limit */
>>
>> Otherwise LGTM.
> 
> There is already this in the code:
> 
> #define SVGA_MAX_WIDTH                  2368
> #define SVGA_MAX_HEIGHT                 1770
> 
> Shall I use those? WDYT?

Ah, these look like good candidates :)

> 
>   Thomas
> 
>>> +                goto rewind;
>>> +            }
>>>               args = x * y;
>>>               goto badcmd;
>>>           case SVGA_CMD_RECT_ROP_FILL:
>>> @@ -776,7 +779,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",
>>
> 
>