* [Qemu-devel] [PULL for-2.9 0/7] cirrus: more blitter security fixes.
@ 2017-03-16 9:30 Gerd Hoffmann
2017-03-16 9:30 ` [Qemu-devel] [PULL for-2.9 1/7] fix :cirrus_vga fix OOB read case qemu Segmentation fault Gerd Hoffmann
` (7 more replies)
0 siblings, 8 replies; 12+ messages in thread
From: Gerd Hoffmann @ 2017-03-16 9:30 UTC (permalink / raw)
To: qemu-devel; +Cc: Gerd Hoffmann
Hi,
Another pile of cirrus blitter fixes, including cve fixes for known
issues, so clearly 2.9 material.
Patches 6+7 implement a new approach to blitter memory access sanity
checking. We pass around offsets not pointers, and at the place where
the actual memory access happens we mask the offset to the valid
range before calculating the pointer.
That should put an end to security holes due to blit_is_unsafe() sanity
checks failing to calculate some special case correctly, or due to
blit_is_unsafe() calls missing, and kill any dragons which might still
be lurking in the code. In theory this even obsoletes blit_is_unsafe(),
but I don't feel like ripping it out right away ...
please pull,
Gerd
The following changes since commit 1883ff34b540daacae948f493b0ba525edf5f642:
Merge remote-tracking branch 'remotes/mst/tags/for_upstream' into staging (2017-03-15 18:44:05 +0000)
are available in the git repository at:
git://git.kraxel.org/qemu tags/pull-cirrus-20170316-1
for you to fetch changes up to ffaf857778286ca54e3804432a2369a279e73aa7:
cirrus: stop passing around src pointers in the blitter (2017-03-16 08:58:16 +0100)
----------------------------------------------------------------
cirrus: blitter fixes.
----------------------------------------------------------------
Gerd Hoffmann (6):
cirrus/vnc: zap bitblit support from console code.
cirrus: switch to 4 MB video memory by default
cirrus: add option to disable blitter
cirrus: fix cirrus_invalidate_region
cirrus: stop passing around dst pointers in the blitter
cirrus: stop passing around src pointers in the blitter
hangaohuai (1):
fix :cirrus_vga fix OOB read case qemu Segmentation fault
hw/display/cirrus_vga.c | 106 ++++++++++++++++--------
hw/display/cirrus_vga_rop.h | 191 ++++++++++++++++++++++++++-----------------
hw/display/cirrus_vga_rop2.h | 125 ++++++++++++++--------------
include/hw/compat.h | 8 ++
include/ui/console.h | 7 --
ui/console.c | 28 -------
ui/vnc.c | 100 ----------------------
7 files changed, 259 insertions(+), 306 deletions(-)
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Qemu-devel] [PULL for-2.9 1/7] fix :cirrus_vga fix OOB read case qemu Segmentation fault
2017-03-16 9:30 [Qemu-devel] [PULL for-2.9 0/7] cirrus: more blitter security fixes Gerd Hoffmann
@ 2017-03-16 9:30 ` Gerd Hoffmann
2017-03-16 9:30 ` [Qemu-devel] [PULL for-2.9 2/7] cirrus/vnc: zap bitblit support from console code Gerd Hoffmann
` (6 subsequent siblings)
7 siblings, 0 replies; 12+ messages in thread
From: Gerd Hoffmann @ 2017-03-16 9:30 UTC (permalink / raw)
To: qemu-devel; +Cc: hangaohuai, fangying, Gerd Hoffmann
From: hangaohuai <hangaohuai@huawei.com>
check the validity of parameters in cirrus_bitblt_rop_fwd_transp_xxx
and cirrus_bitblt_rop_fwd_xxx to avoid the OOB read which causes qemu Segmentation fault.
After the fix, we will touch the assert in
cirrus_invalidate_region:
assert(off_cur_end >= off_cur);
Signed-off-by: fangying <fangying1@huawei.com>
Signed-off-by: hangaohuai <hangaohuai@huawei.com>
Message-id: 20170314063919.16200-1-hangaohuai@huawei.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
hw/display/cirrus_vga_rop.h | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/hw/display/cirrus_vga_rop.h b/hw/display/cirrus_vga_rop.h
index 0925a00..b7447f8 100644
--- a/hw/display/cirrus_vga_rop.h
+++ b/hw/display/cirrus_vga_rop.h
@@ -97,6 +97,11 @@ glue(glue(cirrus_bitblt_rop_fwd_transp_, ROP_NAME),_8)(CirrusVGAState *s,
uint8_t p;
dstpitch -= bltwidth;
srcpitch -= bltwidth;
+
+ if (bltheight > 1 && (dstpitch < 0 || srcpitch < 0)) {
+ return;
+ }
+
for (y = 0; y < bltheight; y++) {
for (x = 0; x < bltwidth; x++) {
p = *dst;
@@ -143,6 +148,11 @@ glue(glue(cirrus_bitblt_rop_fwd_transp_, ROP_NAME),_16)(CirrusVGAState *s,
uint8_t p1, p2;
dstpitch -= bltwidth;
srcpitch -= bltwidth;
+
+ if (bltheight > 1 && (dstpitch < 0 || srcpitch < 0)) {
+ return;
+ }
+
for (y = 0; y < bltheight; y++) {
for (x = 0; x < bltwidth; x+=2) {
p1 = *dst;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Qemu-devel] [PULL for-2.9 2/7] cirrus/vnc: zap bitblit support from console code.
2017-03-16 9:30 [Qemu-devel] [PULL for-2.9 0/7] cirrus: more blitter security fixes Gerd Hoffmann
2017-03-16 9:30 ` [Qemu-devel] [PULL for-2.9 1/7] fix :cirrus_vga fix OOB read case qemu Segmentation fault Gerd Hoffmann
@ 2017-03-16 9:30 ` Gerd Hoffmann
2017-03-16 9:30 ` [Qemu-devel] [PULL for-2.9 3/7] cirrus: switch to 4 MB video memory by default Gerd Hoffmann
` (5 subsequent siblings)
7 siblings, 0 replies; 12+ messages in thread
From: Gerd Hoffmann @ 2017-03-16 9:30 UTC (permalink / raw)
To: qemu-devel; +Cc: Gerd Hoffmann
There is a special code path (dpy_gfx_copy) to allow graphic emulation
notify user interface code about bitblit operations carryed out by
guests. It is supported by cirrus and vnc server. The intended purpose
is to optimize display scrolls and just send over the scroll op instead
of a full display update.
This is rarely used these days though because modern guests simply don't
use the cirrus blitter any more. Any linux guest using the cirrus drm
driver doesn't. Any windows guest newer than winxp doesn't ship with a
cirrus driver any more and thus uses the cirrus as simple framebuffer.
So this code tends to bitrot and bugs can go unnoticed for a long time.
See for example commit "3e10c3e vnc: fix qemu crash because of SIGSEGV"
which fixes a bug lingering in the code for almost a year, added by
commit "c7628bf vnc: only alloc server surface with clients connected".
Also the vnc server will throttle the frame rate in case it figures the
network can't keep up (send buffers are full). This doesn't work with
dpy_gfx_copy, for any copy operation sent to the vnc client we have to
send all outstanding updates beforehand, otherwise the vnc client might
run the client side blit on outdated data and thereby corrupt the
display. So this dpy_gfx_copy "optimization" might even make things
worse on slow network links.
Lets kill it once for all.
Oh, and one more reason: Turns out (after writing the patch) we have a
security bug in that code path ...
Fixes: CVE-2016-9603
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-id: 1489494419-14340-1-git-send-email-kraxel@redhat.com
---
hw/display/cirrus_vga.c | 12 ++----
include/ui/console.h | 7 ----
ui/console.c | 28 --------------
ui/vnc.c | 100 ------------------------------------------------
4 files changed, 3 insertions(+), 144 deletions(-)
diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index b9e7cb1..c90a4a3 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -796,21 +796,15 @@ static int cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h)
}
}
- /* we have to flush all pending changes so that the copy
- is generated at the appropriate moment in time */
- if (notify)
- graphic_hw_update(s->vga.con);
-
(*s->cirrus_rop) (s, s->vga.vram_ptr + s->cirrus_blt_dstaddr,
s->vga.vram_ptr + s->cirrus_blt_srcaddr,
s->cirrus_blt_dstpitch, s->cirrus_blt_srcpitch,
s->cirrus_blt_width, s->cirrus_blt_height);
if (notify) {
- qemu_console_copy(s->vga.con,
- sx, sy, dx, dy,
- s->cirrus_blt_width / depth,
- s->cirrus_blt_height);
+ dpy_gfx_update(s->vga.con, dx, dy,
+ s->cirrus_blt_width / depth,
+ s->cirrus_blt_height);
}
/* we don't have to notify the display that this portion has
diff --git a/include/ui/console.h b/include/ui/console.h
index ac2895c..d759338 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -189,9 +189,6 @@ typedef struct DisplayChangeListenerOps {
int x, int y, int w, int h);
void (*dpy_gfx_switch)(DisplayChangeListener *dcl,
struct DisplaySurface *new_surface);
- void (*dpy_gfx_copy)(DisplayChangeListener *dcl,
- int src_x, int src_y,
- int dst_x, int dst_y, int w, int h);
bool (*dpy_gfx_check_format)(DisplayChangeListener *dcl,
pixman_format_code_t format);
@@ -277,8 +274,6 @@ int dpy_set_ui_info(QemuConsole *con, QemuUIInfo *info);
void dpy_gfx_update(QemuConsole *con, int x, int y, int w, int h);
void dpy_gfx_replace_surface(QemuConsole *con,
DisplaySurface *surface);
-void dpy_gfx_copy(QemuConsole *con, int src_x, int src_y,
- int dst_x, int dst_y, int w, int h);
void dpy_text_cursor(QemuConsole *con, int x, int y);
void dpy_text_update(QemuConsole *con, int x, int y, int w, int h);
void dpy_text_resize(QemuConsole *con, int w, int h);
@@ -411,8 +406,6 @@ void qemu_console_set_window_id(QemuConsole *con, int window_id);
void console_select(unsigned int index);
void qemu_console_resize(QemuConsole *con, int width, int height);
-void qemu_console_copy(QemuConsole *con, int src_x, int src_y,
- int dst_x, int dst_y, int w, int h);
DisplaySurface *qemu_console_surface(QemuConsole *con);
/* console-gl.c */
diff --git a/ui/console.c b/ui/console.c
index d1ff750..4c70d8b 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1586,27 +1586,6 @@ static void dpy_refresh(DisplayState *s)
}
}
-void dpy_gfx_copy(QemuConsole *con, int src_x, int src_y,
- int dst_x, int dst_y, int w, int h)
-{
- DisplayState *s = con->ds;
- DisplayChangeListener *dcl;
-
- if (!qemu_console_is_visible(con)) {
- return;
- }
- QLIST_FOREACH(dcl, &s->listeners, next) {
- if (con != (dcl->con ? dcl->con : active_console)) {
- continue;
- }
- if (dcl->ops->dpy_gfx_copy) {
- dcl->ops->dpy_gfx_copy(dcl, src_x, src_y, dst_x, dst_y, w, h);
- } else { /* TODO */
- dcl->ops->dpy_gfx_update(dcl, dst_x, dst_y, w, h);
- }
- }
-}
-
void dpy_text_cursor(QemuConsole *con, int x, int y)
{
DisplayState *s = con->ds;
@@ -2138,13 +2117,6 @@ void qemu_console_resize(QemuConsole *s, int width, int height)
dpy_gfx_replace_surface(s, surface);
}
-void qemu_console_copy(QemuConsole *con, int src_x, int src_y,
- int dst_x, int dst_y, int w, int h)
-{
- assert(con->console_type == GRAPHIC_CONSOLE);
- dpy_gfx_copy(con, src_x, src_y, dst_x, dst_y, w, h);
-}
-
DisplaySurface *qemu_console_surface(QemuConsole *console)
{
return console->surface;
diff --git a/ui/vnc.c b/ui/vnc.c
index 51f4b30..8bfb1e0 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -894,105 +894,6 @@ int vnc_send_framebuffer_update(VncState *vs, int x, int y, int w, int h)
return n;
}
-static void vnc_copy(VncState *vs, int src_x, int src_y, int dst_x, int dst_y, int w, int h)
-{
- /* send bitblit op to the vnc client */
- vnc_lock_output(vs);
- vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
- vnc_write_u8(vs, 0);
- vnc_write_u16(vs, 1); /* number of rects */
- vnc_framebuffer_update(vs, dst_x, dst_y, w, h, VNC_ENCODING_COPYRECT);
- vnc_write_u16(vs, src_x);
- vnc_write_u16(vs, src_y);
- vnc_unlock_output(vs);
- vnc_flush(vs);
-}
-
-static void vnc_dpy_copy(DisplayChangeListener *dcl,
- int src_x, int src_y,
- int dst_x, int dst_y, int w, int h)
-{
- VncDisplay *vd = container_of(dcl, VncDisplay, dcl);
- VncState *vs, *vn;
- uint8_t *src_row;
- uint8_t *dst_row;
- int i, x, y, pitch, inc, w_lim, s;
- int cmp_bytes;
-
- if (!vd->server) {
- /* no client connected */
- return;
- }
-
- vnc_refresh_server_surface(vd);
- QTAILQ_FOREACH_SAFE(vs, &vd->clients, next, vn) {
- if (vnc_has_feature(vs, VNC_FEATURE_COPYRECT)) {
- vs->force_update = 1;
- vnc_update_client(vs, 1, true);
- /* vs might be free()ed here */
- }
- }
-
- if (!vd->server) {
- /* no client connected */
- return;
- }
- /* do bitblit op on the local surface too */
- pitch = vnc_server_fb_stride(vd);
- src_row = vnc_server_fb_ptr(vd, src_x, src_y);
- dst_row = vnc_server_fb_ptr(vd, dst_x, dst_y);
- y = dst_y;
- inc = 1;
- if (dst_y > src_y) {
- /* copy backwards */
- src_row += pitch * (h-1);
- dst_row += pitch * (h-1);
- pitch = -pitch;
- y = dst_y + h - 1;
- inc = -1;
- }
- w_lim = w - (VNC_DIRTY_PIXELS_PER_BIT - (dst_x % VNC_DIRTY_PIXELS_PER_BIT));
- if (w_lim < 0) {
- w_lim = w;
- } else {
- w_lim = w - (w_lim % VNC_DIRTY_PIXELS_PER_BIT);
- }
- for (i = 0; i < h; i++) {
- for (x = 0; x <= w_lim;
- x += s, src_row += cmp_bytes, dst_row += cmp_bytes) {
- if (x == w_lim) {
- if ((s = w - w_lim) == 0)
- break;
- } else if (!x) {
- s = (VNC_DIRTY_PIXELS_PER_BIT -
- (dst_x % VNC_DIRTY_PIXELS_PER_BIT));
- s = MIN(s, w_lim);
- } else {
- s = VNC_DIRTY_PIXELS_PER_BIT;
- }
- cmp_bytes = s * VNC_SERVER_FB_BYTES;
- if (memcmp(src_row, dst_row, cmp_bytes) == 0)
- continue;
- memmove(dst_row, src_row, cmp_bytes);
- QTAILQ_FOREACH(vs, &vd->clients, next) {
- if (!vnc_has_feature(vs, VNC_FEATURE_COPYRECT)) {
- set_bit(((x + dst_x) / VNC_DIRTY_PIXELS_PER_BIT),
- vs->dirty[y]);
- }
- }
- }
- src_row += pitch - w * VNC_SERVER_FB_BYTES;
- dst_row += pitch - w * VNC_SERVER_FB_BYTES;
- y += inc;
- }
-
- QTAILQ_FOREACH(vs, &vd->clients, next) {
- if (vnc_has_feature(vs, VNC_FEATURE_COPYRECT)) {
- vnc_copy(vs, src_x, src_y, dst_x, dst_y, w, h);
- }
- }
-}
-
static void vnc_mouse_set(DisplayChangeListener *dcl,
int x, int y, int visible)
{
@@ -3120,7 +3021,6 @@ static gboolean vnc_listen_io(QIOChannel *ioc,
static const DisplayChangeListenerOps dcl_ops = {
.dpy_name = "vnc",
.dpy_refresh = vnc_refresh,
- .dpy_gfx_copy = vnc_dpy_copy,
.dpy_gfx_update = vnc_dpy_update,
.dpy_gfx_switch = vnc_dpy_switch,
.dpy_gfx_check_format = qemu_pixman_check_format,
--
1.8.3.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Qemu-devel] [PULL for-2.9 3/7] cirrus: switch to 4 MB video memory by default
2017-03-16 9:30 [Qemu-devel] [PULL for-2.9 0/7] cirrus: more blitter security fixes Gerd Hoffmann
2017-03-16 9:30 ` [Qemu-devel] [PULL for-2.9 1/7] fix :cirrus_vga fix OOB read case qemu Segmentation fault Gerd Hoffmann
2017-03-16 9:30 ` [Qemu-devel] [PULL for-2.9 2/7] cirrus/vnc: zap bitblit support from console code Gerd Hoffmann
@ 2017-03-16 9:30 ` Gerd Hoffmann
2017-03-16 9:30 ` [Qemu-devel] [PULL for-2.9 4/7] cirrus: add option to disable blitter Gerd Hoffmann
` (4 subsequent siblings)
7 siblings, 0 replies; 12+ messages in thread
From: Gerd Hoffmann @ 2017-03-16 9:30 UTC (permalink / raw)
To: qemu-devel; +Cc: Gerd Hoffmann
Quoting cirrus source code:
Follow real hardware, cirrus card emulated has 4 MB video memory.
Also accept 8 MB/16 MB for backward compatibility.
So just use 4MB by default. We decided to leave that at 8MB by default
a while ago, for live migration compatibility reasons. But we have
compat properties to handle that, so that isn't a compeling reason.
This also removes some sanity check inconsistencies in the cirrus code.
Some places check against the allocated video memory, some places check
against the 4MB physical hardware has. Guest code can trigger asserts
because of that.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-id: 1489494514-15606-1-git-send-email-kraxel@redhat.com
---
hw/display/cirrus_vga.c | 4 ++--
include/hw/compat.h | 8 ++++++++
2 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index c90a4a3..6ffe64f 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -3023,7 +3023,7 @@ static void isa_cirrus_vga_realizefn(DeviceState *dev, Error **errp)
static Property isa_cirrus_vga_properties[] = {
DEFINE_PROP_UINT32("vgamem_mb", struct ISACirrusVGAState,
- cirrus_vga.vga.vram_size_mb, 8),
+ cirrus_vga.vga.vram_size_mb, 4),
DEFINE_PROP_END_OF_LIST(),
};
@@ -3092,7 +3092,7 @@ static void pci_cirrus_vga_realize(PCIDevice *dev, Error **errp)
static Property pci_vga_cirrus_properties[] = {
DEFINE_PROP_UINT32("vgamem_mb", struct PCICirrusVGAState,
- cirrus_vga.vga.vram_size_mb, 8),
+ cirrus_vga.vga.vram_size_mb, 4),
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/include/hw/compat.h b/include/hw/compat.h
index b7db438..b7e0e9f 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -18,6 +18,14 @@
.driver = "pci-bridge",\
.property = "shpc",\
.value = "on",\
+ },{\
+ .driver = "cirrus-vga",\
+ .property = "vgamem_mb",\
+ .value = "8",\
+ },{\
+ .driver = "isa-cirrus-vga",\
+ .property = "vgamem_mb",\
+ .value = "8",\
},
#define HW_COMPAT_2_7 \
--
1.8.3.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Qemu-devel] [PULL for-2.9 4/7] cirrus: add option to disable blitter
2017-03-16 9:30 [Qemu-devel] [PULL for-2.9 0/7] cirrus: more blitter security fixes Gerd Hoffmann
` (2 preceding siblings ...)
2017-03-16 9:30 ` [Qemu-devel] [PULL for-2.9 3/7] cirrus: switch to 4 MB video memory by default Gerd Hoffmann
@ 2017-03-16 9:30 ` Gerd Hoffmann
2017-03-16 9:51 ` 李强
2017-03-16 9:30 ` [Qemu-devel] [PULL for-2.9 5/7] cirrus: fix cirrus_invalidate_region Gerd Hoffmann
` (3 subsequent siblings)
7 siblings, 1 reply; 12+ messages in thread
From: Gerd Hoffmann @ 2017-03-16 9:30 UTC (permalink / raw)
To: qemu-devel; +Cc: Gerd Hoffmann
Ok, we have this beast in the cirrus code which is not used at all by
modern guests, except when you try to find security holes in qemu. So,
add an option to disable blitter altogether. Guests released within
the last ten years should not show any rendering issues if you turn off
blitter support.
There are no known bugs in the cirrus blitter code. But in the past we
hoped a few times already that we've finally nailed the last issue. So
having some easy way to mitigate in case yet another blitter issue shows
up certainly makes me sleep a bit better at night.
For completeness: The by far better way to mitigate is to switch away
from cirrus and use stdvga instead. Or something more modern like
virtio-vga in case your guest has support for it.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-id: 1489494540-15745-1-git-send-email-kraxel@redhat.com
---
hw/display/cirrus_vga.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index 6ffe64f..326d511 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -205,6 +205,7 @@ typedef struct CirrusVGAState {
uint32_t cirrus_bank_base[2];
uint32_t cirrus_bank_limit[2];
uint8_t cirrus_hidden_palette[48];
+ bool enable_blitter;
int cirrus_blt_pixelwidth;
int cirrus_blt_width;
int cirrus_blt_height;
@@ -960,6 +961,10 @@ static void cirrus_bitblt_start(CirrusVGAState * s)
{
uint8_t blt_rop;
+ if (!s->enable_blitter) {
+ goto bitblt_ignore;
+ }
+
s->vga.gr[0x31] |= CIRRUS_BLT_BUSY;
s->cirrus_blt_width = (s->vga.gr[0x20] | (s->vga.gr[0x21] << 8)) + 1;
@@ -3024,6 +3029,8 @@ static void isa_cirrus_vga_realizefn(DeviceState *dev, Error **errp)
static Property isa_cirrus_vga_properties[] = {
DEFINE_PROP_UINT32("vgamem_mb", struct ISACirrusVGAState,
cirrus_vga.vga.vram_size_mb, 4),
+ DEFINE_PROP_BOOL("blitter", struct ISACirrusVGAState,
+ cirrus_vga.enable_blitter, true),
DEFINE_PROP_END_OF_LIST(),
};
@@ -3093,6 +3100,8 @@ static void pci_cirrus_vga_realize(PCIDevice *dev, Error **errp)
static Property pci_vga_cirrus_properties[] = {
DEFINE_PROP_UINT32("vgamem_mb", struct PCICirrusVGAState,
cirrus_vga.vga.vram_size_mb, 4),
+ DEFINE_PROP_BOOL("blitter", struct PCICirrusVGAState,
+ cirrus_vga.enable_blitter, true),
DEFINE_PROP_END_OF_LIST(),
};
--
1.8.3.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Qemu-devel] [PULL for-2.9 5/7] cirrus: fix cirrus_invalidate_region
2017-03-16 9:30 [Qemu-devel] [PULL for-2.9 0/7] cirrus: more blitter security fixes Gerd Hoffmann
` (3 preceding siblings ...)
2017-03-16 9:30 ` [Qemu-devel] [PULL for-2.9 4/7] cirrus: add option to disable blitter Gerd Hoffmann
@ 2017-03-16 9:30 ` Gerd Hoffmann
2017-03-16 9:30 ` [Qemu-devel] [PULL for-2.9 6/7] cirrus: stop passing around dst pointers in the blitter Gerd Hoffmann
` (2 subsequent siblings)
7 siblings, 0 replies; 12+ messages in thread
From: Gerd Hoffmann @ 2017-03-16 9:30 UTC (permalink / raw)
To: qemu-devel; +Cc: Gerd Hoffmann
off_cur_end is exclusive, so off_cur_end == cirrus_addr_mask is valid.
Fix calculation to make sure to allow that, otherwise the assert added
by commit f153b563f8cf121aebf5a2fff5f0110faf58ccb3 can trigger for valid
blits.
Test case: boot windows nt 4.0
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-id: 1489579606-26020-1-git-send-email-kraxel@redhat.com
---
hw/display/cirrus_vga.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index 326d511..a9f6d5b 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -667,11 +667,11 @@ static void cirrus_invalidate_region(CirrusVGAState * s, int off_begin,
}
for (y = 0; y < lines; y++) {
- off_cur = off_begin;
- off_cur_end = (off_cur + bytesperline) & s->cirrus_addr_mask;
+ off_cur = off_begin;
+ off_cur_end = ((off_cur + bytesperline - 1) & s->cirrus_addr_mask) + 1;
assert(off_cur_end >= off_cur);
memory_region_set_dirty(&s->vga.vram, off_cur, off_cur_end - off_cur);
- off_begin += off_pitch;
+ off_begin += off_pitch;
}
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Qemu-devel] [PULL for-2.9 6/7] cirrus: stop passing around dst pointers in the blitter
2017-03-16 9:30 [Qemu-devel] [PULL for-2.9 0/7] cirrus: more blitter security fixes Gerd Hoffmann
` (4 preceding siblings ...)
2017-03-16 9:30 ` [Qemu-devel] [PULL for-2.9 5/7] cirrus: fix cirrus_invalidate_region Gerd Hoffmann
@ 2017-03-16 9:30 ` Gerd Hoffmann
2017-03-16 9:30 ` [Qemu-devel] [PULL for-2.9 7/7] cirrus: stop passing around src " Gerd Hoffmann
2017-03-16 17:53 ` [Qemu-devel] [PULL for-2.9 0/7] cirrus: more blitter security fixes Peter Maydell
7 siblings, 0 replies; 12+ messages in thread
From: Gerd Hoffmann @ 2017-03-16 9:30 UTC (permalink / raw)
To: qemu-devel; +Cc: Gerd Hoffmann
Instead pass around the address (aka offset into vga memory). Calculate
the pointer in the rop_* functions, after applying the mask to the
address, to make sure the address stays within the valid range.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-id: 1489574872-8679-1-git-send-email-kraxel@redhat.com
---
hw/display/cirrus_vga.c | 20 +++---
hw/display/cirrus_vga_rop.h | 161 +++++++++++++++++++++++++------------------
hw/display/cirrus_vga_rop2.h | 97 +++++++++++++-------------
3 files changed, 153 insertions(+), 125 deletions(-)
diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index a9f6d5b..9ae1d4b 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -178,11 +178,12 @@
struct CirrusVGAState;
typedef void (*cirrus_bitblt_rop_t) (struct CirrusVGAState *s,
- uint8_t * dst, const uint8_t * src,
+ uint32_t dstaddr, const uint8_t *src,
int dstpitch, int srcpitch,
int bltwidth, int bltheight);
typedef void (*cirrus_fill_t)(struct CirrusVGAState *s,
- uint8_t *dst, int dst_pitch, int width, int height);
+ uint32_t dstaddr, int dst_pitch,
+ int width, int height);
typedef struct CirrusVGAState {
VGACommonState vga;
@@ -321,14 +322,14 @@ static bool blit_is_unsafe(struct CirrusVGAState *s, bool dst_only)
}
static void cirrus_bitblt_rop_nop(CirrusVGAState *s,
- uint8_t *dst,const uint8_t *src,
+ uint32_t dstaddr, const uint8_t *src,
int dstpitch,int srcpitch,
int bltwidth,int bltheight)
{
}
static void cirrus_bitblt_fill_nop(CirrusVGAState *s,
- uint8_t *dst,
+ uint32_t dstaddr,
int dstpitch, int bltwidth,int bltheight)
{
}
@@ -678,11 +679,8 @@ static void cirrus_invalidate_region(CirrusVGAState * s, int off_begin,
static int cirrus_bitblt_common_patterncopy(CirrusVGAState *s, bool videosrc)
{
uint32_t patternsize;
- uint8_t *dst;
uint8_t *src;
- dst = s->vga.vram_ptr + s->cirrus_blt_dstaddr;
-
if (videosrc) {
switch (s->vga.get_bpp(&s->vga)) {
case 8:
@@ -711,7 +709,7 @@ static int cirrus_bitblt_common_patterncopy(CirrusVGAState *s, bool videosrc)
return 0;
}
- (*s->cirrus_rop) (s, dst, src,
+ (*s->cirrus_rop) (s, s->cirrus_blt_dstaddr, src,
s->cirrus_blt_dstpitch, 0,
s->cirrus_blt_width, s->cirrus_blt_height);
cirrus_invalidate_region(s, s->cirrus_blt_dstaddr,
@@ -730,7 +728,7 @@ static int cirrus_bitblt_solidfill(CirrusVGAState *s, int blt_rop)
return 0;
}
rop_func = cirrus_fill[rop_to_index[blt_rop]][s->cirrus_blt_pixelwidth - 1];
- rop_func(s, s->vga.vram_ptr + s->cirrus_blt_dstaddr,
+ rop_func(s, s->cirrus_blt_dstaddr,
s->cirrus_blt_dstpitch,
s->cirrus_blt_width, s->cirrus_blt_height);
cirrus_invalidate_region(s, s->cirrus_blt_dstaddr,
@@ -797,7 +795,7 @@ static int cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h)
}
}
- (*s->cirrus_rop) (s, s->vga.vram_ptr + s->cirrus_blt_dstaddr,
+ (*s->cirrus_rop) (s, s->cirrus_blt_dstaddr,
s->vga.vram_ptr + s->cirrus_blt_srcaddr,
s->cirrus_blt_dstpitch, s->cirrus_blt_srcpitch,
s->cirrus_blt_width, s->cirrus_blt_height);
@@ -848,7 +846,7 @@ static void cirrus_bitblt_cputovideo_next(CirrusVGAState * s)
} else {
/* at least one scan line */
do {
- (*s->cirrus_rop)(s, s->vga.vram_ptr + s->cirrus_blt_dstaddr,
+ (*s->cirrus_rop)(s, s->cirrus_blt_dstaddr,
s->cirrus_bltbuf, 0, 0, s->cirrus_blt_width, 1);
cirrus_invalidate_region(s, s->cirrus_blt_dstaddr, 0,
s->cirrus_blt_width, 1);
diff --git a/hw/display/cirrus_vga_rop.h b/hw/display/cirrus_vga_rop.h
index b7447f8..1aa778d 100644
--- a/hw/display/cirrus_vga_rop.h
+++ b/hw/display/cirrus_vga_rop.h
@@ -22,31 +22,65 @@
* THE SOFTWARE.
*/
-static inline void glue(rop_8_,ROP_NAME)(uint8_t *dst, uint8_t src)
+static inline void glue(rop_8_, ROP_NAME)(CirrusVGAState *s,
+ uint32_t dstaddr, uint8_t src)
{
+ uint8_t *dst = &s->vga.vram_ptr[dstaddr & s->cirrus_addr_mask];
*dst = ROP_FN(*dst, src);
}
-static inline void glue(rop_16_,ROP_NAME)(uint16_t *dst, uint16_t src)
+static inline void glue(rop_tr_8_, ROP_NAME)(CirrusVGAState *s,
+ uint32_t dstaddr, uint8_t src,
+ uint8_t transp)
{
+ uint8_t *dst = &s->vga.vram_ptr[dstaddr & s->cirrus_addr_mask];
+ uint8_t pixel = ROP_FN(*dst, src);
+ if (pixel != transp) {
+ *dst = pixel;
+ }
+}
+
+static inline void glue(rop_16_, ROP_NAME)(CirrusVGAState *s,
+ uint32_t dstaddr, uint16_t src)
+{
+ uint16_t *dst = (uint16_t *)
+ (&s->vga.vram_ptr[dstaddr & s->cirrus_addr_mask & ~1]);
*dst = ROP_FN(*dst, src);
}
-static inline void glue(rop_32_,ROP_NAME)(uint32_t *dst, uint32_t src)
+static inline void glue(rop_tr_16_, ROP_NAME)(CirrusVGAState *s,
+ uint32_t dstaddr, uint16_t src,
+ uint16_t transp)
+{
+ uint16_t *dst = (uint16_t *)
+ (&s->vga.vram_ptr[dstaddr & s->cirrus_addr_mask & ~1]);
+ uint16_t pixel = ROP_FN(*dst, src);
+ if (pixel != transp) {
+ *dst = pixel;
+ }
+}
+
+static inline void glue(rop_32_, ROP_NAME)(CirrusVGAState *s,
+ uint32_t dstaddr, uint32_t src)
{
+ uint32_t *dst = (uint32_t *)
+ (&s->vga.vram_ptr[dstaddr & s->cirrus_addr_mask & ~3]);
*dst = ROP_FN(*dst, src);
}
-#define ROP_OP(d, s) glue(rop_8_,ROP_NAME)(d, s)
-#define ROP_OP_16(d, s) glue(rop_16_,ROP_NAME)(d, s)
-#define ROP_OP_32(d, s) glue(rop_32_,ROP_NAME)(d, s)
+#define ROP_OP(st, d, s) glue(rop_8_, ROP_NAME)(st, d, s)
+#define ROP_OP_TR(st, d, s, t) glue(rop_tr_8_, ROP_NAME)(st, d, s, t)
+#define ROP_OP_16(st, d, s) glue(rop_16_, ROP_NAME)(st, d, s)
+#define ROP_OP_TR_16(st, d, s, t) glue(rop_tr_16_, ROP_NAME)(st, d, s, t)
+#define ROP_OP_32(st, d, s) glue(rop_32_, ROP_NAME)(st, d, s)
#undef ROP_FN
static void
glue(cirrus_bitblt_rop_fwd_, ROP_NAME)(CirrusVGAState *s,
- uint8_t *dst,const uint8_t *src,
- int dstpitch,int srcpitch,
- int bltwidth,int bltheight)
+ uint32_t dstaddr,
+ const uint8_t *src,
+ int dstpitch, int srcpitch,
+ int bltwidth, int bltheight)
{
int x,y;
dstpitch -= bltwidth;
@@ -58,43 +92,47 @@ glue(cirrus_bitblt_rop_fwd_, ROP_NAME)(CirrusVGAState *s,
for (y = 0; y < bltheight; y++) {
for (x = 0; x < bltwidth; x++) {
- ROP_OP(dst, *src);
- dst++;
+ ROP_OP(s, dstaddr, *src);
+ dstaddr++;
src++;
}
- dst += dstpitch;
+ dstaddr += dstpitch;
src += srcpitch;
}
}
static void
glue(cirrus_bitblt_rop_bkwd_, ROP_NAME)(CirrusVGAState *s,
- uint8_t *dst,const uint8_t *src,
- int dstpitch,int srcpitch,
- int bltwidth,int bltheight)
+ uint32_t dstaddr,
+ const uint8_t *src,
+ int dstpitch, int srcpitch,
+ int bltwidth, int bltheight)
{
int x,y;
dstpitch += bltwidth;
srcpitch += bltwidth;
for (y = 0; y < bltheight; y++) {
for (x = 0; x < bltwidth; x++) {
- ROP_OP(dst, *src);
- dst--;
+ ROP_OP(s, dstaddr, *src);
+ dstaddr--;
src--;
}
- dst += dstpitch;
+ dstaddr += dstpitch;
src += srcpitch;
}
}
static void
glue(glue(cirrus_bitblt_rop_fwd_transp_, ROP_NAME),_8)(CirrusVGAState *s,
- uint8_t *dst,const uint8_t *src,
- int dstpitch,int srcpitch,
- int bltwidth,int bltheight)
+ uint32_t dstaddr,
+ const uint8_t *src,
+ int dstpitch,
+ int srcpitch,
+ int bltwidth,
+ int bltheight)
{
int x,y;
- uint8_t p;
+ uint8_t transp = s->vga.gr[0x34];
dstpitch -= bltwidth;
srcpitch -= bltwidth;
@@ -104,48 +142,50 @@ glue(glue(cirrus_bitblt_rop_fwd_transp_, ROP_NAME),_8)(CirrusVGAState *s,
for (y = 0; y < bltheight; y++) {
for (x = 0; x < bltwidth; x++) {
- p = *dst;
- ROP_OP(&p, *src);
- if (p != s->vga.gr[0x34]) *dst = p;
- dst++;
+ ROP_OP_TR(s, dstaddr, *src, transp);
+ dstaddr++;
src++;
}
- dst += dstpitch;
+ dstaddr += dstpitch;
src += srcpitch;
}
}
static void
glue(glue(cirrus_bitblt_rop_bkwd_transp_, ROP_NAME),_8)(CirrusVGAState *s,
- uint8_t *dst,const uint8_t *src,
- int dstpitch,int srcpitch,
- int bltwidth,int bltheight)
+ uint32_t dstaddr,
+ const uint8_t *src,
+ int dstpitch,
+ int srcpitch,
+ int bltwidth,
+ int bltheight)
{
int x,y;
- uint8_t p;
+ uint8_t transp = s->vga.gr[0x34];
dstpitch += bltwidth;
srcpitch += bltwidth;
for (y = 0; y < bltheight; y++) {
for (x = 0; x < bltwidth; x++) {
- p = *dst;
- ROP_OP(&p, *src);
- if (p != s->vga.gr[0x34]) *dst = p;
- dst--;
+ ROP_OP_TR(s, dstaddr, *src, transp);
+ dstaddr--;
src--;
}
- dst += dstpitch;
+ dstaddr += dstpitch;
src += srcpitch;
}
}
static void
glue(glue(cirrus_bitblt_rop_fwd_transp_, ROP_NAME),_16)(CirrusVGAState *s,
- uint8_t *dst,const uint8_t *src,
- int dstpitch,int srcpitch,
- int bltwidth,int bltheight)
+ uint32_t dstaddr,
+ const uint8_t *src,
+ int dstpitch,
+ int srcpitch,
+ int bltwidth,
+ int bltheight)
{
int x,y;
- uint8_t p1, p2;
+ uint16_t transp = s->vga.gr[0x34] | (uint16_t)s->vga.gr[0x35] << 8;
dstpitch -= bltwidth;
srcpitch -= bltwidth;
@@ -155,46 +195,35 @@ glue(glue(cirrus_bitblt_rop_fwd_transp_, ROP_NAME),_16)(CirrusVGAState *s,
for (y = 0; y < bltheight; y++) {
for (x = 0; x < bltwidth; x+=2) {
- p1 = *dst;
- p2 = *(dst+1);
- ROP_OP(&p1, *src);
- ROP_OP(&p2, *(src + 1));
- if ((p1 != s->vga.gr[0x34]) || (p2 != s->vga.gr[0x35])) {
- *dst = p1;
- *(dst+1) = p2;
- }
- dst+=2;
- src+=2;
+ ROP_OP_TR_16(s, dstaddr, *(uint16_t *)src, transp);
+ dstaddr += 2;
+ src += 2;
}
- dst += dstpitch;
+ dstaddr += dstpitch;
src += srcpitch;
}
}
static void
glue(glue(cirrus_bitblt_rop_bkwd_transp_, ROP_NAME),_16)(CirrusVGAState *s,
- uint8_t *dst,const uint8_t *src,
- int dstpitch,int srcpitch,
- int bltwidth,int bltheight)
+ uint32_t dstaddr,
+ const uint8_t *src,
+ int dstpitch,
+ int srcpitch,
+ int bltwidth,
+ int bltheight)
{
int x,y;
- uint8_t p1, p2;
+ uint16_t transp = s->vga.gr[0x34] | (uint16_t)s->vga.gr[0x35] << 8;
dstpitch += bltwidth;
srcpitch += bltwidth;
for (y = 0; y < bltheight; y++) {
for (x = 0; x < bltwidth; x+=2) {
- p1 = *(dst-1);
- p2 = *dst;
- ROP_OP(&p1, *(src - 1));
- ROP_OP(&p2, *src);
- if ((p1 != s->vga.gr[0x34]) || (p2 != s->vga.gr[0x35])) {
- *(dst-1) = p1;
- *dst = p2;
- }
- dst-=2;
- src-=2;
+ ROP_OP_TR_16(s, dstaddr, *(uint16_t *)src, transp);
+ dstaddr -= 2;
+ src -= 2;
}
- dst += dstpitch;
+ dstaddr += dstpitch;
src += srcpitch;
}
}
diff --git a/hw/display/cirrus_vga_rop2.h b/hw/display/cirrus_vga_rop2.h
index d28bcc6..bc92f0e 100644
--- a/hw/display/cirrus_vga_rop2.h
+++ b/hw/display/cirrus_vga_rop2.h
@@ -23,27 +23,29 @@
*/
#if DEPTH == 8
-#define PUTPIXEL() ROP_OP(&d[0], col)
+#define PUTPIXEL(s, a, c) ROP_OP(s, a, c)
#elif DEPTH == 16
-#define PUTPIXEL() ROP_OP_16((uint16_t *)&d[0], col)
+#define PUTPIXEL(s, a, c) ROP_OP_16(s, a, c)
#elif DEPTH == 24
-#define PUTPIXEL() ROP_OP(&d[0], col); \
- ROP_OP(&d[1], (col >> 8)); \
- ROP_OP(&d[2], (col >> 16))
+#define PUTPIXEL(s, a, c) do { \
+ ROP_OP(s, a, c); \
+ ROP_OP(s, a + 1, (col >> 8)); \
+ ROP_OP(s, a + 2, (col >> 16)); \
+ } while (0)
#elif DEPTH == 32
-#define PUTPIXEL() ROP_OP_32(((uint32_t *)&d[0]), col)
+#define PUTPIXEL(s, a, c) ROP_OP_32(s, a, c)
#else
#error unsupported DEPTH
#endif
static void
glue(glue(glue(cirrus_patternfill_, ROP_NAME), _),DEPTH)
- (CirrusVGAState * s, uint8_t * dst,
- const uint8_t * src,
+ (CirrusVGAState *s, uint32_t dstaddr,
+ const uint8_t *src,
int dstpitch, int srcpitch,
int bltwidth, int bltheight)
{
- uint8_t *d;
+ uint32_t addr;
int x, y, pattern_y, pattern_pitch, pattern_x;
unsigned int col;
const uint8_t *src1;
@@ -63,7 +65,7 @@ glue(glue(glue(cirrus_patternfill_, ROP_NAME), _),DEPTH)
pattern_y = s->cirrus_blt_srcaddr & 7;
for(y = 0; y < bltheight; y++) {
pattern_x = skipleft;
- d = dst + skipleft;
+ addr = dstaddr + skipleft;
src1 = src + pattern_y * pattern_pitch;
for (x = skipleft; x < bltwidth; x += (DEPTH / 8)) {
#if DEPTH == 8
@@ -82,23 +84,23 @@ glue(glue(glue(cirrus_patternfill_, ROP_NAME), _),DEPTH)
col = ((uint32_t *)(src1 + pattern_x))[0];
pattern_x = (pattern_x + 4) & 31;
#endif
- PUTPIXEL();
- d += (DEPTH / 8);
+ PUTPIXEL(s, addr, col);
+ addr += (DEPTH / 8);
}
pattern_y = (pattern_y + 1) & 7;
- dst += dstpitch;
+ dstaddr += dstpitch;
}
}
/* NOTE: srcpitch is ignored */
static void
glue(glue(glue(cirrus_colorexpand_transp_, ROP_NAME), _),DEPTH)
- (CirrusVGAState * s, uint8_t * dst,
- const uint8_t * src,
+ (CirrusVGAState *s, uint32_t dstaddr,
+ const uint8_t *src,
int dstpitch, int srcpitch,
int bltwidth, int bltheight)
{
- uint8_t *d;
+ uint32_t addr;
int x, y;
unsigned bits, bits_xor;
unsigned int col;
@@ -123,7 +125,7 @@ glue(glue(glue(cirrus_colorexpand_transp_, ROP_NAME), _),DEPTH)
for(y = 0; y < bltheight; y++) {
bitmask = 0x80 >> srcskipleft;
bits = *src++ ^ bits_xor;
- d = dst + dstskipleft;
+ addr = dstaddr + dstskipleft;
for (x = dstskipleft; x < bltwidth; x += (DEPTH / 8)) {
if ((bitmask & 0xff) == 0) {
bitmask = 0x80;
@@ -131,24 +133,24 @@ glue(glue(glue(cirrus_colorexpand_transp_, ROP_NAME), _),DEPTH)
}
index = (bits & bitmask);
if (index) {
- PUTPIXEL();
+ PUTPIXEL(s, addr, col);
}
- d += (DEPTH / 8);
+ addr += (DEPTH / 8);
bitmask >>= 1;
}
- dst += dstpitch;
+ dstaddr += dstpitch;
}
}
static void
glue(glue(glue(cirrus_colorexpand_, ROP_NAME), _),DEPTH)
- (CirrusVGAState * s, uint8_t * dst,
- const uint8_t * src,
+ (CirrusVGAState *s, uint32_t dstaddr,
+ const uint8_t *src,
int dstpitch, int srcpitch,
int bltwidth, int bltheight)
{
uint32_t colors[2];
- uint8_t *d;
+ uint32_t addr;
int x, y;
unsigned bits;
unsigned int col;
@@ -161,29 +163,29 @@ glue(glue(glue(cirrus_colorexpand_, ROP_NAME), _),DEPTH)
for(y = 0; y < bltheight; y++) {
bitmask = 0x80 >> srcskipleft;
bits = *src++;
- d = dst + dstskipleft;
+ addr = dstaddr + dstskipleft;
for (x = dstskipleft; x < bltwidth; x += (DEPTH / 8)) {
if ((bitmask & 0xff) == 0) {
bitmask = 0x80;
bits = *src++;
}
col = colors[!!(bits & bitmask)];
- PUTPIXEL();
- d += (DEPTH / 8);
+ PUTPIXEL(s, addr, col);
+ addr += (DEPTH / 8);
bitmask >>= 1;
}
- dst += dstpitch;
+ dstaddr += dstpitch;
}
}
static void
glue(glue(glue(cirrus_colorexpand_pattern_transp_, ROP_NAME), _),DEPTH)
- (CirrusVGAState * s, uint8_t * dst,
- const uint8_t * src,
+ (CirrusVGAState *s, uint32_t dstaddr,
+ const uint8_t *src,
int dstpitch, int srcpitch,
int bltwidth, int bltheight)
{
- uint8_t *d;
+ uint32_t addr;
int x, y, bitpos, pattern_y;
unsigned int bits, bits_xor;
unsigned int col;
@@ -207,28 +209,28 @@ glue(glue(glue(cirrus_colorexpand_pattern_transp_, ROP_NAME), _),DEPTH)
for(y = 0; y < bltheight; y++) {
bits = src[pattern_y] ^ bits_xor;
bitpos = 7 - srcskipleft;
- d = dst + dstskipleft;
+ addr = dstaddr + dstskipleft;
for (x = dstskipleft; x < bltwidth; x += (DEPTH / 8)) {
if ((bits >> bitpos) & 1) {
- PUTPIXEL();
+ PUTPIXEL(s, addr, col);
}
- d += (DEPTH / 8);
+ addr += (DEPTH / 8);
bitpos = (bitpos - 1) & 7;
}
pattern_y = (pattern_y + 1) & 7;
- dst += dstpitch;
+ dstaddr += dstpitch;
}
}
static void
glue(glue(glue(cirrus_colorexpand_pattern_, ROP_NAME), _),DEPTH)
- (CirrusVGAState * s, uint8_t * dst,
- const uint8_t * src,
+ (CirrusVGAState *s, uint32_t dstaddr,
+ const uint8_t *src,
int dstpitch, int srcpitch,
int bltwidth, int bltheight)
{
uint32_t colors[2];
- uint8_t *d;
+ uint32_t addr;
int x, y, bitpos, pattern_y;
unsigned int bits;
unsigned int col;
@@ -242,38 +244,37 @@ glue(glue(glue(cirrus_colorexpand_pattern_, ROP_NAME), _),DEPTH)
for(y = 0; y < bltheight; y++) {
bits = src[pattern_y];
bitpos = 7 - srcskipleft;
- d = dst + dstskipleft;
+ addr = dstaddr + dstskipleft;
for (x = dstskipleft; x < bltwidth; x += (DEPTH / 8)) {
col = colors[(bits >> bitpos) & 1];
- PUTPIXEL();
- d += (DEPTH / 8);
+ PUTPIXEL(s, addr, col);
+ addr += (DEPTH / 8);
bitpos = (bitpos - 1) & 7;
}
pattern_y = (pattern_y + 1) & 7;
- dst += dstpitch;
+ dstaddr += dstpitch;
}
}
static void
glue(glue(glue(cirrus_fill_, ROP_NAME), _),DEPTH)
(CirrusVGAState *s,
- uint8_t *dst, int dst_pitch,
+ uint32_t dstaddr, int dst_pitch,
int width, int height)
{
- uint8_t *d, *d1;
+ uint32_t addr;
uint32_t col;
int x, y;
col = s->cirrus_blt_fgcol;
- d1 = dst;
for(y = 0; y < height; y++) {
- d = d1;
+ addr = dstaddr;
for(x = 0; x < width; x += (DEPTH / 8)) {
- PUTPIXEL();
- d += (DEPTH / 8);
+ PUTPIXEL(s, addr, col);
+ addr += (DEPTH / 8);
}
- d1 += dst_pitch;
+ dstaddr += dst_pitch;
}
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Qemu-devel] [PULL for-2.9 7/7] cirrus: stop passing around src pointers in the blitter
2017-03-16 9:30 [Qemu-devel] [PULL for-2.9 0/7] cirrus: more blitter security fixes Gerd Hoffmann
` (5 preceding siblings ...)
2017-03-16 9:30 ` [Qemu-devel] [PULL for-2.9 6/7] cirrus: stop passing around dst pointers in the blitter Gerd Hoffmann
@ 2017-03-16 9:30 ` Gerd Hoffmann
2017-03-16 17:53 ` [Qemu-devel] [PULL for-2.9 0/7] cirrus: more blitter security fixes Peter Maydell
7 siblings, 0 replies; 12+ messages in thread
From: Gerd Hoffmann @ 2017-03-16 9:30 UTC (permalink / raw)
To: qemu-devel; +Cc: Gerd Hoffmann
Does basically the same as "cirrus: stop passing around dst pointers in
the blitter", just for the src pointer instead of the dst pointer.
For the src we have to care about cputovideo blits though and fetch the
data from s->cirrus_bltbuf instead of vga memory. The cirrus_src*()
helper functions handle that.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-id: 1489584487-3489-1-git-send-email-kraxel@redhat.com
---
hw/display/cirrus_vga.c | 61 +++++++++++++++++++++++++++++++++++---------
hw/display/cirrus_vga_rop.h | 48 +++++++++++++++++-----------------
hw/display/cirrus_vga_rop2.h | 38 ++++++++++++++-------------
3 files changed, 93 insertions(+), 54 deletions(-)
diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index 9ae1d4b..afc290a 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -178,7 +178,7 @@
struct CirrusVGAState;
typedef void (*cirrus_bitblt_rop_t) (struct CirrusVGAState *s,
- uint32_t dstaddr, const uint8_t *src,
+ uint32_t dstaddr, uint32_t srcaddr,
int dstpitch, int srcpitch,
int bltwidth, int bltheight);
typedef void (*cirrus_fill_t)(struct CirrusVGAState *s,
@@ -322,7 +322,7 @@ static bool blit_is_unsafe(struct CirrusVGAState *s, bool dst_only)
}
static void cirrus_bitblt_rop_nop(CirrusVGAState *s,
- uint32_t dstaddr, const uint8_t *src,
+ uint32_t dstaddr, uint32_t srcaddr,
int dstpitch,int srcpitch,
int bltwidth,int bltheight)
{
@@ -334,6 +334,45 @@ static void cirrus_bitblt_fill_nop(CirrusVGAState *s,
{
}
+static inline uint8_t cirrus_src(CirrusVGAState *s, uint32_t srcaddr)
+{
+ if (s->cirrus_srccounter) {
+ /* cputovideo */
+ return s->cirrus_bltbuf[srcaddr & (CIRRUS_BLTBUFSIZE - 1)];
+ } else {
+ /* videotovideo */
+ return s->vga.vram_ptr[srcaddr & s->cirrus_addr_mask];
+ }
+}
+
+static inline uint16_t cirrus_src16(CirrusVGAState *s, uint32_t srcaddr)
+{
+ uint16_t *src;
+
+ if (s->cirrus_srccounter) {
+ /* cputovideo */
+ src = (void *)&s->cirrus_bltbuf[srcaddr & (CIRRUS_BLTBUFSIZE - 1) & ~1];
+ } else {
+ /* videotovideo */
+ src = (void *)&s->vga.vram_ptr[srcaddr & s->cirrus_addr_mask & ~1];
+ }
+ return *src;
+}
+
+static inline uint32_t cirrus_src32(CirrusVGAState *s, uint32_t srcaddr)
+{
+ uint32_t *src;
+
+ if (s->cirrus_srccounter) {
+ /* cputovideo */
+ src = (void *)&s->cirrus_bltbuf[srcaddr & (CIRRUS_BLTBUFSIZE - 1) & ~3];
+ } else {
+ /* videotovideo */
+ src = (void *)&s->vga.vram_ptr[srcaddr & s->cirrus_addr_mask & ~3];
+ }
+ return *src;
+}
+
#define ROP_NAME 0
#define ROP_FN(d, s) 0
#include "cirrus_vga_rop.h"
@@ -676,10 +715,10 @@ static void cirrus_invalidate_region(CirrusVGAState * s, int off_begin,
}
}
-static int cirrus_bitblt_common_patterncopy(CirrusVGAState *s, bool videosrc)
+static int cirrus_bitblt_common_patterncopy(CirrusVGAState *s)
{
uint32_t patternsize;
- uint8_t *src;
+ bool videosrc = !s->cirrus_srccounter;
if (videosrc) {
switch (s->vga.get_bpp(&s->vga)) {
@@ -700,16 +739,14 @@ static int cirrus_bitblt_common_patterncopy(CirrusVGAState *s, bool videosrc)
if (s->cirrus_blt_srcaddr + patternsize > s->vga.vram_size) {
return 0;
}
- src = s->vga.vram_ptr + s->cirrus_blt_srcaddr;
- } else {
- src = s->cirrus_bltbuf;
}
if (blit_is_unsafe(s, true)) {
return 0;
}
- (*s->cirrus_rop) (s, s->cirrus_blt_dstaddr, src,
+ (*s->cirrus_rop) (s, s->cirrus_blt_dstaddr,
+ videosrc ? s->cirrus_blt_srcaddr : 0,
s->cirrus_blt_dstpitch, 0,
s->cirrus_blt_width, s->cirrus_blt_height);
cirrus_invalidate_region(s, s->cirrus_blt_dstaddr,
@@ -746,7 +783,7 @@ static int cirrus_bitblt_solidfill(CirrusVGAState *s, int blt_rop)
static int cirrus_bitblt_videotovideo_patterncopy(CirrusVGAState * s)
{
- return cirrus_bitblt_common_patterncopy(s, true);
+ return cirrus_bitblt_common_patterncopy(s);
}
static int cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h)
@@ -796,7 +833,7 @@ static int cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h)
}
(*s->cirrus_rop) (s, s->cirrus_blt_dstaddr,
- s->vga.vram_ptr + s->cirrus_blt_srcaddr,
+ s->cirrus_blt_srcaddr,
s->cirrus_blt_dstpitch, s->cirrus_blt_srcpitch,
s->cirrus_blt_width, s->cirrus_blt_height);
@@ -839,7 +876,7 @@ static void cirrus_bitblt_cputovideo_next(CirrusVGAState * s)
if (s->cirrus_srccounter > 0) {
if (s->cirrus_blt_mode & CIRRUS_BLTMODE_PATTERNCOPY) {
- cirrus_bitblt_common_patterncopy(s, false);
+ cirrus_bitblt_common_patterncopy(s);
the_end:
s->cirrus_srccounter = 0;
cirrus_bitblt_reset(s);
@@ -847,7 +884,7 @@ static void cirrus_bitblt_cputovideo_next(CirrusVGAState * s)
/* at least one scan line */
do {
(*s->cirrus_rop)(s, s->cirrus_blt_dstaddr,
- s->cirrus_bltbuf, 0, 0, s->cirrus_blt_width, 1);
+ 0, 0, 0, s->cirrus_blt_width, 1);
cirrus_invalidate_region(s, s->cirrus_blt_dstaddr, 0,
s->cirrus_blt_width, 1);
s->cirrus_blt_dstaddr += s->cirrus_blt_dstpitch;
diff --git a/hw/display/cirrus_vga_rop.h b/hw/display/cirrus_vga_rop.h
index 1aa778d..c61a677 100644
--- a/hw/display/cirrus_vga_rop.h
+++ b/hw/display/cirrus_vga_rop.h
@@ -78,7 +78,7 @@ static inline void glue(rop_32_, ROP_NAME)(CirrusVGAState *s,
static void
glue(cirrus_bitblt_rop_fwd_, ROP_NAME)(CirrusVGAState *s,
uint32_t dstaddr,
- const uint8_t *src,
+ uint32_t srcaddr,
int dstpitch, int srcpitch,
int bltwidth, int bltheight)
{
@@ -92,19 +92,19 @@ glue(cirrus_bitblt_rop_fwd_, ROP_NAME)(CirrusVGAState *s,
for (y = 0; y < bltheight; y++) {
for (x = 0; x < bltwidth; x++) {
- ROP_OP(s, dstaddr, *src);
+ ROP_OP(s, dstaddr, cirrus_src(s, srcaddr));
dstaddr++;
- src++;
+ srcaddr++;
}
dstaddr += dstpitch;
- src += srcpitch;
+ srcaddr += srcpitch;
}
}
static void
glue(cirrus_bitblt_rop_bkwd_, ROP_NAME)(CirrusVGAState *s,
uint32_t dstaddr,
- const uint8_t *src,
+ uint32_t srcaddr,
int dstpitch, int srcpitch,
int bltwidth, int bltheight)
{
@@ -113,19 +113,19 @@ glue(cirrus_bitblt_rop_bkwd_, ROP_NAME)(CirrusVGAState *s,
srcpitch += bltwidth;
for (y = 0; y < bltheight; y++) {
for (x = 0; x < bltwidth; x++) {
- ROP_OP(s, dstaddr, *src);
+ ROP_OP(s, dstaddr, cirrus_src(s, srcaddr));
dstaddr--;
- src--;
+ srcaddr--;
}
dstaddr += dstpitch;
- src += srcpitch;
+ srcaddr += srcpitch;
}
}
static void
glue(glue(cirrus_bitblt_rop_fwd_transp_, ROP_NAME),_8)(CirrusVGAState *s,
uint32_t dstaddr,
- const uint8_t *src,
+ uint32_t srcaddr,
int dstpitch,
int srcpitch,
int bltwidth,
@@ -142,19 +142,19 @@ glue(glue(cirrus_bitblt_rop_fwd_transp_, ROP_NAME),_8)(CirrusVGAState *s,
for (y = 0; y < bltheight; y++) {
for (x = 0; x < bltwidth; x++) {
- ROP_OP_TR(s, dstaddr, *src, transp);
+ ROP_OP_TR(s, dstaddr, cirrus_src(s, srcaddr), transp);
dstaddr++;
- src++;
+ srcaddr++;
}
dstaddr += dstpitch;
- src += srcpitch;
+ srcaddr += srcpitch;
}
}
static void
glue(glue(cirrus_bitblt_rop_bkwd_transp_, ROP_NAME),_8)(CirrusVGAState *s,
uint32_t dstaddr,
- const uint8_t *src,
+ uint32_t srcaddr,
int dstpitch,
int srcpitch,
int bltwidth,
@@ -166,19 +166,19 @@ glue(glue(cirrus_bitblt_rop_bkwd_transp_, ROP_NAME),_8)(CirrusVGAState *s,
srcpitch += bltwidth;
for (y = 0; y < bltheight; y++) {
for (x = 0; x < bltwidth; x++) {
- ROP_OP_TR(s, dstaddr, *src, transp);
+ ROP_OP_TR(s, dstaddr, cirrus_src(s, srcaddr), transp);
dstaddr--;
- src--;
+ srcaddr--;
}
dstaddr += dstpitch;
- src += srcpitch;
+ srcaddr += srcpitch;
}
}
static void
glue(glue(cirrus_bitblt_rop_fwd_transp_, ROP_NAME),_16)(CirrusVGAState *s,
uint32_t dstaddr,
- const uint8_t *src,
+ uint32_t srcaddr,
int dstpitch,
int srcpitch,
int bltwidth,
@@ -195,19 +195,19 @@ glue(glue(cirrus_bitblt_rop_fwd_transp_, ROP_NAME),_16)(CirrusVGAState *s,
for (y = 0; y < bltheight; y++) {
for (x = 0; x < bltwidth; x+=2) {
- ROP_OP_TR_16(s, dstaddr, *(uint16_t *)src, transp);
+ ROP_OP_TR_16(s, dstaddr, cirrus_src16(s, srcaddr), transp);
dstaddr += 2;
- src += 2;
+ srcaddr += 2;
}
dstaddr += dstpitch;
- src += srcpitch;
+ srcaddr += srcpitch;
}
}
static void
glue(glue(cirrus_bitblt_rop_bkwd_transp_, ROP_NAME),_16)(CirrusVGAState *s,
uint32_t dstaddr,
- const uint8_t *src,
+ uint32_t srcaddr,
int dstpitch,
int srcpitch,
int bltwidth,
@@ -219,12 +219,12 @@ glue(glue(cirrus_bitblt_rop_bkwd_transp_, ROP_NAME),_16)(CirrusVGAState *s,
srcpitch += bltwidth;
for (y = 0; y < bltheight; y++) {
for (x = 0; x < bltwidth; x+=2) {
- ROP_OP_TR_16(s, dstaddr, *(uint16_t *)src, transp);
+ ROP_OP_TR_16(s, dstaddr, cirrus_src16(s, srcaddr), transp);
dstaddr -= 2;
- src -= 2;
+ srcaddr -= 2;
}
dstaddr += dstpitch;
- src += srcpitch;
+ srcaddr += srcpitch;
}
}
diff --git a/hw/display/cirrus_vga_rop2.h b/hw/display/cirrus_vga_rop2.h
index bc92f0e..b86bcd6 100644
--- a/hw/display/cirrus_vga_rop2.h
+++ b/hw/display/cirrus_vga_rop2.h
@@ -41,14 +41,14 @@
static void
glue(glue(glue(cirrus_patternfill_, ROP_NAME), _),DEPTH)
(CirrusVGAState *s, uint32_t dstaddr,
- const uint8_t *src,
+ uint32_t srcaddr,
int dstpitch, int srcpitch,
int bltwidth, int bltheight)
{
uint32_t addr;
int x, y, pattern_y, pattern_pitch, pattern_x;
unsigned int col;
- const uint8_t *src1;
+ uint32_t src1addr;
#if DEPTH == 24
int skipleft = s->vga.gr[0x2f] & 0x1f;
#else
@@ -66,22 +66,24 @@ glue(glue(glue(cirrus_patternfill_, ROP_NAME), _),DEPTH)
for(y = 0; y < bltheight; y++) {
pattern_x = skipleft;
addr = dstaddr + skipleft;
- src1 = src + pattern_y * pattern_pitch;
+ src1addr = srcaddr + pattern_y * pattern_pitch;
for (x = skipleft; x < bltwidth; x += (DEPTH / 8)) {
#if DEPTH == 8
- col = src1[pattern_x];
+ col = cirrus_src(s, src1addr + pattern_x);
pattern_x = (pattern_x + 1) & 7;
#elif DEPTH == 16
- col = ((uint16_t *)(src1 + pattern_x))[0];
+ col = cirrus_src16(s, src1addr + pattern_x);
pattern_x = (pattern_x + 2) & 15;
#elif DEPTH == 24
{
- const uint8_t *src2 = src1 + pattern_x * 3;
- col = src2[0] | (src2[1] << 8) | (src2[2] << 16);
+ uint32_t src2addr = src1addr + pattern_x * 3;
+ col = cirrus_src(s, src2addr) |
+ (cirrus_src(s, src2addr + 1) << 8) |
+ (cirrus_src(s, src2addr + 2) << 16);
pattern_x = (pattern_x + 1) & 7;
}
#else
- col = ((uint32_t *)(src1 + pattern_x))[0];
+ col = cirrus_src32(s, src1addr + pattern_x);
pattern_x = (pattern_x + 4) & 31;
#endif
PUTPIXEL(s, addr, col);
@@ -96,7 +98,7 @@ glue(glue(glue(cirrus_patternfill_, ROP_NAME), _),DEPTH)
static void
glue(glue(glue(cirrus_colorexpand_transp_, ROP_NAME), _),DEPTH)
(CirrusVGAState *s, uint32_t dstaddr,
- const uint8_t *src,
+ uint32_t srcaddr,
int dstpitch, int srcpitch,
int bltwidth, int bltheight)
{
@@ -124,12 +126,12 @@ glue(glue(glue(cirrus_colorexpand_transp_, ROP_NAME), _),DEPTH)
for(y = 0; y < bltheight; y++) {
bitmask = 0x80 >> srcskipleft;
- bits = *src++ ^ bits_xor;
+ bits = cirrus_src(s, srcaddr++) ^ bits_xor;
addr = dstaddr + dstskipleft;
for (x = dstskipleft; x < bltwidth; x += (DEPTH / 8)) {
if ((bitmask & 0xff) == 0) {
bitmask = 0x80;
- bits = *src++ ^ bits_xor;
+ bits = cirrus_src(s, srcaddr++) ^ bits_xor;
}
index = (bits & bitmask);
if (index) {
@@ -145,7 +147,7 @@ glue(glue(glue(cirrus_colorexpand_transp_, ROP_NAME), _),DEPTH)
static void
glue(glue(glue(cirrus_colorexpand_, ROP_NAME), _),DEPTH)
(CirrusVGAState *s, uint32_t dstaddr,
- const uint8_t *src,
+ uint32_t srcaddr,
int dstpitch, int srcpitch,
int bltwidth, int bltheight)
{
@@ -162,12 +164,12 @@ glue(glue(glue(cirrus_colorexpand_, ROP_NAME), _),DEPTH)
colors[1] = s->cirrus_blt_fgcol;
for(y = 0; y < bltheight; y++) {
bitmask = 0x80 >> srcskipleft;
- bits = *src++;
+ bits = cirrus_src(s, srcaddr++);
addr = dstaddr + dstskipleft;
for (x = dstskipleft; x < bltwidth; x += (DEPTH / 8)) {
if ((bitmask & 0xff) == 0) {
bitmask = 0x80;
- bits = *src++;
+ bits = cirrus_src(s, srcaddr++);
}
col = colors[!!(bits & bitmask)];
PUTPIXEL(s, addr, col);
@@ -181,7 +183,7 @@ glue(glue(glue(cirrus_colorexpand_, ROP_NAME), _),DEPTH)
static void
glue(glue(glue(cirrus_colorexpand_pattern_transp_, ROP_NAME), _),DEPTH)
(CirrusVGAState *s, uint32_t dstaddr,
- const uint8_t *src,
+ uint32_t srcaddr,
int dstpitch, int srcpitch,
int bltwidth, int bltheight)
{
@@ -207,7 +209,7 @@ glue(glue(glue(cirrus_colorexpand_pattern_transp_, ROP_NAME), _),DEPTH)
pattern_y = s->cirrus_blt_srcaddr & 7;
for(y = 0; y < bltheight; y++) {
- bits = src[pattern_y] ^ bits_xor;
+ bits = cirrus_src(s, srcaddr + pattern_y) ^ bits_xor;
bitpos = 7 - srcskipleft;
addr = dstaddr + dstskipleft;
for (x = dstskipleft; x < bltwidth; x += (DEPTH / 8)) {
@@ -225,7 +227,7 @@ glue(glue(glue(cirrus_colorexpand_pattern_transp_, ROP_NAME), _),DEPTH)
static void
glue(glue(glue(cirrus_colorexpand_pattern_, ROP_NAME), _),DEPTH)
(CirrusVGAState *s, uint32_t dstaddr,
- const uint8_t *src,
+ uint32_t srcaddr,
int dstpitch, int srcpitch,
int bltwidth, int bltheight)
{
@@ -242,7 +244,7 @@ glue(glue(glue(cirrus_colorexpand_pattern_, ROP_NAME), _),DEPTH)
pattern_y = s->cirrus_blt_srcaddr & 7;
for(y = 0; y < bltheight; y++) {
- bits = src[pattern_y];
+ bits = cirrus_src(s, srcaddr + pattern_y);
bitpos = 7 - srcskipleft;
addr = dstaddr + dstskipleft;
for (x = dstskipleft; x < bltwidth; x += (DEPTH / 8)) {
--
1.8.3.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PULL for-2.9 4/7] cirrus: add option to disable blitter
2017-03-16 9:30 ` [Qemu-devel] [PULL for-2.9 4/7] cirrus: add option to disable blitter Gerd Hoffmann
@ 2017-03-16 9:51 ` 李强
2017-03-16 11:07 ` Thomas Huth
2017-03-16 14:00 ` Gerd Hoffmann
0 siblings, 2 replies; 12+ messages in thread
From: 李强 @ 2017-03-16 9:51 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: qemu-devel
Hello Gerd,
> -----Original Message-----
> From: Qemu-devel
> [mailto:qemu-devel-bounces+liqiang6-s=360.cn@nongnu.org] On Behalf Of
> Gerd Hoffmann
> Sent: Thursday, March 16, 2017 5:31 PM
> To: qemu-devel@nongnu.org
> Cc: Gerd Hoffmann
> Subject: [Qemu-devel] [PULL for-2.9 4/7] cirrus: add option to disable blitter
>
> Ok, we have this beast in the cirrus code which is not used at all by modern
> guests, except when you try to find security holes in qemu. So, add an option
> to disable blitter altogether. Guests released within the last ten years should
> not show any rendering issues if you turn off blitter support.
>
> There are no known bugs in the cirrus blitter code. But in the past we hoped a
> few times already that we've finally nailed the last issue. So having some easy
> way to mitigate in case yet another blitter issue shows up certainly makes me
> sleep a bit better at night.
>
> For completeness: The by far better way to mitigate is to switch away from
> cirrus and use stdvga instead. Or something more modern like virtio-vga in
> case your guest has support for it.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Message-id: 1489494540-15745-1-git-send-email-kraxel@redhat.com
> ---
> hw/display/cirrus_vga.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c index
> 6ffe64f..326d511 100644
> --- a/hw/display/cirrus_vga.c
> +++ b/hw/display/cirrus_vga.c
> @@ -205,6 +205,7 @@ typedef struct CirrusVGAState {
> uint32_t cirrus_bank_base[2];
> uint32_t cirrus_bank_limit[2];
> uint8_t cirrus_hidden_palette[48];
> + bool enable_blitter;
> int cirrus_blt_pixelwidth;
> int cirrus_blt_width;
> int cirrus_blt_height;
> @@ -960,6 +961,10 @@ static void cirrus_bitblt_start(CirrusVGAState * s) {
> uint8_t blt_rop;
>
> + if (!s->enable_blitter) {
> + goto bitblt_ignore;
> + }
> +
> s->vga.gr[0x31] |= CIRRUS_BLT_BUSY;
>
> s->cirrus_blt_width = (s->vga.gr[0x20] | (s->vga.gr[0x21] << 8)) + 1; @@
> -3024,6 +3029,8 @@ static void isa_cirrus_vga_realizefn(DeviceState *dev,
> Error **errp) static Property isa_cirrus_vga_properties[] = {
> DEFINE_PROP_UINT32("vgamem_mb", struct ISACirrusVGAState,
> cirrus_vga.vga.vram_size_mb, 4),
> + DEFINE_PROP_BOOL("blitter", struct ISACirrusVGAState,
> + cirrus_vga.enable_blitter, true),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> @@ -3093,6 +3100,8 @@ static void pci_cirrus_vga_realize(PCIDevice *dev,
> Error **errp) static Property pci_vga_cirrus_properties[] = {
> DEFINE_PROP_UINT32("vgamem_mb", struct PCICirrusVGAState,
> cirrus_vga.vga.vram_size_mb, 4),
> + DEFINE_PROP_BOOL("blitter", struct PCICirrusVGAState,
> + cirrus_vga.enable_blitter, true),
The default is 'ENABLE'? I think there should be 'false'.
Thanks.
Qiang
> DEFINE_PROP_END_OF_LIST(),
> };
>
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PULL for-2.9 4/7] cirrus: add option to disable blitter
2017-03-16 9:51 ` 李强
@ 2017-03-16 11:07 ` Thomas Huth
2017-03-16 14:00 ` Gerd Hoffmann
1 sibling, 0 replies; 12+ messages in thread
From: Thomas Huth @ 2017-03-16 11:07 UTC (permalink / raw)
To: 李强, Gerd Hoffmann; +Cc: qemu-devel
On 16.03.2017 10:51, 李强 wrote:
> Hello Gerd,
>
>> -----Original Message-----
>> From: Qemu-devel
>> [mailto:qemu-devel-bounces+liqiang6-s=360.cn@nongnu.org] On Behalf Of
>> Gerd Hoffmann
>> Sent: Thursday, March 16, 2017 5:31 PM
>> To: qemu-devel@nongnu.org
>> Cc: Gerd Hoffmann
>> Subject: [Qemu-devel] [PULL for-2.9 4/7] cirrus: add option to disable blitter
>>
>> Ok, we have this beast in the cirrus code which is not used at all by modern
>> guests, except when you try to find security holes in qemu. So, add an option
>> to disable blitter altogether. Guests released within the last ten years should
>> not show any rendering issues if you turn off blitter support.
>>
>> There are no known bugs in the cirrus blitter code. But in the past we hoped a
>> few times already that we've finally nailed the last issue. So having some easy
>> way to mitigate in case yet another blitter issue shows up certainly makes me
>> sleep a bit better at night.
>>
>> For completeness: The by far better way to mitigate is to switch away from
>> cirrus and use stdvga instead. Or something more modern like virtio-vga in
>> case your guest has support for it.
>>
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>> Message-id: 1489494540-15745-1-git-send-email-kraxel@redhat.com
>> ---
>> hw/display/cirrus_vga.c | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c index
>> 6ffe64f..326d511 100644
>> --- a/hw/display/cirrus_vga.c
>> +++ b/hw/display/cirrus_vga.c
>> @@ -205,6 +205,7 @@ typedef struct CirrusVGAState {
>> uint32_t cirrus_bank_base[2];
>> uint32_t cirrus_bank_limit[2];
>> uint8_t cirrus_hidden_palette[48];
>> + bool enable_blitter;
>> int cirrus_blt_pixelwidth;
>> int cirrus_blt_width;
>> int cirrus_blt_height;
>> @@ -960,6 +961,10 @@ static void cirrus_bitblt_start(CirrusVGAState * s) {
>> uint8_t blt_rop;
>>
>> + if (!s->enable_blitter) {
>> + goto bitblt_ignore;
>> + }
>> +
>> s->vga.gr[0x31] |= CIRRUS_BLT_BUSY;
>>
>> s->cirrus_blt_width = (s->vga.gr[0x20] | (s->vga.gr[0x21] << 8)) + 1; @@
>> -3024,6 +3029,8 @@ static void isa_cirrus_vga_realizefn(DeviceState *dev,
>> Error **errp) static Property isa_cirrus_vga_properties[] = {
>> DEFINE_PROP_UINT32("vgamem_mb", struct ISACirrusVGAState,
>> cirrus_vga.vga.vram_size_mb, 4),
>> + DEFINE_PROP_BOOL("blitter", struct ISACirrusVGAState,
>> + cirrus_vga.enable_blitter, true),
>> DEFINE_PROP_END_OF_LIST(),
>> };
>>
>> @@ -3093,6 +3100,8 @@ static void pci_cirrus_vga_realize(PCIDevice *dev,
>> Error **errp) static Property pci_vga_cirrus_properties[] = {
>> DEFINE_PROP_UINT32("vgamem_mb", struct PCICirrusVGAState,
>> cirrus_vga.vga.vram_size_mb, 4),
>> + DEFINE_PROP_BOOL("blitter", struct PCICirrusVGAState,
>> + cirrus_vga.enable_blitter, true),
>
> The default is 'ENABLE'? I think there should be 'false'.
I think it has to be enabled at least for the older machine types -
otherwise you change the hardware of guests during migration.
Thomas
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PULL for-2.9 4/7] cirrus: add option to disable blitter
2017-03-16 9:51 ` 李强
2017-03-16 11:07 ` Thomas Huth
@ 2017-03-16 14:00 ` Gerd Hoffmann
1 sibling, 0 replies; 12+ messages in thread
From: Gerd Hoffmann @ 2017-03-16 14:00 UTC (permalink / raw)
To: 李强; +Cc: qemu-devel
Hi,
> > + DEFINE_PROP_BOOL("blitter", struct PCICirrusVGAState,
>
> > + cirrus_vga.enable_blitter, true),
>
>
> The default is 'ENABLE'?
Yes.
> I think there should be 'false'.
Just flipping it to false isn't an option. At minimum we would have to
keep it enabled by default for older machine types, for backward
compatibility with older qemu versions.
Beside that I don't want rush it. When flipping the default I'd do it
early in the 2.10 development cycle, so we have a few months to the next
release to see what the fallout is and decide which default we want.
But I think the most sensible approach would be to have upper management
layer handle this. We have libosinfo, which records capabilities of
guests, and we could add a field there telling whenever the guest os in
question uses the blitter (and therefore breaks when we turn it off) or
whenever it is safe to turn it off.
cheers,
Gerd
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PULL for-2.9 0/7] cirrus: more blitter security fixes.
2017-03-16 9:30 [Qemu-devel] [PULL for-2.9 0/7] cirrus: more blitter security fixes Gerd Hoffmann
` (6 preceding siblings ...)
2017-03-16 9:30 ` [Qemu-devel] [PULL for-2.9 7/7] cirrus: stop passing around src " Gerd Hoffmann
@ 2017-03-16 17:53 ` Peter Maydell
7 siblings, 0 replies; 12+ messages in thread
From: Peter Maydell @ 2017-03-16 17:53 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: QEMU Developers
On 16 March 2017 at 09:30, Gerd Hoffmann <kraxel@redhat.com> wrote:
> Hi,
>
> Another pile of cirrus blitter fixes, including cve fixes for known
> issues, so clearly 2.9 material.
>
> Patches 6+7 implement a new approach to blitter memory access sanity
> checking. We pass around offsets not pointers, and at the place where
> the actual memory access happens we mask the offset to the valid
> range before calculating the pointer.
>
> That should put an end to security holes due to blit_is_unsafe() sanity
> checks failing to calculate some special case correctly, or due to
> blit_is_unsafe() calls missing, and kill any dragons which might still
> be lurking in the code. In theory this even obsoletes blit_is_unsafe(),
> but I don't feel like ripping it out right away ...
>
> please pull,
> Gerd
>
> The following changes since commit 1883ff34b540daacae948f493b0ba525edf5f642:
>
> Merge remote-tracking branch 'remotes/mst/tags/for_upstream' into staging (2017-03-15 18:44:05 +0000)
>
> are available in the git repository at:
>
>
> git://git.kraxel.org/qemu tags/pull-cirrus-20170316-1
>
> for you to fetch changes up to ffaf857778286ca54e3804432a2369a279e73aa7:
>
> cirrus: stop passing around src pointers in the blitter (2017-03-16 08:58:16 +0100)
>
> ----------------------------------------------------------------
> cirrus: blitter fixes.
>
Applied, thanks.
-- PMM
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-03-16 17:53 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-16 9:30 [Qemu-devel] [PULL for-2.9 0/7] cirrus: more blitter security fixes Gerd Hoffmann
2017-03-16 9:30 ` [Qemu-devel] [PULL for-2.9 1/7] fix :cirrus_vga fix OOB read case qemu Segmentation fault Gerd Hoffmann
2017-03-16 9:30 ` [Qemu-devel] [PULL for-2.9 2/7] cirrus/vnc: zap bitblit support from console code Gerd Hoffmann
2017-03-16 9:30 ` [Qemu-devel] [PULL for-2.9 3/7] cirrus: switch to 4 MB video memory by default Gerd Hoffmann
2017-03-16 9:30 ` [Qemu-devel] [PULL for-2.9 4/7] cirrus: add option to disable blitter Gerd Hoffmann
2017-03-16 9:51 ` 李强
2017-03-16 11:07 ` Thomas Huth
2017-03-16 14:00 ` Gerd Hoffmann
2017-03-16 9:30 ` [Qemu-devel] [PULL for-2.9 5/7] cirrus: fix cirrus_invalidate_region Gerd Hoffmann
2017-03-16 9:30 ` [Qemu-devel] [PULL for-2.9 6/7] cirrus: stop passing around dst pointers in the blitter Gerd Hoffmann
2017-03-16 9:30 ` [Qemu-devel] [PULL for-2.9 7/7] cirrus: stop passing around src " Gerd Hoffmann
2017-03-16 17:53 ` [Qemu-devel] [PULL for-2.9 0/7] cirrus: more blitter security fixes Peter Maydell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.