Patchwork [v2] vmware_vga: fix out of bounds and invalid rects updating

login
register
mail settings
Submitter Michael Tokarev
Date Jan. 25, 2013, 5:23 p.m.
Message ID <1359134604-27516-2-git-send-email-mjt@msgid.tls.msk.ru>
Download mbox | patch
Permalink /patch/215816/
State New
Headers show

Comments

Michael Tokarev - Jan. 25, 2013, 5:23 p.m.
This is a follow up for several attempts to fix this issue.

Previous incarnations:

1. http://thread.gmane.org/gmane.linux.ubuntu.bugs.general/3156089
https://bugs.launchpad.net/bugs/918791
"qemu-kvm dies when using vmvga driver and unity in the guest" bug.
Fix by Serge Hallyn:
 https://launchpadlibrarian.net/94916786/qemu-vmware.debdiff
This fix is incomplete, since it does not check width and height
for being negative.  Serge weren't sure if that's the right place
to fix it, maybe the fix should be up the stack somewhere.

2. http://thread.gmane.org/gmane.comp.emulators.qemu/166064
by Marek Vasut: "vmware_vga: Redraw only visible area"

This one adds the (incomplete) check to vmsvga_update_rect_delayed(),
the routine just queues the rect updating but does no interesting
stuff.  It is also incomplete in the same way as patch by Serge,
but also does not touch width&height at all after adjusting x&y,
which is wrong.

As far as I can see, when processing guest requests, the device
places them into a queue (vmsvga_update_rect_delayed()) and
processes this queue in different place/time, namely, in
vmsvga_update_rect().  Sometimes, vmsvga_update_rect() is
called directly, without placing the request to the gueue.
This is the place this patch changes, which is the last
(deepest) in the stack.  I'm not sure if this is the right
place still, since it is possible we have some queue optimization
(or may have in the future) which will be upset by negative/wrong
values here, so maybe we should check for validity of input
right when receiving request from the guest (and maybe even
use unsigned types there).  But I don't know the protocol
and implementation enough to have a definitive answer.

But since vmsvga_update_rect() has other sanity checks already,
I'm adding the missing ones there as well.

Cc'ing BALATON Zoltan and Andrzej Zaborowski who shows in `git blame'
output and may know something in this area.

If this patch is accepted, it should be applied to all active
stable branches (at least since 1.1, maybe even before), with
minor context change (ds_get_*(s->vga.ds) => s->*).  I'm not
Cc'ing -stable yet, will do it explicitly once the patch is
accepted.

BTW, these checks use fprintf(stderr) -- it should be converted
to something more appropriate, since stderr will most likely
disappear somewhere.

Cc: Marek Vasut <marex@denx.de>
CC: Serge Hallyn <serge.hallyn@ubuntu.com>
Cc: BALATON Zoltan <balaton@eik.bme.hu>
Cc: Andrzej Zaborowski <balrogg@gmail.com>
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
Reviewed-by: Marek Vasut <marex@denx.de>
Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com>
---
 hw/vmware_vga.c |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
BALATON Zoltan - Jan. 25, 2013, 6:03 p.m.
On Fri, 25 Jan 2013, Michael Tokarev wrote:
> But since vmsvga_update_rect() has other sanity checks already,
> I'm adding the missing ones there as well.
>
> Cc'ing BALATON Zoltan and Andrzej Zaborowski who shows in `git blame'
> output and may know something in this area.

I've proposed this patch before: http://patchwork.ozlabs.org/patch/197904/ 
but then thought that later commit 
4b4496dbccc5f286f0ef411f0ff702d67cb95145 fixed this and similar problems 
in other devices by clipping out of bounds updates further down the road 
so no further checks should be needed. There was some debate on where to 
put these checks but I'm not sure about the outcome so I'm not sure if we 
need more checks in devices now or not. I'm afraid I don't have more 
helpful information on this.

Regards,
BALATON Zoltan
Blue Swirl - Jan. 26, 2013, 3:09 p.m.
Thanks, applied.

