| Message ID | 1359025878-15531-1-git-send-email-mjt@msgid.tls.msk.ru |
|---|---|
| State | New |
| Headers | show |
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
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 >
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
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
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
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
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);
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(+)