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