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

marcandre.lureau@redhat.com posted 1 patch 21 hours ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20260725122734.1775774-1-marcandre.lureau@redhat.com
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, "Alex Bennée" <alex.bennee@linaro.org>, Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>, Dmitry Osipenko <dmitry.osipenko@collabora.com>
hw/display/virtio-gpu.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
[PATCH v2] hw/display/virtio-gpu: fix offset wraparound in scanout_blob_to_fb
Posted by marcandre.lureau@redhat.com 21 hours ago
From: Marc-André Lureau <marcandre.lureau@redhat.com>

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.

Fixes: 32db3c63ae11 ("virtio-gpu: Add virtio_gpu_set_scanout_blob")
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3871
Based-on: <20260719-bpp-v1-1-9b91946d6cf3@rsg.ci.i.u-tokyo.ac.jp>
("[PATCH] hw/display/virtio-gpu: Remove the bytes_pp field")
Reported-by: Cyber_black <Cyberblackk@proton.me>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
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;
 }
 
-- 
2.55.0


Re: [PATCH v2] hw/display/virtio-gpu: fix offset wraparound in scanout_blob_to_fb
Posted by Akihiko Odaki 2 hours ago
On 2026/07/25 21:27, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> 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.

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.

> 
> Fixes: 32db3c63ae11 ("virtio-gpu: Add virtio_gpu_set_scanout_blob")
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3871
> Based-on: <20260719-bpp-v1-1-9b91946d6cf3@rsg.ci.i.u-tokyo.ac.jp>
> ("[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/20260719-bpp-v1-1-9b91946d6cf3@rsg.ci.i.u-tokyo.ac.jp/

Regards,
Akihiko Odaki

> Reported-by: Cyber_black <Cyberblackk@proton.me>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>> ---
> 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;
>   }
>