Patchwork vmware_vga: fix out of bounds and invalid rects updating

login
register
mail settings
Submitter Michael Tokarev
Date Jan. 24, 2013, 11:11 a.m.
Message ID <1359025878-15531-1-git-send-email-mjt@msgid.tls.msk.ru>
Download mbox | patch
Permalink /patch/215315/
State New
Headers show

Comments

Michael Tokarev - Jan. 24, 2013, 11:11 a.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>
---
 hw/vmware_vga.c |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
Marek Vasut - Jan. 24, 2013, 12:23 p.m.
Dear Michael Tokarev,

> 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"

Looks like this should do the trick as well.

Reviewed-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut
Serge Hallyn - Jan. 24, 2013, 3 p.m.
Quoting Michael Tokarev (mjt@tls.msk.ru):
> 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>

Acked-by: Serge E. Hallyn <serge.hallyn@ubuntu.com>

Thanks for pushing this.

> Cc: BALATON Zoltan <balaton@eik.bme.hu>
> Cc: Andrzej Zaborowski <balrogg@gmail.com>
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  hw/vmware_vga.c |   18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
> index 62771bb..c8a95c2 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", __FUNCTION__, x);
> +        w += x;
> +        x = 0;
> +    }
> +    if (w < 0) {
> +        fprintf(stderr, "%s: update w was < 0 (%d)\n", __FUNCTION__, 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",  __FUNCTION__, y);
> +        h += y;
> +        y = 0;
> +    }
> +    if (h < 0) {
> +        fprintf(stderr, "%s: update h was < 0 (%d)\n",  __FUNCTION__, 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
>
Anthony Liguori - Jan. 25, 2013, 2:15 p.m.
Hi,

Thank you for submitting your patch series.  checkpatch.pl has
detected that one or more of the patches in this series violate
the QEMU coding style.

If you believe this message was sent in error, please ignore it
or respond here with an explanation.

Otherwise, please correct the coding style issues and resubmit a
new version of the patch.

For more information about QEMU coding style, see:

http://git.qemu.org/?p=qemu.git;a=blob_plain;f=CODING_STYLE;hb=HEAD

Here is the output from checkpatch.pl:

Subject: vmware_vga: fix out of bounds and invalid rects updating
WARNING: __func__ should be used instead of gcc specific __FUNCTION__
#73: FILE: hw/vmware_vga.c:300:
+        fprintf(stderr, "%s: update x was < 0 (%d)\n", __FUNCTION__, x);

WARNING: __func__ should be used instead of gcc specific __FUNCTION__
#78: FILE: hw/vmware_vga.c:305:
+        fprintf(stderr, "%s: update w was < 0 (%d)\n", __FUNCTION__, w);

WARNING: __func__ should be used instead of gcc specific __FUNCTION__
#89: FILE: hw/vmware_vga.c:316:
+        fprintf(stderr, "%s: update y was < 0 (%d)\n",  __FUNCTION__, y);

WARNING: __func__ should be used instead of gcc specific __FUNCTION__
#94: FILE: hw/vmware_vga.c:321:
+        fprintf(stderr, "%s: update h was < 0 (%d)\n",  __FUNCTION__, h);

total: 0 errors, 4 warnings, 30 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.



Regards,

Anthony Liguori
Michael Tokarev - Jan. 25, 2013, 2:46 p.m.
25.01.2013 18:15, Anthony Liguori wrote:
> Hi,
> 
> Thank you for submitting your patch series.  checkpatch.pl has
> detected that one or more of the patches in this series violate
> the QEMU coding style.
> 
> If you believe this message was sent in error, please ignore it
> or respond here with an explanation.
> 
> Otherwise, please correct the coding style issues and resubmit a
> new version of the patch.
> 
> For more information about QEMU coding style, see:
> 
> http://git.qemu.org/?p=qemu.git;a=blob_plain;f=CODING_STYLE;hb=HEAD
> 
> Here is the output from checkpatch.pl:
> 
> Subject: vmware_vga: fix out of bounds and invalid rects updating
> WARNING: __func__ should be used instead of gcc specific __FUNCTION__
> #73: FILE: hw/vmware_vga.c:300:
> +        fprintf(stderr, "%s: update x was < 0 (%d)\n", __FUNCTION__, x);
> 
> WARNING: __func__ should be used instead of gcc specific __FUNCTION__
> #78: FILE: hw/vmware_vga.c:305:
> +        fprintf(stderr, "%s: update w was < 0 (%d)\n", __FUNCTION__, w);
> 
> WARNING: __func__ should be used instead of gcc specific __FUNCTION__
> #89: FILE: hw/vmware_vga.c:316:
> +        fprintf(stderr, "%s: update y was < 0 (%d)\n",  __FUNCTION__, y);
> 
> WARNING: __func__ should be used instead of gcc specific __FUNCTION__
> #94: FILE: hw/vmware_vga.c:321:
> +        fprintf(stderr, "%s: update h was < 0 (%d)\n",  __FUNCTION__, h);
> 
> total: 0 errors, 4 warnings, 30 lines checked
> 
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.


Okay.  I used the same style as were used in all the surrounding code.

Should I change all the other code too?

Or should the new code be different from the rest of this file?

If we were to change it, I'd vote for applying this bugfix first
and changing all occurences of __FUNCTION__ in one go in the next
patch.

Thanks,

/mjt
Anthony Liguori - Jan. 25, 2013, 5:08 p.m.
Michael Tokarev <mjt@tls.msk.ru> writes:

> 25.01.2013 18:15, Anthony Liguori wrote:
>> Hi,
>> 
>> Thank you for submitting your patch series.  checkpatch.pl has
>> detected that one or more of the patches in this series violate
>> the QEMU coding style.
>> 
>> If you believe this message was sent in error, please ignore it
>> or respond here with an explanation.
>> 
>> Otherwise, please correct the coding style issues and resubmit a
>> new version of the patch.
>> 
>> For more information about QEMU coding style, see:
>> 
>> http://git.qemu.org/?p=qemu.git;a=blob_plain;f=CODING_STYLE;hb=HEAD
>> 
>> Here is the output from checkpatch.pl:
>> 
>> Subject: vmware_vga: fix out of bounds and invalid rects updating
>> WARNING: __func__ should be used instead of gcc specific __FUNCTION__
>> #73: FILE: hw/vmware_vga.c:300:
>> +        fprintf(stderr, "%s: update x was < 0 (%d)\n", __FUNCTION__, x);
>> 
>> WARNING: __func__ should be used instead of gcc specific __FUNCTION__
>> #78: FILE: hw/vmware_vga.c:305:
>> +        fprintf(stderr, "%s: update w was < 0 (%d)\n", __FUNCTION__, w);
>> 
>> WARNING: __func__ should be used instead of gcc specific __FUNCTION__
>> #89: FILE: hw/vmware_vga.c:316:
>> +        fprintf(stderr, "%s: update y was < 0 (%d)\n",  __FUNCTION__, y);
>> 
>> WARNING: __func__ should be used instead of gcc specific __FUNCTION__
>> #94: FILE: hw/vmware_vga.c:321:
>> +        fprintf(stderr, "%s: update h was < 0 (%d)\n",  __FUNCTION__, h);
>> 
>> total: 0 errors, 4 warnings, 30 lines checked
>> 
>> Your patch has style problems, please review.  If any of these errors
>> are false positives report them to the maintainer, see
>> CHECKPATCH in MAINTAINERS.
>
>
> Okay.  I used the same style as were used in all the surrounding code.
>
> Should I change all the other code too?
>
> Or should the new code be different from the rest of this file?
>
> If we were to change it, I'd vote for applying this bugfix first
> and changing all occurences of __FUNCTION__ in one go in the next
> patch.

The patch is fine as-is.  Thanks for the explanation.

Regards,

Anthony Liguori

>
> Thanks,
>
> /mjt
Michael Tokarev - Jan. 25, 2013, 5:15 p.m.
25.01.2013 21:08, Anthony Liguori wrote:
> Michael Tokarev <mjt@tls.msk.ru> writes:
> 
>> 25.01.2013 18:15, Anthony Liguori wrote:

>>> WARNING: __func__ should be used instead of gcc specific __FUNCTION__
>>> #73: FILE: hw/vmware_vga.c:300:
>>> +        fprintf(stderr, "%s: update x was < 0 (%d)\n", __FUNCTION__, x);

>>> Your patch has style problems, please review.  If any of these errors
>>> are false positives report them to the maintainer, see
>>> CHECKPATCH in MAINTAINERS.
>>
>> Okay.  I used the same style as were used in all the surrounding code.

> The patch is fine as-is.  Thanks for the explanation.

Actually it is not, and I'm wrong.  I used the wrong branch when cooked
my patch.  I'll resend.  All other places has already been converted
from __FUNCTION__ to __func__.

Sorry for the noize.

Thanks!

/mjt

Patch

diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index 62771bb..c8a95c2 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", __FUNCTION__, x);
+        w += x;
+        x = 0;
+    }
+    if (w < 0) {
+        fprintf(stderr, "%s: update w was < 0 (%d)\n", __FUNCTION__, 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",  __FUNCTION__, y);
+        h += y;
+        y = 0;
+    }
+    if (h < 0) {
+        fprintf(stderr, "%s: update h was < 0 (%d)\n",  __FUNCTION__, 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);