Re: [PATCH v2] hw/display/virtio-gpu: fix offset wraparound in scanout_blob_to_fb

On 2026/07/28 5:24, Marc-André Lureau wrote:
> Hi
> 
> On Sun, Jul 26, 2026 at 11:03 AM Akihiko Odaki
> <[email protected]> wrote:
>>
>> On 2026/07/25 21:27, [email protected] wrote:
>>> From: Marc-André Lureau <[email protected]>
>>>
>>> virtio_gpu_scanout_blob_to_fb() computes the framebuffer offset from
>>> guest-controlled offsets[0], r.x, r.y and stride using uint32_t
>>> arithmetic. When the sum exceeds UINT32_MAX, silent wraparound lets
>>> the guest steer the scanout to an arbitrary in-bounds region of the
>>> blob instead of the intended rectangle.
>>>
>>> Compute the offset in uint64_t, reject values exceeding UINT32_MAX
>>> (the width of fb->offset), and only store into fb->offset once both
>>> range checks pass.
>>
>> The calculation of offset can still overflow uint64_t. All supported
>> formats use four bytes per pixel, so
>> the maximum mathematical value is:
>>
>>       UINT32_MAX + 4 * UINT32_MAX + UINT32_MAX * UINT32_MAX
>>
>> which exceeds UINT64_MAX.
> 
> The code rejects stride > INT_MAX, that should make the sum maximum
> below UINT64_MAX

You are right. Commit 9c724426f519
("hw/display/virtio-gpu: reject strides exceeding INT_MAX") has landed,
so the offset calculation can no longer overflow uint64_t:

Reviewed-by: Akihiko Odaki <[email protected]>

> 
>>
>> This is reachable through virgl_cmd_set_scanout_blob(), whose
>> rectangle checks use wrapping uint32_t additions. For example:
>>
>>       width = height = 16
>>       stride = r.y = blob_size = UINT32_MAX
>>       r.x = 0x7fffffff
>>       r.width = 0x80000001
>>       r.height = 1
>>       offsets[0] = 3
>>
>> Both rectangle sums wrap to zero, while the mathematical offset is
>> exactly 2^64. Consequently, offset becomes zero, fbend becomes
>> M, and both bounds checks pass. The non-virgl caller retroactively
>> rejects this geometry. The virgl caller fails to reject it.
> 
> That's a separate fix though, open a security issue?

I do not think a security issue is needed. The remaining wrapping 
rectangle bug can still cause QEMU to abort on invalid scanout 
dimensions. Since triggering it requires guest kernel privileges, it
should be fixed separately as a hardening bug.

Regards,
Akihiko Odaki

> 
>>
>>>
>>> Fixes: 32db3c63ae11 ("virtio-gpu: Add virtio_gpu_set_scanout_blob")
>>> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3871
>>> Based-on: <[email protected]>
>>> ("[PATCH] hw/display/virtio-gpu: Remove the bytes_pp field")
>>
>> Please remove Based-on: from the patch message when sending a pull
>> request. You can put the tag in the footer. For example:
>>
>> https://patchew.org/QEMU/[email protected]/
>>
>> Regards,
>> Akihiko Odaki
>>
>>> Reported-by: Cyber_black <[email protected]>
>>> Signed-off-by: Marc-André Lureau <[email protected]>> ---
>>> v2:
>>>    - no OOB, dropping CVE, make commit message adjustments
>>> ---
>>>    hw/display/virtio-gpu.c | 14 ++++++++------
>>>    1 file changed, 8 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
>>> index b21ac8dace08..bc45cfb194f0 100644
>>> --- a/hw/display/virtio-gpu.c
>>> +++ b/hw/display/virtio-gpu.c
>>> @@ -785,7 +785,7 @@ bool virtio_gpu_scanout_blob_to_fb(struct virtio_gpu_framebuffer *fb,
>>>                                       struct virtio_gpu_set_scanout_blob *ss,
>>>                                       uint64_t blob_size)
>>>    {
>>> -    uint64_t fbend;
>>> +    uint64_t fbend, offset;
>>>        uint32_t bytes_pp;
>>>
>>>        fb->format = virtio_gpu_get_pixman_format(ss->format);
>>> @@ -815,18 +815,20 @@ bool virtio_gpu_scanout_blob_to_fb(struct virtio_gpu_framebuffer *fb,
>>>            return false;
>>>        }
>>>
>>> -    fb->offset = ss->offsets[0] + ss->r.x * bytes_pp + ss->r.y * fb->stride;
>>> +    offset = (uint64_t)ss->offsets[0] + (uint64_t)ss->r.x * bytes_pp +
>>> +             (uint64_t)ss->r.y * fb->stride;
>>>
>>> -    fbend = fb->offset;
>>> -    fbend += (uint64_t) fb->stride * ss->r.height;
>>> +    fbend = offset + (uint64_t)fb->stride * ss->r.height;
>>>
>>> -    if (fbend > blob_size) {
>>> +    if (offset > UINT32_MAX || fbend > blob_size) {
>>>            qemu_log_mask(LOG_GUEST_ERROR,
>>> -                      "%s: fb end out of range\n",
>>> +                      "%s: invalid fb bounds\n",
>>>                          __func__);
>>>            return false;
>>>        }
>>>
>>> +    fb->offset = offset;
>>> +
>>>        return true;
>>>    }
>>>
>>
>>