[Qemu-devel] [Patch] Segfault with -vnc option

[Qemu-devel] [Patch] Segfault with -vnc option

[Jan Niehusmann]

Hi!

I observed a reproducible segmentation fault of kvm under the following
conditions:

 - start kvm with -vnc option, using cirrus vga (default)
 - connect to the vnc console
 - boot windows xp
 - login
 - screen resolutions 1024x768
 - change to qemu monitor (ctrl-alt-2)
 - wait for the windows screensaver to kick in

Here is the stack trace of the segmentation fault:

(gdb) info stack
#0  0xb7cc4573 in memmove () from /lib/i686/cmov/libc.so.6
#1  0x080db22a in vnc_copy (ds=0x81d0b20, src_x=50, src_y=50, dst_x=195, dst_y=576, w=275, h=174) at vnc.c:477
#2  0x080b2626 in cirrus_bitblt_start (s=0x86eb1e4) at /tmp/kvm-72+dfsg/qemu/hw/cirrus_vga.c:780
#3  0x080b29e5 in cirrus_hook_write_gr (s=0x86eb1e4, reg_index=0, reg_value=275) at /tmp/kvm-72+dfsg/qemu/hw/cirrus_vga.c:1091
#4  0x080fe885 in cpu_physical_memory_rw (addr=753728, buf=0x5a408028 <Address 0x5a408028 out of bounds>, len=1, is_write=1) at /tmp/kvm-72+dfsg/qemu/exec.c:2800
#5  0x08159eae in kvm_mmio_write (opaque=0x0, addr=753728, data=0x5a408028 <Address 0x5a408028 out of bounds>, len=1) at /tmp/kvm-72+dfsg/qemu/qemu-kvm.c:690
#6  0x0818d30b in handle_mmio (kvm=0x83ec028, kvm_run=0x5a408000) at libkvm.c:849
#7  0x0818d820 in kvm_run (kvm=0x83ec028, vcpu=0) at libkvm.c:975
#8  0x0815a680 in kvm_cpu_exec (env=0x84da9b0) at /tmp/kvm-72+dfsg/qemu/qemu-kvm.c:218
#9  0x0815a980 in ap_main_loop (_env=0x84da9b0) at /tmp/kvm-72+dfsg/qemu/qemu-kvm.c:407
#10 0xb7e964c0 in start_thread () from /lib/i686/cmov/libpthread.so.0
#11 0xb7d2d55e in clone () from /lib/i686/cmov/libc.so.6

The cause is that the default windows screensaver uses bitblt to
move the windows logo around, and cirrus_bitblt_start does call vnc_copy
with parameters suitable for the VGA mode the virtual VGA card uses. But
vnc_copy uses the currently active console, which is a text console with
a resolution of 800x600:

(gdb) up
#1  0x080db22a in vnc_copy (ds=0x81d0b20, src_x=50, src_y=50, dst_x=195, dst_y=576, w=275, h=174) at vnc.c:477
477             memmove(old_row, src_row, w * vs->depth);

As you can see, dst_y + h = 750 > 600.

As vnc_copy doesn't check these parameters, a segmentation fault occurs.

I think it's quite easy to prevent this behaviour: If we are not on the
graphics console, just skip the vnc_copy.

This patch assumes that there is only one graphic console. This is,
of course, the usual case - but I'm not sure if this is guaranteed in
every case.

Regards,
Jan


Signed-off-by: Jan Niehusmann <jan@gondor.com>

--- qemu/vnc.c.orig	2008-09-22 18:38:08.000000000 +0200
+++ qemu/vnc.c	2008-09-22 18:39:13.000000000 +0200
@@ -457,6 +457,9 @@
     int pitch = ds->linesize;
     VncState *vs = ds->opaque;
 
+    /* Skip copy when on text console */
+    if(!is_graphic_console()) return;
+
     vnc_update_client(vs);
 
     if (dst_y > src_y) {

[Qemu-devel] Re: [Patch] Segfault with -vnc option

[Anthony Liguori]

Jan Niehusmann wrote:
> Hi!
>   

Hi Jan,

Very good catch.  My only suggestion would be to move this check into 
cirrus_vga.c and vmware_vga.c.  Even better would be to introduce a 
wrapper around callers of dpy_copy.

Regards,

Anthony Liguori

> Signed-off-by: Jan Niehusmann <jan@gondor.com>
>
> --- qemu/vnc.c.orig	2008-09-22 18:38:08.000000000 +0200
> +++ qemu/vnc.c	2008-09-22 18:39:13.000000000 +0200
> @@ -457,6 +457,9 @@
>      int pitch = ds->linesize;
>      VncState *vs = ds->opaque;
>  
> +    /* Skip copy when on text console */
> +    if(!is_graphic_console()) return;
> +
>      vnc_update_client(vs);
>  
>      if (dst_y > src_y) {
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   

Re: [Qemu-devel] Re: [Patch] Segfault with -vnc option

[andrzej zaborowski]

2008/9/22 Anthony Liguori <anthony@codemonkey.ws>:
> Jan Niehusmann wrote:
>>
>> Hi!
>>
>
> Hi Jan,
>
> Very good catch.  My only suggestion would be to move this check into
> cirrus_vga.c and vmware_vga.c.  Even better would be to introduce a wrapper
> around callers of dpy_copy.

Yes, I don't think hw/ code should be concerned with what console is
active.  Logically the dpy_ functions should take the pointer returned
from graphic_console_init() as first parameter.

Please also check the code is formatted consistently with qemu.

I didn't receive Jan's message but the check seems to not be enough
because there can be multiple graphical consoles with different sizes
- if I'm guessing correctly what this patch tries to fix.
Cheers

[Qemu-devel] [PATCH] Wrapper around dpy_copy to fix segfault with -vnc option

[Jan Niehusmann]

On Mon, Sep 22, 2008 at 11:15:04PM +0200, andrzej zaborowski wrote:
> Yes, I don't think hw/ code should be concerned with what console is
> active.  Logically the dpy_ functions should take the pointer returned
> from graphic_console_init() as first parameter.
> 
> Please also check the code is formatted consistently with qemu.
> 
> I didn't receive Jan's message but the check seems to not be enough
> because there can be multiple graphical consoles with different sizes
> - if I'm guessing correctly what this patch tries to fix.

Based on these comments I had another look at the code. If there can
be multiple graphical consoles, the only sensible test is 'console
== active_console' where console must be provided by the caller. So,
indeed, a pointer to the console must be provided instead of a pointer
to the DisplayState.

To make function names consistent, I called the function qemu_console_copy
in analogy to qemu_console_resize (which is a similar wrapper around
dpy_resize).


Signed-off-by: Jan Niehusmann <jan@gondor.com>


diff --git a/qemu/console.c b/qemu/console.c
index 785710a..1f4907c 100644
--- a/qemu/console.c
+++ b/qemu/console.c
@@ -1343,3 +1343,10 @@ void qemu_console_resize(QEMUConsole *console, int width, int height)
         }
     }
 }
