On Thu, 29 Oct 2026, Tristan Madani wrote:
From: Tristan Madani <[email protected]>
The VRAM bounds check in ati_2d_do_blt() computes
(dst.y + dst.height) * dst_stride using signed 32-bit arithmetic.
On Rage128, setup_2d_blt_ctx() multiplies dst_stride by bpp (up to
32), producing strides up to 524,256 bytes/row. With large
dst.y + dst.height values, the signed int multiplication overflows
to a negative value, causing the bounds check to evaluate false
when it should reject the operation.
The subsequent blit loop uses unsigned arithmetic for the same index
computation, producing large positive offsets that write past the
VRAM allocation into host process memory.
The same overflow affects the source bounds check in the SRCCOPY
path, where (vis_src.y + vis_dst.height) * src_stride can also
overflow when the guest sets large pitch values.
Fix both by pre-computing the end offsets as uint64_t, which
prevents the signed overflow while keeping the existing bounds
check logic.
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/4646
Cc: [email protected]
Signed-off-by: Tristan Madani <[email protected]>
---
v2:
- Actually use src_end for the source bounds check in the SRCCOPY path;
v1 declared it but never used it (Marc-André Lureau)
- Sent standalone; v1 was part of an 8-patch display series
hw/display/ati_2d.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
index 7a4a115..cfa4947 100644
--- a/hw/display/ati_2d.c
+++ b/hw/display/ati_2d.c
@@ -157,6 +157,7 @@ static bool ati_2d_do_blt(const ATI2DCtx *ctx, uint8_t
use_pixman)
QemuRect vis_src, vis_dst;
unsigned int x, y, i, j, bypp = ctx->bpp / 8;
const uint8_t *vram_end = ctx->vga->vram_ptr + ctx->vga->vram_size;
+ uint64_t dst_end;
if (!ctx->bpp) {
qemu_log_mask(LOG_GUEST_ERROR, "Invalid bpp\n");
@@ -166,9 +167,11 @@ static bool ati_2d_do_blt(const ATI2DCtx *ctx, uint8_t
use_pixman)
qemu_log_mask(LOG_GUEST_ERROR, "Zero dest pitch\n");
return false;
}
+ dst_end = (uint64_t)ctx->dst.x * bypp +
+ (uint64_t)(ctx->dst.y + ctx->dst.height) * ctx->dst_stride;
if (ctx->dst.x > 0x3fff || ctx->dst.y > 0x3fff ||
- ctx->dst_bits >= vram_end - bypp || ctx->dst_bits + ctx->dst.x * bypp +
- (ctx->dst.y + ctx->dst.height) * ctx->dst_stride >= vram_end - bypp) {
+ ctx->dst_bits >= vram_end - bypp ||
+ ctx->dst_bits + dst_end >= vram_end - bypp) {
qemu_log_mask(LOG_UNIMP, "blt outside vram not implemented\n");
return false;
}
@@ -203,11 +206,12 @@ static bool ati_2d_do_blt(const ATI2DCtx *ctx, uint8_t
use_pixman)
qemu_log_mask(LOG_GUEST_ERROR, "Zero source pitch\n");
return false;
}
+ uint64_t src_end = (uint64_t)vis_src.x * bypp +
QEMU coding style says this has to be defined at the beginning of the
function not here. Also I think it would be better to calculate the whole
pointer as a const uint8_t * including {src,dst}_bits to match vram_end
instead of using an uint64_t offset.
Regards,
BALATON Zoltan
+ (uint64_t)(vis_src.y + vis_dst.height) * ctx->src_stride;
if (!ctx->host_data_active &&
(vis_src.x > 0x3fff || vis_src.y > 0x3fff ||
ctx->src_bits >= vram_end - bypp ||
- ctx->src_bits + vis_src.x * bypp + (vis_src.y + vis_dst.height) *
- ctx->src_stride >= vram_end - bypp)) {
+ ctx->src_bits + src_end >= vram_end - bypp)) {
qemu_log_mask(LOG_UNIMP, "blt outside vram not implemented\n");
return false;
}
