Instead pass around the address (aka offset into vga memory).
Add vga_read_* helper functions which apply vbe_size_mask to
the address, to make sure the address stays within the valid
range, similar to the cirrus blitter fixes (commits ffaf857778
and 026aeffcb4).
Impact: DoS for privileged guest users. qemu crashes with
a segfault, when hitting the guard page after vga memory
allocation, while reading vga memory for display updates.
Fixes: CVE-2017-13672
Cc: P J P <ppandit@redhat.com>
Reported-by: David Buchanan <d@vidbuchanan.co.uk>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
hw/display/vga-helpers.h | 202 ++++++++++++++++++++++++++---------------------
hw/display/vga_int.h | 1 +
hw/display/vga.c | 5 +-
3 files changed, 114 insertions(+), 94 deletions(-)
diff --git a/hw/display/vga-helpers.h b/hw/display/vga-helpers.h
index 94f6de2046..5a752b3f9e 100644
--- a/hw/display/vga-helpers.h
+++ b/hw/display/vga-helpers.h
@@ -95,20 +95,46 @@ static void vga_draw_glyph9(uint8_t *d, int linesize,
} while (--h);
}
+static inline uint8_t vga_read_byte(VGACommonState *vga, uint32_t addr)
+{
+ return vga->vram_ptr[addr & vga->vbe_size_mask];
+}
+
+static inline uint16_t vga_read_word_le(VGACommonState *vga, uint32_t addr)
+{
+ uint32_t offset = addr & vga->vbe_size_mask & ~1;
+ uint16_t *ptr = (uint16_t *)(vga->vram_ptr + offset);
+ return lduw_le_p(ptr);
+}
+
+static inline uint16_t vga_read_word_be(VGACommonState *vga, uint32_t addr)
+{
+ uint32_t offset = addr & vga->vbe_size_mask & ~1;
+ uint16_t *ptr = (uint16_t *)(vga->vram_ptr + offset);
+ return lduw_be_p(ptr);
+}
+
+static inline uint32_t vga_read_dword_le(VGACommonState *vga, uint32_t addr)
+{
+ uint32_t offset = addr & vga->vbe_size_mask & ~3;
+ uint32_t *ptr = (uint32_t *)(vga->vram_ptr + offset);
+ return ldl_le_p(ptr);
+}
+
/*
* 4 color mode
*/
-static void vga_draw_line2(VGACommonState *s1, uint8_t *d,
- const uint8_t *s, int width)
+static void vga_draw_line2(VGACommonState *vga, uint8_t *d,
+ uint32_t addr, int width)
{
uint32_t plane_mask, *palette, data, v;
int x;
- palette = s1->last_palette;
- plane_mask = mask16[s1->ar[VGA_ATC_PLANE_ENABLE] & 0xf];
+ palette = vga->last_palette;
+ plane_mask = mask16[vga->ar[VGA_ATC_PLANE_ENABLE] & 0xf];
width >>= 3;
for(x = 0; x < width; x++) {
- data = ((uint32_t *)s)[0];
+ data = vga_read_dword_le(vga, addr);
data &= plane_mask;
v = expand2[GET_PLANE(data, 0)];
v |= expand2[GET_PLANE(data, 2)] << 2;
@@ -124,7 +150,7 @@ static void vga_draw_line2(VGACommonState *s1, uint8_t *d,
((uint32_t *)d)[6] = palette[(v >> 4) & 0xf];
((uint32_t *)d)[7] = palette[(v >> 0) & 0xf];
d += 32;
- s += 4;
+ addr += 4;
}
}
@@ -134,17 +160,17 @@ static void vga_draw_line2(VGACommonState *s1, uint8_t *d,
/*
* 4 color mode, dup2 horizontal
*/
-static void vga_draw_line2d2(VGACommonState *s1, uint8_t *d,
- const uint8_t *s, int width)
+static void vga_draw_line2d2(VGACommonState *vga, uint8_t *d,
+ uint32_t addr, int width)
{
uint32_t plane_mask, *palette, data, v;
int x;
- palette = s1->last_palette;
- plane_mask = mask16[s1->ar[VGA_ATC_PLANE_ENABLE] & 0xf];
+ palette = vga->last_palette;
+ plane_mask = mask16[vga->ar[VGA_ATC_PLANE_ENABLE] & 0xf];
width >>= 3;
for(x = 0; x < width; x++) {
- data = ((uint32_t *)s)[0];
+ data = vga_read_dword_le(vga, addr);
data &= plane_mask;
v = expand2[GET_PLANE(data, 0)];
v |= expand2[GET_PLANE(data, 2)] << 2;
@@ -160,24 +186,24 @@ static void vga_draw_line2d2(VGACommonState *s1, uint8_t *d,
PUT_PIXEL2(d, 6, palette[(v >> 4) & 0xf]);
PUT_PIXEL2(d, 7, palette[(v >> 0) & 0xf]);
d += 64;
- s += 4;
+ addr += 4;
}
}
/*
* 16 color mode
*/
-static void vga_draw_line4(VGACommonState *s1, uint8_t *d,
- const uint8_t *s, int width)
+static void vga_draw_line4(VGACommonState *vga, uint8_t *d,
+ uint32_t addr, int width)
{
uint32_t plane_mask, data, v, *palette;
int x;
- palette = s1->last_palette;
- plane_mask = mask16[s1->ar[VGA_ATC_PLANE_ENABLE] & 0xf];
+ palette = vga->last_palette;
+ plane_mask = mask16[vga->ar[VGA_ATC_PLANE_ENABLE] & 0xf];
width >>= 3;
for(x = 0; x < width; x++) {
- data = ((uint32_t *)s)[0];
+ data = vga_read_dword_le(vga, addr);
data &= plane_mask;
v = expand4[GET_PLANE(data, 0)];
v |= expand4[GET_PLANE(data, 1)] << 1;
@@ -192,24 +218,24 @@ static void vga_draw_line4(VGACommonState *s1, uint8_t *d,
((uint32_t *)d)[6] = palette[(v >> 4) & 0xf];
((uint32_t *)d)[7] = palette[(v >> 0) & 0xf];
d += 32;
- s += 4;
+ addr += 4;
}
}
/*
* 16 color mode, dup2 horizontal
*/
-static void vga_draw_line4d2(VGACommonState *s1, uint8_t *d,
- const uint8_t *s, int width)
+static void vga_draw_line4d2(VGACommonState *vga, uint8_t *d,
+ uint32_t addr, int width)
{
uint32_t plane_mask, data, v, *palette;
int x;
- palette = s1->last_palette;
- plane_mask = mask16[s1->ar[VGA_ATC_PLANE_ENABLE] & 0xf];
+ palette = vga->last_palette;
+ plane_mask = mask16[vga->ar[VGA_ATC_PLANE_ENABLE] & 0xf];
width >>= 3;
for(x = 0; x < width; x++) {
- data = ((uint32_t *)s)[0];
+ data = vga_read_dword_le(vga, addr);
data &= plane_mask;
v = expand4[GET_PLANE(data, 0)];
v |= expand4[GET_PLANE(data, 1)] << 1;
@@ -224,7 +250,7 @@ static void vga_draw_line4d2(VGACommonState *s1, uint8_t *d,
PUT_PIXEL2(d, 6, palette[(v >> 4) & 0xf]);
PUT_PIXEL2(d, 7, palette[(v >> 0) & 0xf]);
d += 64;
- s += 4;
+ addr += 4;
}
}
@@ -233,21 +259,21 @@ static void vga_draw_line4d2(VGACommonState *s1, uint8_t *d,
*
* XXX: add plane_mask support (never used in standard VGA modes)
*/
-static void vga_draw_line8d2(VGACommonState *s1, uint8_t *d,
- const uint8_t *s, int width)
+static void vga_draw_line8d2(VGACommonState *vga, uint8_t *d,
+ uint32_t addr, int width)
{
uint32_t *palette;
int x;
- palette = s1->last_palette;
+ palette = vga->last_palette;
width >>= 3;
for(x = 0; x < width; x++) {
- PUT_PIXEL2(d, 0, palette[s[0]]);
- PUT_PIXEL2(d, 1, palette[s[1]]);
- PUT_PIXEL2(d, 2, palette[s[2]]);
- PUT_PIXEL2(d, 3, palette[s[3]]);
+ PUT_PIXEL2(d, 0, palette[vga_read_byte(vga, addr + 0)]);
+ PUT_PIXEL2(d, 1, palette[vga_read_byte(vga, addr + 1)]);
+ PUT_PIXEL2(d, 2, palette[vga_read_byte(vga, addr + 2)]);
+ PUT_PIXEL2(d, 3, palette[vga_read_byte(vga, addr + 3)]);
d += 32;
- s += 4;
+ addr += 4;
}
}
@@ -256,63 +282,63 @@ static void vga_draw_line8d2(VGACommonState *s1, uint8_t *d,
*
* XXX: add plane_mask support (never used in standard VGA modes)
*/
-static void vga_draw_line8(VGACommonState *s1, uint8_t *d,
- const uint8_t *s, int width)
+static void vga_draw_line8(VGACommonState *vga, uint8_t *d,
+ uint32_t addr, int width)
{
uint32_t *palette;
int x;
- palette = s1->last_palette;
+ palette = vga->last_palette;
width >>= 3;
for(x = 0; x < width; x++) {
- ((uint32_t *)d)[0] = palette[s[0]];
- ((uint32_t *)d)[1] = palette[s[1]];
- ((uint32_t *)d)[2] = palette[s[2]];
- ((uint32_t *)d)[3] = palette[s[3]];
- ((uint32_t *)d)[4] = palette[s[4]];
- ((uint32_t *)d)[5] = palette[s[5]];
- ((uint32_t *)d)[6] = palette[s[6]];
- ((uint32_t *)d)[7] = palette[s[7]];
+ ((uint32_t *)d)[0] = palette[vga_read_byte(vga, addr + 0)];
+ ((uint32_t *)d)[1] = palette[vga_read_byte(vga, addr + 1)];
+ ((uint32_t *)d)[2] = palette[vga_read_byte(vga, addr + 2)];
+ ((uint32_t *)d)[3] = palette[vga_read_byte(vga, addr + 3)];
+ ((uint32_t *)d)[4] = palette[vga_read_byte(vga, addr + 4)];
+ ((uint32_t *)d)[5] = palette[vga_read_byte(vga, addr + 5)];
+ ((uint32_t *)d)[6] = palette[vga_read_byte(vga, addr + 6)];
+ ((uint32_t *)d)[7] = palette[vga_read_byte(vga, addr + 7)];
d += 32;
- s += 8;
+ addr += 8;
}
}
/*
* 15 bit color
*/
-static void vga_draw_line15_le(VGACommonState *s1, uint8_t *d,
- const uint8_t *s, int width)
+static void vga_draw_line15_le(VGACommonState *vga, uint8_t *d,
+ uint32_t addr, int width)
{
int w;
uint32_t v, r, g, b;
w = width;
do {
- v = lduw_le_p((void *)s);
+ v = vga_read_word_le(vga, addr);
r = (v >> 7) & 0xf8;
g = (v >> 2) & 0xf8;
b = (v << 3) & 0xf8;
((uint32_t *)d)[0] = rgb_to_pixel32(r, g, b);
- s += 2;
+ addr += 2;
d += 4;
} while (--w != 0);
}
-static void vga_draw_line15_be(VGACommonState *s1, uint8_t *d,
- const uint8_t *s, int width)
+static void vga_draw_line15_be(VGACommonState *vga, uint8_t *d,
+ uint32_t addr, int width)
{
int w;
uint32_t v, r, g, b;
w = width;
do {
- v = lduw_be_p((void *)s);
+ v = vga_read_word_be(vga, addr);
r = (v >> 7) & 0xf8;
g = (v >> 2) & 0xf8;
b = (v << 3) & 0xf8;
((uint32_t *)d)[0] = rgb_to_pixel32(r, g, b);
- s += 2;
+ addr += 2;
d += 4;
} while (--w != 0);
}
@@ -320,38 +346,38 @@ static void vga_draw_line15_be(VGACommonState *s1, uint8_t *d,
/*
* 16 bit color
*/
-static void vga_draw_line16_le(VGACommonState *s1, uint8_t *d,
- const uint8_t *s, int width)
+static void vga_draw_line16_le(VGACommonState *vga, uint8_t *d,
+ uint32_t addr, int width)
{
int w;
uint32_t v, r, g, b;
w = width;
do {
- v = lduw_le_p((void *)s);
+ v = vga_read_word_le(vga, addr);
r = (v >> 8) & 0xf8;
g = (v >> 3) & 0xfc;
b = (v << 3) & 0xf8;
((uint32_t *)d)[0] = rgb_to_pixel32(r, g, b);
- s += 2;
+ addr += 2;
d += 4;
} while (--w != 0);
}
-static void vga_draw_line16_be(VGACommonState *s1, uint8_t *d,
- const uint8_t *s, int width)
+static void vga_draw_line16_be(VGACommonState *vga, uint8_t *d,
+ uint32_t addr, int width)
{
int w;
uint32_t v, r, g, b;
w = width;
do {
- v = lduw_be_p((void *)s);
+ v = vga_read_word_be(vga, addr);
r = (v >> 8) & 0xf8;
g = (v >> 3) & 0xfc;
b = (v << 3) & 0xf8;
((uint32_t *)d)[0] = rgb_to_pixel32(r, g, b);
- s += 2;
+ addr += 2;
d += 4;
} while (--w != 0);
}
@@ -359,36 +385,36 @@ static void vga_draw_line16_be(VGACommonState *s1, uint8_t *d,
/*
* 24 bit color
*/
-static void vga_draw_line24_le(VGACommonState *s1, uint8_t *d,
- const uint8_t *s, int width)
+static void vga_draw_line24_le(VGACommonState *vga, uint8_t *d,
+ uint32_t addr, int width)
{
int w;
uint32_t r, g, b;
w = width;
do {
- b = s[0];
- g = s[1];
- r = s[2];
+ b = vga_read_byte(vga, addr + 0);
+ g = vga_read_byte(vga, addr + 1);
+ r = vga_read_byte(vga, addr + 2);
((uint32_t *)d)[0] = rgb_to_pixel32(r, g, b);
- s += 3;
+ addr += 3;
d += 4;
} while (--w != 0);
}
-static void vga_draw_line24_be(VGACommonState *s1, uint8_t *d,
- const uint8_t *s, int width)
+static void vga_draw_line24_be(VGACommonState *vga, uint8_t *d,
+ uint32_t addr, int width)
{
int w;
uint32_t r, g, b;
w = width;
do {
- r = s[0];
- g = s[1];
- b = s[2];
+ r = vga_read_byte(vga, addr + 0);
+ g = vga_read_byte(vga, addr + 1);
+ b = vga_read_byte(vga, addr + 2);
((uint32_t *)d)[0] = rgb_to_pixel32(r, g, b);
- s += 3;
+ addr += 3;
d += 4;
} while (--w != 0);
}
@@ -396,44 +422,36 @@ static void vga_draw_line24_be(VGACommonState *s1, uint8_t *d,
/*
* 32 bit color
*/
-static void vga_draw_line32_le(VGACommonState *s1, uint8_t *d,
- const uint8_t *s, int width)
+static void vga_draw_line32_le(VGACommonState *vga, uint8_t *d,
+ uint32_t addr, int width)
{
-#ifndef HOST_WORDS_BIGENDIAN
- memcpy(d, s, width * 4);
-#else
int w;
uint32_t r, g, b;
w = width;
do {
- b = s[0];
- g = s[1];
- r = s[2];
+ b = vga_read_byte(vga, addr + 0);
+ g = vga_read_byte(vga, addr + 1);
+ r = vga_read_byte(vga, addr + 2);
((uint32_t *)d)[0] = rgb_to_pixel32(r, g, b);
- s += 4;
+ addr += 4;
d += 4;
} while (--w != 0);
-#endif
}
-static void vga_draw_line32_be(VGACommonState *s1, uint8_t *d,
- const uint8_t *s, int width)
+static void vga_draw_line32_be(VGACommonState *vga, uint8_t *d,
+ uint32_t addr, int width)
{
-#ifdef HOST_WORDS_BIGENDIAN
- memcpy(d, s, width * 4);
-#else
int w;
uint32_t r, g, b;
w = width;
do {
- r = s[1];
- g = s[2];
- b = s[3];
+ r = vga_read_byte(vga, addr + 1);
+ g = vga_read_byte(vga, addr + 2);
+ b = vga_read_byte(vga, addr + 3);
((uint32_t *)d)[0] = rgb_to_pixel32(r, g, b);
- s += 4;
+ addr += 4;
d += 4;
} while (--w != 0);
-#endif
}
diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
index dd6c958da3..ad34a1f048 100644
--- a/hw/display/vga_int.h
+++ b/hw/display/vga_int.h
@@ -94,6 +94,7 @@ typedef struct VGACommonState {
uint32_t vram_size;
uint32_t vram_size_mb; /* property */
uint32_t vbe_size;
+ uint32_t vbe_size_mask;
uint32_t latch;
bool has_chain4_alias;
MemoryRegion chain4_alias;
diff --git a/hw/display/vga.c b/hw/display/vga.c
index 63421f9ee8..4c2ee0be28 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -1005,7 +1005,7 @@ void vga_mem_writeb(VGACommonState *s, hwaddr addr, uint32_t val)
}
typedef void vga_draw_line_func(VGACommonState *s1, uint8_t *d,
- const uint8_t *s, int width);
+ uint32_t srcaddr, int width);
#include "vga-helpers.h"
@@ -1660,7 +1660,7 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
if (y_start < 0)
y_start = y;
if (!(is_buffer_shared(surface))) {
- vga_draw_line(s, d, s->vram_ptr + addr, width);
+ vga_draw_line(s, d, addr, width);
if (s->cursor_draw_line)
s->cursor_draw_line(s, d, y);
}
@@ -2164,6 +2164,7 @@ void vga_common_init(VGACommonState *s, Object *obj, bool global_vmstate)
if (!s->vbe_size) {
s->vbe_size = s->vram_size;
}
+ s->vbe_size_mask = s->vbe_size - 1;
s->is_vbe_vmstate = 1;
memory_region_init_ram_nomigrate(&s->vram, obj, "vga.vram", s->vram_size,
--
2.9.3
I might be mistaken, but I don't think this patch actually fixes CVE-2017-13672. I tested the latest git repo (last commit 530049bc1d) against my initial reproducer, and QEMU still segfaults. I think this is because the actual OOB read occurs inside pixman, which of course is not affected by this patch. Perhaps bounds checks need to be applied to the arguments passed into pixman?
On Mon, 2017-10-09 at 12:55 +0100, David Buchanan wrote: > I might be mistaken, but I don't think this patch actually fixes > CVE-2017-13672. I tested the latest git repo (last commit 530049bc1d) > against my initial reproducer, and QEMU still segfaults. Hmm, no segfault here. Tried gtk, sdl, vnc, spice. How do you start qemu? Which user interface? > I think this is because the actual OOB read occurs inside pixman, > which > of course is not affected by this patch. Perhaps bounds checks need > to > be applied to the arguments passed into pixman? Hmm, 24bpp modes are typically not handled by pixman (at least not in a way that qemu creates a pixman image backed by vga memory). Have you seen a stacktrace with pixman in there? Care to share it? thanks, Gerd
On 09/10/17 13:56, Gerd Hoffmann wrote:
> How do you start
> qemu? Which user interface?
Like this:
qemu-system-x86_64 -vga cirrus [disk image]
(which I assume is using the GTK interface)
I have attached the reproducer NASM source and disk image.
Note that the reproducer is using VBE.
> Have you seen a stacktrace with pixman in there? Care to share it?
#0 0x00007fffe1c2bf61 in () at /usr/lib/libpixman-1.so.0
#1 0x00007fffe1c385db in () at /usr/lib/libpixman-1.so.0
#2 0x00007fffe1c38991 in () at /usr/lib/libpixman-1.so.0
#3 0x00007fffe1c6eb7c in () at /usr/lib/libpixman-1.so.0
#4 0x00007fffe1c2aca1 in pixman_image_composite32 () at
/usr/lib/libpixman-1.so.0
#5 0x0000555555be5630 in gd_switch (dcl=0x555557e088b0,
surface=0x555556d76ac0) at /tmp/qemu/ui/gtk.c:628
#6 0x0000555555bb216a in dpy_gfx_replace_surface (con=0x5555569718d0,
surface=0x555556d76ac0)
at /tmp/qemu/ui/console.c:1552
#7 0x000055555580aab6 in vga_draw_graphic (s=0x5555576de2c0,
full_update=0x1) at /tmp/qemu/hw/display/vga.c:1561
#8 0x000055555580b388 in vga_update_display (opaque=0x5555576de2c0) at
/tmp/qemu/hw/display/vga.c:1756
#9 0x0000555555bae64d in graphic_hw_update (con=0x5555569718d0) at
/tmp/qemu/ui/console.c:263
#10 0x0000555555be50f8 in gd_refresh (dcl=0x555557e088b0) at
/tmp/qemu/ui/gtk.c:493
#11 0x0000555555bb22a8 in dpy_refresh (s=0x5555577b05c0) at
/tmp/qemu/ui/console.c:1589
#12 0x0000555555bae362 in gui_update (opaque=0x5555577b05c0) at
/tmp/qemu/ui/console.c:201
#13 0x0000555555d20445 in timerlist_run_timers
(timer_list=0x555556928210) at /tmp/qemu/util/qemu-timer.c:536
#14 0x0000555555d204a2 in qemu_clock_run_timers
(type=QEMU_CLOCK_REALTIME) at /tmp/qemu/util/qemu-timer.c:547
#15 0x0000555555d208fe in qemu_clock_run_all_timers () at
/tmp/qemu/util/qemu-timer.c:662
#16 0x0000555555d21125 in main_loop_wait (nonblocking=0x0) at
/tmp/qemu/util/main-loop.c:521
#17 0x0000555555926553 in main_loop () at /tmp/qemu/vl.c:1995
#18 0x000055555592e793 in main (argc=0x4, argv=0x7fffffffe218,
envp=0x7fffffffe240) at /tmp/qemu/vl.c:4902
#19 0x00007fffdc663f6a in __libc_start_main () at /usr/lib/libc.so.6
#20 0x000055555577394a in _start ()
Thanks.
© 2016 - 2024 Red Hat, Inc.