+
+void qemu_console_copy(QEMUConsole *console, int src_x, int src_y, int dst_x, int dst_y, int w, int h) {
+    if ( active_console == console && console->ds->dpy_copy ) {
+        console->ds->dpy_copy(console->ds, src_x, src_y, dst_x, dst_y, w, h);
+    }
+}
+
diff --git a/qemu/console.h b/qemu/console.h
index 7b8571f..05e7e64 100644
--- a/qemu/console.h
+++ b/qemu/console.h
@@ -140,6 +140,7 @@ CharDriverState *text_console_init(DisplayState *ds, const char *p);
 void console_select(unsigned int index);
 void console_color_init(DisplayState *ds);
 void qemu_console_resize(QEMUConsole *console, int width, int height);
+void qemu_console_copy(QEMUConsole *console, int src_x, int src_y, int dst_x, int dst_y, int w, int h);
 
 /* sdl.c */
 void sdl_display_init(DisplayState *ds, int full_screen, int no_frame);
diff --git a/qemu/hw/cirrus_vga.c b/qemu/hw/cirrus_vga.c
index 0cf5b24..4f3aef9 100644
--- a/qemu/hw/cirrus_vga.c
+++ b/qemu/hw/cirrus_vga.c
@@ -775,13 +775,13 @@ static void cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h)
 		      s->cirrus_blt_width, s->cirrus_blt_height);
 
     if (notify)
-	s->ds->dpy_copy(s->ds,
-			sx, sy, dx, dy,
-			s->cirrus_blt_width / depth,
-			s->cirrus_blt_height);
+	qemu_console_copy(s->console,
+			  sx, sy, dx, dy,
+			  s->cirrus_blt_width / depth,
+			  s->cirrus_blt_height);
 
     /* we don't have to notify the display that this portion has
-       changed since dpy_copy implies this */
+       changed since qemu_console_copy implies this */
 
     if (!notify)
 	cirrus_invalidate_region(s, s->cirrus_blt_dstaddr,
diff --git a/qemu/hw/vmware_vga.c b/qemu/hw/vmware_vga.c
index efbcd96..625cd83 100644
--- a/qemu/hw/vmware_vga.c
+++ b/qemu/hw/vmware_vga.c
@@ -384,7 +384,7 @@ static inline void vmsvga_copy_rect(struct vmsvga_state_s *s,
 
 # ifdef DIRECT_VRAM
     if (s->ds->dpy_copy)
-        s->ds->dpy_copy(s->ds, x0, y0, x1, y1, w, h);
+        qemu_console_copy(s->console, x0, y0, x1, y1, w, h);
     else
 # endif
     {

[Qemu-devel] Re: [PATCH] Wrapper around dpy_copy to fix segfault with -vnc option

[andrzej zaborowski]

2008/9/23 Jan Niehusmann <jan@gondor.com>:
> On Mon, Sep 22, 2008 at 11:15:04PM +0200, andrzej zaborowski wrote:
>> Yes, I don't think hw/ code should be concerned with what console is
>> active.  Logically the dpy_ functions should take the pointer returned
>> from graphic_console_init() as first parameter.
>>
>> Please also check the code is formatted consistently with qemu.
>>
>> I didn't receive Jan's message but the check seems to not be enough
>> because there can be multiple graphical consoles with different sizes
>> - if I'm guessing correctly what this patch tries to fix.
>
> Based on these comments I had another look at the code. If there can
> be multiple graphical consoles, the only sensible test is 'console
> == active_console' where console must be provided by the caller. So,
> indeed, a pointer to the console must be provided instead of a pointer
> to the DisplayState.
>
> To make function names consistent, I called the function qemu_console_copy
> in analogy to qemu_console_resize (which is a similar wrapper around
> dpy_resize).

I committed the patch slightly modified because I found this still
doesn't account for all the cases.  Imagine ds->dpy_copy is not set,
the call does nothing and the screen is not updated until fully
invalidated.  We need to either implement a generic dpy_copy in
console.c or have a fallback in hw/cirrus_vga.c depending on which is
faster.

Also note that till now hw/ files could only call into the ds->dpy_
functions from inside their own vga_hw_update callback, this
guaranteed some consistency.  The use of dpy_copy inside cirrus_vga.c
changed this which is the source of these bugs.  I hadn't noticed when
this happened.

Regards