| Message ID | 1359134604-27516-2-git-send-email-mjt@msgid.tls.msk.ru |
|---|---|
| State | New |
| Headers | show |
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
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 > >
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 >> >>
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
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);