On Fri, Jan 25, 2013 at 5:23 PM, Michael Tokarev <mjt@tls.msk.ru> wrote:
> This is a follow up for several attempts to fix this issue.
>
> Previous incarnations:
>
> 1. http://thread.gmane.org/gmane.linux.ubuntu.bugs.general/3156089
> https://bugs.launchpad.net/bugs/918791
> "qemu-kvm dies when using vmvga driver and unity in the guest" bug.
> Fix by Serge Hallyn:
>  https://launchpadlibrarian.net/94916786/qemu-vmware.debdiff
> This fix is incomplete, since it does not check width and height
> for being negative.  Serge weren't sure if that's the right place
> to fix it, maybe the fix should be up the stack somewhere.
>
> 2. http://thread.gmane.org/gmane.comp.emulators.qemu/166064
> by Marek Vasut: "vmware_vga: Redraw only visible area"
>
> This one adds the (incomplete) check to vmsvga_update_rect_delayed(),
> the routine just queues the rect updating but does no interesting
> stuff.  It is also incomplete in the same way as patch by Serge,
> but also does not touch width&height at all after adjusting x&y,
> which is wrong.
>
> As far as I can see, when processing guest requests, the device
> places them into a queue (vmsvga_update_rect_delayed()) and
> processes this queue in different place/time, namely, in
> vmsvga_update_rect().  Sometimes, vmsvga_update_rect() is
> called directly, without placing the request to the gueue.
> This is the place this patch changes, which is the last
> (deepest) in the stack.  I'm not sure if this is the right
> place still, since it is possible we have some queue optimization
> (or may have in the future) which will be upset by negative/wrong
> values here, so maybe we should check for validity of input
> right when receiving request from the guest (and maybe even
> use unsigned types there).  But I don't know the protocol
> and implementation enough to have a definitive answer.
>
> But since vmsvga_update_rect() has other sanity checks already,
> I'm adding the missing ones there as well.
>
> Cc'ing BALATON Zoltan and Andrzej Zaborowski who shows in `git blame'
> output and may know something in this area.
>
> If this patch is accepted, it should be applied to all active
> stable branches (at least since 1.1, maybe even before), with
> minor context change (ds_get_*(s->vga.ds) => s->*).  I'm not
> Cc'ing -stable yet, will do it explicitly once the patch is
> accepted.
>
> BTW, these checks use fprintf(stderr) -- it should be converted
> to something more appropriate, since stderr will most likely
> disappear somewhere.
>
> Cc: Marek Vasut <marex@denx.de>
> CC: Serge Hallyn <serge.hallyn@ubuntu.com>
> Cc: BALATON Zoltan <balaton@eik.bme.hu>
> Cc: Andrzej Zaborowski <balrogg@gmail.com>
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> Reviewed-by: Marek Vasut <marex@denx.de>
> Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com>
> ---
>  hw/vmware_vga.c |   18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
> index 62771bb..cd15ee4 100644
> --- a/hw/vmware_vga.c
> +++ b/hw/vmware_vga.c
> @@ -296,6 +296,15 @@ 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)\n", __func__, x);
> +        w += x;
> +        x = 0;
> +    }
> +    if (w < 0) {
> +        fprintf(stderr, "%s: update w was < 0 (%d)\n", __func__, w);
> +        w = 0;
> +    }
>      if (x + w > ds_get_width(s->vga.ds)) {
>          fprintf(stderr, "%s: update width too large x: %d, w: %d\n",
>                  __func__, x, w);
> @@ -303,6 +312,15 @@ static inline void vmsvga_update_rect(struct vmsvga_state_s *s,
>          w = ds_get_width(s->vga.ds) - x;
>      }
>
> +    if (y < 0) {
> +        fprintf(stderr, "%s: update y was < 0 (%d)\n",  __func__, y);
> +        h += y;
> +        y = 0;
> +    }
> +    if (h < 0) {
> +        fprintf(stderr, "%s: update h was < 0 (%d)\n",  __func__, h);
> +        h = 0;
> +    }
>      if (y + h > ds_get_height(s->vga.ds)) {
>          fprintf(stderr, "%s: update height too large y: %d, h: %d\n",
>                  __func__, y, h);
> --
> 1.7.10.4
>
>
Michael Tokarev - Feb. 1, 2013, 12:57 p.m.
26.01.2013 19:09, Blue Swirl wrote:
> Thanks, applied.

Now when I've comments from BALATON Zoltan, I asked Serge to try
out the current qemu in the environment where this buh was easy
to trigger (ubuntu guest with unity desktop) - and he says he
can't reproduce the issue anymore (this does not mean it does not
exist ofcourse).

So maybe this patch isn't really needed?  I dunno.

Thanks,

/mjt

> On Fri, Jan 25, 2013 at 5:23 PM, Michael Tokarev <mjt@tls.msk.ru> wrote:
>> This is a follow up for several attempts to fix this issue.
>>
>> Previous incarnations:
>>
>> 1. http://thread.gmane.org/gmane.linux.ubuntu.bugs.general/3156089
>> https://bugs.launchpad.net/bugs/918791
>> "qemu-kvm dies when using vmvga driver and unity in the guest" bug.
>> Fix by Serge Hallyn:
>>   https://launchpadlibrarian.net/94916786/qemu-vmware.debdiff
>> This fix is incomplete, since it does not check width and height
>> for being negative.  Serge weren't sure if that's the right place
>> to fix it, maybe the fix should be up the stack somewhere.
>>
>> 2. http://thread.gmane.org/gmane.comp.emulators.qemu/166064
>> by Marek Vasut: "vmware_vga: Redraw only visible area"
>>
>> This one adds the (incomplete) check to vmsvga_update_rect_delayed(),
>> the routine just queues the rect updating but does no interesting
>> stuff.  It is also incomplete in the same way as patch by Serge,
>> but also does not touch width&height at all after adjusting x&y,
>> which is wrong.
>>
>> As far as I can see, when processing guest requests, the device
>> places them into a queue (vmsvga_update_rect_delayed()) and
>> processes this queue in different place/time, namely, in
>> vmsvga_update_rect().  Sometimes, vmsvga_update_rect() is
>> called directly, without placing the request to the gueue.
>> This is the place this patch changes, which is the last
>> (deepest) in the stack.  I'm not sure if this is the right
>> place still, since it is possible we have some queue optimization
>> (or may have in the future) which will be upset by negative/wrong
>> values here, so maybe we should check for validity of input
>> right when receiving request from the guest (and maybe even
>> use unsigned types there).  But I don't know the protocol
>> and implementation enough to have a definitive answer.
>>
>> But since vmsvga_update_rect() has other sanity checks already,
>> I'm adding the missing ones there as well.
>>
>> Cc'ing BALATON Zoltan and Andrzej Zaborowski who shows in `git blame'
>> output and may know something in this area.
>>
>> If this patch is accepted, it should be applied to all active
>> stable branches (at least since 1.1, maybe even before), with
>> minor context change (ds_get_*(s->vga.ds) => s->*).  I'm not
>> Cc'ing -stable yet, will do it explicitly once the patch is
>> accepted.
>>
>> BTW, these checks use fprintf(stderr) -- it should be converted
>> to something more appropriate, since stderr will most likely
>> disappear somewhere.
>>
>> Cc: Marek Vasut <marex@denx.de>
>> CC: Serge Hallyn <serge.hallyn@ubuntu.com>
>> Cc: BALATON Zoltan <balaton@eik.bme.hu>
>> Cc: Andrzej Zaborowski <balrogg@gmail.com>
>> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
>> Reviewed-by: Marek Vasut <marex@denx.de>
>> Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com>
>> ---
>>   hw/vmware_vga.c |   18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
>> index 62771bb..cd15ee4 100644
>> --- a/hw/vmware_vga.c
>> +++ b/hw/vmware_vga.c
>> @@ -296,6 +296,15 @@ 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)\n", __func__, x);
>> +        w += x;
>> +        x = 0;
>> +    }
>> +    if (w < 0) {
>> +        fprintf(stderr, "%s: update w was < 0 (%d)\n", __func__, w);
>> +        w = 0;
>> +    }
>>       if (x + w > ds_get_width(s->vga.ds)) {
>>           fprintf(stderr, "%s: update width too large x: %d, w: %d\n",
>>                   __func__, x, w);
>> @@ -303,6 +312,15 @@ static inline void vmsvga_update_rect(struct vmsvga_state_s *s,
>>           w = ds_get_width(s->vga.ds) - x;
>>       }
>>
>> +    if (y < 0) {
>> +        fprintf(stderr, "%s: update y was < 0 (%d)\n",  __func__, y);
>> +        h += y;
>> +        y = 0;
>> +    }
>> +    if (h < 0) {
>> +        fprintf(stderr, "%s: update h was < 0 (%d)\n",  __func__, h);
>> +        h = 0;
>> +    }
>>       if (y + h > ds_get_height(s->vga.ds)) {
>>           fprintf(stderr, "%s: update height too large y: %d, h: %d\n",
>>                   __func__, y, h);
>> --
>> 1.7.10.4
>>
>>
Marek Vasut - Feb. 1, 2013, 1:01 p.m.
Dear Michael Tokarev,

> 26.01.2013 19:09, Blue Swirl wrote:
> > Thanks, applied.
> 
> Now when I've comments from BALATON Zoltan, I asked Serge to try
> out the current qemu in the environment where this buh was easy
> to trigger (ubuntu guest with unity desktop) - and he says he
> can't reproduce the issue anymore (this does not mean it does not
> exist ofcourse).
> 
> So maybe this patch isn't really needed?  I dunno.

Or maybe the vmware driver was fixed on the host side. This doesn't mean there 
aren't buggy installations in the wild.

> Thanks,
> 
> /mjt
> 
> > On Fri, Jan 25, 2013 at 5:23 PM, Michael Tokarev <mjt@tls.msk.ru> wrote:
> >> This is a follow up for several attempts to fix this issue.
> >> 
> >> Previous incarnations:
> >> 
> >> 1. http://thread.gmane.org/gmane.linux.ubuntu.bugs.general/3156089
> >> https://bugs.launchpad.net/bugs/918791
> >> "qemu-kvm dies when using vmvga driver and unity in the guest" bug.
> >> 
> >> Fix by Serge Hallyn:
> >>   https://launchpadlibrarian.net/94916786/qemu-vmware.debdiff
> >> 
> >> This fix is incomplete, since it does not check width and height
> >> for being negative.  Serge weren't sure if that's the right place
> >> to fix it, maybe the fix should be up the stack somewhere.
> >> 
> >> 2. http://thread.gmane.org/gmane.comp.emulators.qemu/166064
> >> by Marek Vasut: "vmware_vga: Redraw only visible area"
> >> 
> >> This one adds the (incomplete) check to vmsvga_update_rect_delayed(),
> >> the routine just queues the rect updating but does no interesting
> >> stuff.  It is also incomplete in the same way as patch by Serge,
> >> but also does not touch width&height at all after adjusting x&y,
> >> which is wrong.
> >> 
> >> As far as I can see, when processing guest requests, the device
> >> places them into a queue (vmsvga_update_rect_delayed()) and
> >> processes this queue in different place/time, namely, in
> >> vmsvga_update_rect().  Sometimes, vmsvga_update_rect() is
> >> called directly, without placing the request to the gueue.
> >> This is the place this patch changes, which is the last
> >> (deepest) in the stack.  I'm not sure if this is the right
> >> place still, since it is possible we have some queue optimization
> >> (or may have in the future) which will be upset by negative/wrong
> >> values here, so maybe we should check for validity of input
> >> right when receiving request from the guest (and maybe even
> >> use unsigned types there).  But I don't know the protocol
> >> and implementation enough to have a definitive answer.
> >> 
> >> But since vmsvga_update_rect() has other sanity checks already,
> >> I'm adding the missing ones there as well.
> >> 
> >> Cc'ing BALATON Zoltan and Andrzej Zaborowski who shows in `git blame'
> >> output and may know something in this area.
> >> 
> >> If this patch is accepted, it should be applied to all active
> >> stable branches (at least since 1.1, maybe even before), with
> >> minor context change (ds_get_*(s->vga.ds) => s->*).  I'm not
> >> Cc'ing -stable yet, will do it explicitly once the patch is
> >> accepted.
> >> 
> >> BTW, these checks use fprintf(stderr) -- it should be converted
> >> to something more appropriate, since stderr will most likely
> >> disappear somewhere.
> >> 
> >> Cc: Marek Vasut <marex@denx.de>
> >> CC: Serge Hallyn <serge.hallyn@ubuntu.com>
> >> Cc: BALATON Zoltan <balaton@eik.bme.hu>
> >> Cc: Andrzej Zaborowski <balrogg@gmail.com>
> >> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> >> Reviewed-by: Marek Vasut <marex@denx.de>
> >> Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com>
> >> ---
> >> 
> >>   hw/vmware_vga.c |   18 ++++++++++++++++++
> >>   1 file changed, 18 insertions(+)
> >> 
> >> diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
> >> index 62771bb..cd15ee4 100644
> >> --- a/hw/vmware_vga.c
> >> +++ b/hw/vmware_vga.c
> >> @@ -296,6 +296,15 @@ 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)\n", __func__, x);
> >> +        w += x;
> >> +        x = 0;
> >> +    }
> >> +    if (w < 0) {
> >> +        fprintf(stderr, "%s: update w was < 0 (%d)\n", __func__, w);
> >> +        w = 0;
> >> +    }
> >> 
> >>       if (x + w > ds_get_width(s->vga.ds)) {
> >>       
> >>           fprintf(stderr, "%s: update width too large x: %d, w: %d\n",
> >>           
> >>                   __func__, x, w);
> >> 
> >> @@ -303,6 +312,15 @@ static inline void vmsvga_update_rect(struct
> >> vmsvga_state_s *s,
> >> 
> >>           w = ds_get_width(s->vga.ds) - x;
> >>       
> >>       }
> >> 
> >> +    if (y < 0) {
> >> +        fprintf(stderr, "%s: update y was < 0 (%d)\n",  __func__, y);
> >> +        h += y;
> >> +        y = 0;
> >> +    }
> >> +    if (h < 0) {
> >> +        fprintf(stderr, "%s: update h was < 0 (%d)\n",  __func__, h);
> >> +        h = 0;
> >> +    }
> >> 
> >>       if (y + h > ds_get_height(s->vga.ds)) {
> >>       
> >>           fprintf(stderr, "%s: update height too large y: %d, h: %d\n",
> >>           
> >>                   __func__, y, h);
> >> 
> >> --
> >> 1.7.10.4

Best regards,
Marek Vasut

Patch

diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index 62771bb..cd15ee4 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -296,6 +296,15 @@  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)\n", __func__, x);
+        w += x;
+        x = 0;
+    }
+    if (w < 0) {
+        fprintf(stderr, "%s: update w was < 0 (%d)\n", __func__, w);
+        w = 0;
+    }
     if (x + w > ds_get_width(s->vga.ds)) {
         fprintf(stderr, "%s: update width too large x: %d, w: %d\n",
                 __func__, x, w);
@@ -303,6 +312,15 @@  static inline void vmsvga_update_rect(struct vmsvga_state_s *s,
         w = ds_get_width(s->vga.ds) - x;
     }
 
+    if (y < 0) {
+        fprintf(stderr, "%s: update y was < 0 (%d)\n",  __func__, y);
+        h += y;
+        y = 0;
+    }
+    if (h < 0) {
+        fprintf(stderr, "%s: update h was < 0 (%d)\n",  __func__, h);
+        h = 0;
+    }
     if (y + h > ds_get_height(s->vga.ds)) {
         fprintf(stderr, "%s: update height too large y: %d, h: %d\n",
                 __func__, y, h);