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

[Michael Tokarev <mjt@tls.msk.ru>]

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