Re: [Qemu-devel] [PATCH v2] vmware_vga: fix out of bounds and invalid rects updating

Re: [Qemu-devel] [PATCH v2] vmware_vga: fix out of bounds and invalid rects updating

[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.

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

Re: [Qemu-devel] [PATCH v2] vmware_vga: fix out of bounds and invalid rects updating

[Marek Vasut]

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