[Qemu-devel] [RFC] fix crashes with vmware driver

[Qemu-devel] [RFC] fix crashes with vmware driver

[Serge Hallyn]

Hi,

I don't know where the best place to catch this would be, but
with vnc and vmware_vga it's possible to get set_bit called on
a negative index, crashing qemu.  See

https://bugs.launchpad.net/ubuntu/+source/qemu-kvm/+bug/918791

for details.  This patch prevents that.  It's possible this
should be caught earlier, but this patch works for me.

Signed-off-by: Serge Hallyn <serge.hallyn@canonical.com>
---
 vmware_vga.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Index: qemu-kvm-1.0+noroms/hw/vmware_vga.c
===================================================================
--- qemu-kvm-1.0+noroms.orig/hw/vmware_vga.c	2012-03-01 16:19:23.280571798 -0600
+++ qemu-kvm-1.0+noroms/hw/vmware_vga.c	2012-03-01 16:27:27.910975006 -0600
@@ -298,6 +298,22 @@
     uint8_t *src;
     uint8_t *dst;
 
+    if (x < 0) {
+        fprintf(stderr, "%s: update x was < 0 (%d, w %d)\n",
+                        __FUNCTION__, x, w);
+        w += x;
+	if (w < 0)
+		return;
+        x = 0;
+    }
+    if (y < 0) {
+        fprintf(stderr, "%s: update y was < 0 (%d, h %d)\n",
+                        __FUNCTION__, y, h);
+        h += y;
+	if (h < 0)
+		return;
+        y = 0;
+    }
     if (x + w > s->width) {
         fprintf(stderr, "%s: update width too large x: %d, w: %d\n",
                         __FUNCTION__, x, w);

Re: [Qemu-devel] [RFC] fix crashes with vmware driver

[Ryan Harper]

* Serge Hallyn <serge.hallyn@canonical.com> [2012-03-02 15:13]:
> Hi,
> 
> I don't know where the best place to catch this would be, but
> with vnc and vmware_vga it's possible to get set_bit called on
> a negative index, crashing qemu.  See
> 
> https://bugs.launchpad.net/ubuntu/+source/qemu-kvm/+bug/918791
> 
> for details.  This patch prevents that.  It's possible this
> should be caught earlier, but this patch works for me.
> 
> Signed-off-by: Serge Hallyn <serge.hallyn@canonical.com>
> ---
>  vmware_vga.c |   16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> Index: qemu-kvm-1.0+noroms/hw/vmware_vga.c
> ===================================================================
> --- qemu-kvm-1.0+noroms.orig/hw/vmware_vga.c	2012-03-01 16:19:23.280571798 -0600
> +++ qemu-kvm-1.0+noroms/hw/vmware_vga.c	2012-03-01 16:27:27.910975006 -0600
> @@ -298,6 +298,22 @@
>      uint8_t *src;
>      uint8_t *dst;
> 
> +    if (x < 0) {
> +        fprintf(stderr, "%s: update x was < 0 (%d, w %d)\n",
> +                        __FUNCTION__, x, w);
> +        w += x;
> +	if (w < 0)
> +		return;
> +        x = 0;
> +    }
> +    if (y < 0) {
> +        fprintf(stderr, "%s: update y was < 0 (%d, h %d)\n",
> +                        __FUNCTION__, y, h);
> +        h += y;
> +	if (h < 0)
> +		return;
> +        y = 0;
> +    }

Looks like it has mixed spaces and tabs.

CODING_STYLE wants {} on all if's 



>      if (x + w > s->width) {
>          fprintf(stderr, "%s: update width too large x: %d, w: %d\n",
>                          __FUNCTION__, x, w);

-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
ryanh@us.ibm.com

Re: [Qemu-devel] [RFC] fix crashes with vmware driver

[Serge E. Hallyn]

Thanks.  As there's been no substantial feedback, I'll resend with
those changes.

-serge

Quoting Ryan Harper (ryanh@us.ibm.com):
> * Serge Hallyn <serge.hallyn@canonical.com> [2012-03-02 15:13]:
> > Hi,
> > 
> > I don't know where the best place to catch this would be, but
> > with vnc and vmware_vga it's possible to get set_bit called on
> > a negative index, crashing qemu.  See
> > 
> > https://bugs.launchpad.net/ubuntu/+source/qemu-kvm/+bug/918791
> > 
> > for details.  This patch prevents that.  It's possible this
> > should be caught earlier, but this patch works for me.
> > 
> > Signed-off-by: Serge Hallyn <serge.hallyn@canonical.com>
> > ---
> >  vmware_vga.c |   16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > Index: qemu-kvm-1.0+noroms/hw/vmware_vga.c
> > ===================================================================
> > --- qemu-kvm-1.0+noroms.orig/hw/vmware_vga.c	2012-03-01 16:19:23.280571798 -0600
> > +++ qemu-kvm-1.0+noroms/hw/vmware_vga.c	2012-03-01 16:27:27.910975006 -0600
> > @@ -298,6 +298,22 @@
> >      uint8_t *src;
> >      uint8_t *dst;
> > 
> > +    if (x < 0) {
> > +        fprintf(stderr, "%s: update x was < 0 (%d, w %d)\n",
> > +                        __FUNCTION__, x, w);
> > +        w += x;
> > +	if (w < 0)
> > +		return;
> > +        x = 0;
> > +    }
> > +    if (y < 0) {
> > +        fprintf(stderr, "%s: update y was < 0 (%d, h %d)\n",
> > +                        __FUNCTION__, y, h);
> > +        h += y;
> > +	if (h < 0)
> > +		return;
> > +        y = 0;
> > +    }
> 
> Looks like it has mixed spaces and tabs.
> 
> CODING_STYLE wants {} on all if's 
> 
> 
> 
> >      if (x + w > s->width) {
> >          fprintf(stderr, "%s: update width too large x: %d, w: %d\n",
> >                          __FUNCTION__, x, w);
> 
> -- 
> Ryan Harper
> Software Engineer; Linux Technology Center
> IBM Corp., Austin, Tx
> ryanh@us.ibm.com
> 

[Qemu-devel] [PATCH 1/1] vmware_vga: stop crashing

[Serge Hallyn]

if x or y < 0, set them to 0 (and decrement width/height accordingly)>

I don't know where the best place to catch this would be, but
with vnc and vmware_vga it's possible to get set_bit called on
a negative index, crashing qemu.  See

https://bugs.launchpad.net/ubuntu/+source/qemu-kvm/+bug/918791

for details.  This patch prevents that.  It's possible this
should be caught earlier, but this patch works for me.

Changelog:
	Mar 5: As Ryan Harper pointed out, don't mix tabs+spaces,
	       and put {} around all conditionals.

Signed-off-by: Serge Hallyn <serge.hallyn@canonical.com>
---
 hw/vmware_vga.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index 142d9f4..c94f9f3 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -298,6 +298,24 @@ static inline void vmsvga_update_rect(struct vmsvga_state_s *s,
     uint8_t *src;
     uint8_t *dst;
 
+    if (x < 0) {
+        fprintf(stderr, "%s: update x was < 0 (%d, w %d)\n",
+                        __FUNCTION__, x, w);
+        w += x;
+        if (w < 0) {
+            return;
+        }
+        x = 0;
+    }
+    if (y < 0) {
+        fprintf(stderr, "%s: update y was < 0 (%d, h %d)\n",
+                        __FUNCTION__, y, h);
+        h += y;
+        if (h < 0) {
+            return;
+        }
+        y = 0;
+    }
     if (x + w > s->width) {
         fprintf(stderr, "%s: update width too large x: %d, w: %d\n",
                         __FUNCTION__, x, w);
-- 
1.7.9

Re: [Qemu-devel] [PATCH 1/1] vmware_vga: stop crashing

[Gerhard Wiesinger]

Can confirm that this patch fixes a crash which also occoured here. Since 
window was out of the VNC window, crash was reproduceable and has been 
removed reproduceable.

Tested-by: Gerhard Wiesinger <lists@wiesinger.com>

Please apply ASAP.

Ciao,
Gerhard

--
http://www.wiesinger.com/


On Mon, 5 Mar 2012, Serge Hallyn wrote:

>
> if x or y < 0, set them to 0 (and decrement width/height accordingly)>
>
> I don't know where the best place to catch this would be, but
> with vnc and vmware_vga it's possible to get set_bit called on
> a negative index, crashing qemu.  See
>
> https://bugs.launchpad.net/ubuntu/+source/qemu-kvm/+bug/918791
>
> for details.  This patch prevents that.  It's possible this
> should be caught earlier, but this patch works for me.
>
> Changelog:
> 	Mar 5: As Ryan Harper pointed out, don't mix tabs+spaces,
> 	       and put {} around all conditionals.
>
> Signed-off-by: Serge Hallyn <serge.hallyn@canonical.com>
> ---
> hw/vmware_vga.c |   18 ++++++++++++++++++
> 1 files changed, 18 insertions(+), 0 deletions(-)
>
> diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
> index 142d9f4..c94f9f3 100644
> --- a/hw/vmware_vga.c
> +++ b/hw/vmware_vga.c
> @@ -298,6 +298,24 @@ static inline void vmsvga_update_rect(struct vmsvga_state_s *s,
>     uint8_t *src;
>     uint8_t *dst;
>
> +    if (x < 0) {
> +        fprintf(stderr, "%s: update x was < 0 (%d, w %d)\n",
> +                        __FUNCTION__, x, w);
> +        w += x;
> +        if (w < 0) {
> +            return;
> +        }
> +        x = 0;
> +    }
> +    if (y < 0) {
> +        fprintf(stderr, "%s: update y was < 0 (%d, h %d)\n",
> +                        __FUNCTION__, y, h);
> +        h += y;
> +        if (h < 0) {
> +            return;
> +        }
> +        y = 0;
> +    }
>     if (x + w > s->width) {
>         fprintf(stderr, "%s: update width too large x: %d, w: %d\n",
>                         __FUNCTION__, x, w);
> -- 
> 1.7.9
>
>
>