From 6c7565028c272c4c6f2a83c3a90b044eeaf2804a Mon Sep 17 00:00:00 2001 From: Christophe Fergeau Date: Fri, 28 Oct 2016 16:48:40 +0200 Subject: [PATCH 1/4] qxl: Only emit QXL_INTERRUPT_CLIENT_MONITORS_CONFIG on config changes Currently if the client keeps sending the same monitor config to QEMU/spice-server, QEMU will always raise a QXL_INTERRUPT_CLIENT_MONITORS_CONFIG regardless of whether there was a change or not. Guest-side (with fedora 25), the kernel QXL KMS driver will also forward the event to user-space without checking if there were actual changes. Next in line are gnome-shell/mutter (on a default f25 install), which will try to reconfigure everything without checking if there is anything to do. Where this gets ugly is that when applying the resolution changes, gnome-shell/mutter will call drmModeRmFB, drmModeAddFB, and drmModeSetCrtc, which will cause the primary surface to be destroyed and recreated by the QXL KMS driver. This in turn will cause the client to resend a client monitors config message, which will cause QEMU to reemit an interrupt with an unchanged monitors configuration, ... This causes https://bugzilla.redhat.com/show_bug.cgi?id=1266484 This commit makes sure that we only emit QXL_INTERRUPT_CLIENT_MONITORS_CONFIG when there are actual configuration changes the guest should act on. Signed-off-by: Christophe Fergeau Message-id: 20161028144840.18326-1-cfergeau@redhat.com Signed-off-by: Gerd Hoffmann --- hw/display/qxl.c | 37 ++++++++++++++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/hw/display/qxl.c b/hw/display/qxl.c index 0e2682d28b..62d0c80dcf 100644 --- a/hw/display/qxl.c +++ b/hw/display/qxl.c @@ -992,6 +992,34 @@ static uint32_t qxl_crc32(const uint8_t *p, unsigned len) return crc32(0xffffffff, p, len) ^ 0xffffffff; } +static bool qxl_rom_monitors_config_changed(QXLRom *rom, + VDAgentMonitorsConfig *monitors_config, + unsigned int max_outputs) +{ + int i; + unsigned int monitors_count; + + monitors_count = MIN(monitors_config->num_of_monitors, max_outputs); + + if (rom->client_monitors_config.count != monitors_count) { + return true; + } + + for (i = 0 ; i < rom->client_monitors_config.count ; ++i) { + VDAgentMonConfig *monitor = &monitors_config->monitors[i]; + QXLURect *rect = &rom->client_monitors_config.heads[i]; + /* monitor->depth ignored */ + if ((rect->left != monitor->x) || + (rect->top != monitor->y) || + (rect->right != monitor->x + monitor->width) || + (rect->bottom != monitor->y + monitor->height)) { + return true; + } + } + + return false; +} + /* called from main context only */ static int interface_client_monitors_config(QXLInstance *sin, VDAgentMonitorsConfig *monitors_config) @@ -1000,6 +1028,7 @@ static int interface_client_monitors_config(QXLInstance *sin, QXLRom *rom = memory_region_get_ram_ptr(&qxl->rom_bar); int i; unsigned max_outputs = ARRAY_SIZE(rom->client_monitors_config.heads); + bool config_changed = false; if (qxl->revision < 4) { trace_qxl_client_monitors_config_unsupported_by_device(qxl->id, @@ -1030,6 +1059,10 @@ static int interface_client_monitors_config(QXLInstance *sin, } #endif + config_changed = qxl_rom_monitors_config_changed(rom, + monitors_config, + max_outputs); + memset(&rom->client_monitors_config, 0, sizeof(rom->client_monitors_config)); rom->client_monitors_config.count = monitors_config->num_of_monitors; @@ -1059,7 +1092,9 @@ static int interface_client_monitors_config(QXLInstance *sin, trace_qxl_interrupt_client_monitors_config(qxl->id, rom->client_monitors_config.count, rom->client_monitors_config.heads); - qxl_send_events(qxl, QXL_INTERRUPT_CLIENT_MONITORS_CONFIG); + if (config_changed) { + qxl_send_events(qxl, QXL_INTERRUPT_CLIENT_MONITORS_CONFIG); + } return 1; } From 42a8dadc74f8982fc269e54e3c5627b54d9f83d8 Mon Sep 17 00:00:00 2001 From: Li Qiang Date: Tue, 1 Nov 2016 02:53:11 -0700 Subject: [PATCH 2/4] virtio-gpu: fix information leak in getting capset info dispatch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In virgl_cmd_get_capset_info dispatch function, the 'resp' hasn't been full initialized before writing to the guest. This will leak the 'resp.padding' and 'resp.hdr.padding' fieds to the guest. This patch fix this issue. Signed-off-by: Li Qiang Message-id: 5818661e.0860240a.77264.7a56@mx.google.com Reviewed-by: Marc-André Lureau Signed-off-by: Gerd Hoffmann --- hw/display/virtio-gpu-3d.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/display/virtio-gpu-3d.c b/hw/display/virtio-gpu-3d.c index 758d33a09d..23f39de94d 100644 --- a/hw/display/virtio-gpu-3d.c +++ b/hw/display/virtio-gpu-3d.c @@ -347,6 +347,7 @@ static void virgl_cmd_get_capset_info(VirtIOGPU *g, VIRTIO_GPU_FILL_CMD(info); + memset(&resp, 0, sizeof(resp)); if (info.capset_index == 0) { resp.capset_id = VIRTIO_GPU_CAPSET_VIRGL; virgl_renderer_get_cap_set(resp.capset_id, From 2d1cd6c7a91a4beb99a0c3a21be529222a708545 Mon Sep 17 00:00:00 2001 From: Li Qiang Date: Tue, 1 Nov 2016 04:06:58 -0700 Subject: [PATCH 3/4] virtio-gpu: fix memory leak in update_cursor_data_virgl MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In update_cursor_data_virgl function, if the 'width'/ 'height' is not equal to current cursor's width/height it will return without free the 'data' allocated previously. This will lead a memory leak issue. This patch fix this issue. Signed-off-by: Li Qiang Message-id: 58187760.41d71c0a.cca75.4cb9@mx.google.com Reviewed-by: Marc-André Lureau Signed-off-by: Gerd Hoffmann --- hw/display/virtio-gpu.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c index 60bce94d6b..5f32e1aae9 100644 --- a/hw/display/virtio-gpu.c +++ b/hw/display/virtio-gpu.c @@ -84,6 +84,7 @@ static void update_cursor_data_virgl(VirtIOGPU *g, if (width != s->current_cursor->width || height != s->current_cursor->height) { + free(data); return; } From 4299b90e9ba9ce5ca9024572804ba751aa1a7e70 Mon Sep 17 00:00:00 2001 From: Prasad J Pandit Date: Tue, 18 Oct 2016 13:15:17 +0530 Subject: [PATCH 4/4] display: cirrus: check vga bits per pixel(bpp) value In Cirrus CLGD 54xx VGA Emulator, if cirrus graphics mode is VGA, 'cirrus_get_bpp' returns zero(0), which could lead to a divide by zero error in while copying pixel data. The same could occur via blit pitch values. Add check to avoid it. Reported-by: Huawei PSIRT Signed-off-by: Prasad J Pandit Message-id: 1476776717-24807-1-git-send-email-ppandit@redhat.com Signed-off-by: Gerd Hoffmann --- hw/display/cirrus_vga.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c index 3d712d592f..bdb092ee9d 100644 --- a/hw/display/cirrus_vga.c +++ b/hw/display/cirrus_vga.c @@ -272,6 +272,9 @@ static void cirrus_update_memory_access(CirrusVGAState *s); static bool blit_region_is_unsafe(struct CirrusVGAState *s, int32_t pitch, int32_t addr) { + if (!pitch) { + return true; + } if (pitch < 0) { int64_t min = addr + ((int64_t)s->cirrus_blt_height-1) * pitch; @@ -715,7 +718,7 @@ static int cirrus_bitblt_videotovideo_patterncopy(CirrusVGAState * s) s->cirrus_addr_mask)); } -static void cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h) +static int cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h) { int sx = 0, sy = 0; int dx = 0, dy = 0; @@ -729,6 +732,9 @@ static void cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h) int width, height; depth = s->vga.get_bpp(&s->vga) / 8; + if (!depth) { + return 0; + } s->vga.get_resolution(&s->vga, &width, &height); /* extra x, y */ @@ -783,6 +789,8 @@ static void cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h) cirrus_invalidate_region(s, s->cirrus_blt_dstaddr, s->cirrus_blt_dstpitch, s->cirrus_blt_width, s->cirrus_blt_height); + + return 1; } static int cirrus_bitblt_videotovideo_copy(CirrusVGAState * s) @@ -790,11 +798,9 @@ static int cirrus_bitblt_videotovideo_copy(CirrusVGAState * s) if (blit_is_unsafe(s)) return 0; - cirrus_do_copy(s, s->cirrus_blt_dstaddr - s->vga.start_addr, + return cirrus_do_copy(s, s->cirrus_blt_dstaddr - s->vga.start_addr, s->cirrus_blt_srcaddr - s->vga.start_addr, s->cirrus_blt_width, s->cirrus_blt_height); - - return 1; } /***************************************