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

[Serge Hallyn]

Quoting Michael Tokarev (address@hidden):
> 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 <address@hidden>
> Cc: Serge Hallyn <address@hidden>

Acked-by: Serge E. Hallyn <address@hidden>

Thanks for pushing this.

> Cc: BALATON Zoltan <address@hidden>
> Cc: Andrzej Zaborowski <address@hidden>
> Signed-off-by: Michael Tokarev <address@hidden>
> ---
>  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
>