[Qemu-devel] [PATCH to consider for 0.12] vmware_vga: Don't crash on too-big DEFINE_CURSOR command

[Qemu-devel] [PATCH to consider for 0.12] vmware_vga: Don't crash on too-big DEFINE_CURSOR command

[Roland Dreier]

Hi Anthony -- just sent this patch to qemu-devel (although I don't see
it in archives yet).  Anyway I realize it is really really late given
your release timeframe but I think the risk of this pretty minimal, and
the patch fixes a crash in a pretty reasonable config (running a modern
Linux distro with the fastest guest video adapter).  So please consider
this for 0.12.

Another possibility would be to just take the part of the patch that
bumps the array size in the structure, since that seems to have
essentially 0 risk and fixes the crash in the case I've seen.

Thanks,
  Roland

===

QEMU crashes with vmware_vga when running a Linux guest with the latest
X.org vmware video driver if QEMU is using SDL for video output.  In
this case, vmware_vga advertises cursor acceleration to the guest, and
the crash comes when the guest does a DEFINE_CURSOR command with a 64x64
32bpp cursor.  This request overruns the image[] array in struct
vmsvga_cursor_definition_s and QEMU ends up segfaulting because of
memory corruption caused by writing past the end of the array.

Fix this by enlarging the image[] array to be able to hold 4096 32-bit
pixels so we don't fail for the case of 64*64*32bpp, and also add error
checking to avoid a crash if an even bigger request is sent by a guest.

Signed-off-by: Roland Dreier <rolandd@cisco.com>
---
 hw/vmware_vga.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index f3e3749..d253a2e 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -462,7 +462,7 @@ struct vmsvga_cursor_definition_s {
     int hot_x;
     int hot_y;
     uint32_t mask[1024];
-    uint32_t image[1024];
+    uint32_t image[4096];	/* allow for 64x64 32bpp cursor */
 };
 
 #define SVGA_BITMAP_SIZE(w, h)		((((w) + 31) >> 5) * (h))
@@ -557,6 +557,15 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
             cursor.height = y = vmsvga_fifo_read(s);
             vmsvga_fifo_read(s);
             cursor.bpp = vmsvga_fifo_read(s);
+
+	    if (SVGA_BITMAP_SIZE(x, y) > sizeof cursor.mask / sizeof (uint32_t) ||
+		SVGA_PIXMAP_SIZE(x, y, cursor.bpp) > sizeof cursor.image / sizeof (uint32_t)) {
+		    fprintf(stderr, "%s: DEFINE_CSURSOR too large x: %d, y: %d bpp: %d\n",
+			    __FUNCTION__, x, y, cursor.bpp);
+		    args = SVGA_BITMAP_SIZE(x, y) + SVGA_PIXMAP_SIZE(x, y, cursor.bpp);
+		    goto badcmd;
+	    }
+
             for (args = 0; args < SVGA_BITMAP_SIZE(x, y); args ++)
                 cursor.mask[args] = vmsvga_fifo_read_raw(s);
             for (args = 0; args < SVGA_PIXMAP_SIZE(x, y, cursor.bpp); args ++)

[Qemu-devel] Re: [PATCH to consider for 0.12] vmware_vga: Don't crash on too-big DEFINE_CURSOR command

[Anthony Liguori]

Hi Roland,

Roland Dreier wrote:
> Hi Anthony -- just sent this patch to qemu-devel (although I don't see
> it in archives yet).  Anyway I realize it is really really late given
> your release timeframe but I think the risk of this pretty minimal, and
> the patch fixes a crash in a pretty reasonable config (running a modern
> Linux distro with the fastest guest video adapter).  So please consider
> this for 0.12.
>
> Another possibility would be to just take the part of the patch that
> bumps the array size in the structure, since that seems to have
> essentially 0 risk and fixes the crash in the case I've seen.
>   

Thanks for the patch.  I'm planning on giving Dave Airlie's series a try 
for 0.12.0.  I'm pretty comfortable with those patches (since a few of 
them are mine :-)).  I also don't think vmware-vga is going to be 
reliable without them so I don't think pulling in the one fix is good 
enough.

His last patch has the same fix without the printf().  The printf is 
probably something to avoid since a malicious guest could create a storm 
of them.  Since libvirt logs stderr by default, the result could be 
pretty nasty.

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH to consider for 0.12] vmware_vga: Don't crash on too-big DEFINE_CURSOR command

[Roland Dreier]

 > Thanks for the patch.  I'm planning on giving Dave Airlie's series a
 > try for 0.12.0.  I'm pretty comfortable with those patches (since a
 > few of them are mine :-)).  I also don't think vmware-vga is going to
 > be reliable without them so I don't think pulling in the one fix is
 > good enough.
 > 
 > His last patch has the same fix without the printf().  The printf is
 > probably something to avoid since a malicious guest could create a
 > storm of them.  Since libvirt logs stderr by default, the result could
 > be pretty nasty.

Fair enough... I just saw Dave's patches go by, and I guess we
independently fixed the cursor size thing at right around the same time.
How about the following, without the fprintf but with paranoid checks
(since a malicious guest could send a bad DEFINE_CURSOR and do who knows
what with the buffer overrun, which is even worse than spamming logs ;)

====

QEMU crashes with vmware_vga when running a Linux guest with the latest
X.org vmware video driver if QEMU is using SDL for video output.  In
this case, vmware_vga advertises cursor acceleration to the guest, and
the crash comes when the guest does a DEFINE_CURSOR command with a 64x64
32bpp cursor.  This request overruns the image[] array in struct
vmsvga_cursor_definition_s and QEMU ends up segfaulting because of
memory corruption caused by writing past the end of the array.

Fix this by enlarging the image[] array to be able to hold 4096 32-bit
pixels so we don't fail for the case of 64*64*32bpp, and also add error
checking to avoid a crash if an even bigger request is sent by a guest.

Signed-off-by: Roland Dreier <rolandd@cisco.com>
---
 hw/vmware_vga.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index f3e3749..75d90f2 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -462,7 +462,7 @@ struct vmsvga_cursor_definition_s {
     int hot_x;
     int hot_y;
     uint32_t mask[1024];
-    uint32_t image[1024];
+    uint32_t image[4096];	/* allow for 64x64 32bpp cursor */
 };
 
 #define SVGA_BITMAP_SIZE(w, h)		((((w) + 31) >> 5) * (h))
@@ -557,6 +557,13 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
             cursor.height = y = vmsvga_fifo_read(s);
             vmsvga_fifo_read(s);
             cursor.bpp = vmsvga_fifo_read(s);
+
+	    if (SVGA_BITMAP_SIZE(x, y) > sizeof cursor.mask ||
+		SVGA_PIXMAP_SIZE(x, y, cursor.bpp) > sizeof cursor.image) {
+		    args = SVGA_BITMAP_SIZE(x, y) + SVGA_PIXMAP_SIZE(x, y, cursor.bpp);
+		    goto badcmd;
+	    }
+
             for (args = 0; args < SVGA_BITMAP_SIZE(x, y); args ++)
                 cursor.mask[args] = vmsvga_fifo_read_raw(s);
             for (args = 0; args < SVGA_PIXMAP_SIZE(x, y, cursor.bpp); args ++)

[Qemu-devel] Re: [PATCH to consider for 0.12] vmware_vga: Don't crash on too-big DEFINE_CURSOR command

[Roland Dreier]

 > His last patch has the same fix without the printf().  The printf is
 > probably something to avoid since a malicious guest could create a
 > storm of them.  Since libvirt logs stderr by default, the result could
 > be pretty nasty.

By the way, are the

        fprintf(stderr, "%s: update width too large x: %d, w: %d\n",
                        __FUNCTION__, x, w);

        fprintf(stderr, "%s: update height too large y: %d, h: %d\n",
                        __FUNCTION__, y, h);

prints triggerable by a guest?  (I think so -- if so I can send a patch
removing them if you want)

How about the printf()s to stdout?  eg a guest can cause a flood of the

            printf("%s: Unknown command 0x%02x in SVGA command FIFO\n",
                            __FUNCTION__, cmd);

or

            printf("%s: guest runs %s.\n", __FUNCTION__,
                            vmsvga_guest_id[value - GUEST_OS_BASE]);

output if it wants pretty trivially.

[Qemu-devel] Re: [PATCH to consider for 0.12] vmware_vga: Don't crash on too-big DEFINE_CURSOR command

[Anthony Liguori]

Roland Dreier wrote:
>  > His last patch has the same fix without the printf().  The printf is
>  > probably something to avoid since a malicious guest could create a
>  > storm of them.  Since libvirt logs stderr by default, the result could
>  > be pretty nasty.
>
> By the way, are the
>
>         fprintf(stderr, "%s: update width too large x: %d, w: %d\n",
>                         __FUNCTION__, x, w);
>
>         fprintf(stderr, "%s: update height too large y: %d, h: %d\n",
>                         __FUNCTION__, y, h);
>
> prints triggerable by a guest?  (I think so -- if so I can send a patch
> removing them if you want)
>
> How about the printf()s to stdout?  eg a guest can cause a flood of the
>
>             printf("%s: Unknown command 0x%02x in SVGA command FIFO\n",
>                             __FUNCTION__, cmd);
>
> or
>
>             printf("%s: guest runs %s.\n", __FUNCTION__,
>                             vmsvga_guest_id[value - GUEST_OS_BASE]);
>
> output if it wants pretty trivially.
>   
Yeah, that's all stuff that needs to go.

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH to consider for 0.12] vmware_vga: Don't crash on too-big DEFINE_CURSOR command

[Anthony Liguori]

Roland Dreier wrote:
>  > Thanks for the patch.  I'm planning on giving Dave Airlie's series a
>  > try for 0.12.0.  I'm pretty comfortable with those patches (since a
>  > few of them are mine :-)).  I also don't think vmware-vga is going to
>  > be reliable without them so I don't think pulling in the one fix is
>  > good enough.
>  > 
>  > His last patch has the same fix without the printf().  The printf is
>  > probably something to avoid since a malicious guest could create a
>  > storm of them.  Since libvirt logs stderr by default, the result could
>  > be pretty nasty.
>
> Fair enough... I just saw Dave's patches go by, and I guess we
> independently fixed the cursor size thing at right around the same time.
> How about the following, without the fprintf but with paranoid checks
> (since a malicious guest could send a bad DEFINE_CURSOR and do who knows
> what with the buffer overrun, which is even worse than spamming logs ;)
>   

Definitely seems reasonable.

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH to consider for 0.12] vmware_vga: Don't crash on too-big DEFINE_CURSOR command

[Roland Dreier]

I see that you applied Dave's series, no worries on that, but I think we
really want the following, since a clever enough malicious guest can
probably turn the overrun into code execution in the host QEMU (though
I've not tried writing an exploit or anything like that)... not sure if
this merits the whole CVE process but it does look very much worth
fixing, and probably the other guest commands want auditing too...

===
vmware_vga: Check cursor dimensions passed from guest to avoid buffer overflow

Check that the cursor dimensions passed from the guest for the
DEFINE_CURSOR command don't overflow the available space in the
cursor.image[] or cursor.mask[] arrays before copying data from the
guest into those arrays.

Signed-off-by: Roland Dreier <rolandd@cisco.com>
---
 hw/vmware_vga.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index 7ab1c79..5e969ae 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -562,6 +562,13 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
             cursor.height = y = vmsvga_fifo_read(s);
             vmsvga_fifo_read(s);
             cursor.bpp = vmsvga_fifo_read(s);
+
+	    if (SVGA_BITMAP_SIZE(x, y) > sizeof cursor.mask ||
+		SVGA_PIXMAP_SIZE(x, y, cursor.bpp) > sizeof cursor.image) {
+		    args = SVGA_BITMAP_SIZE(x, y) + SVGA_PIXMAP_SIZE(x, y, cursor.bpp);
+		    goto badcmd;
+	    }
+
             for (args = 0; args < SVGA_BITMAP_SIZE(x, y); args ++)
                 cursor.mask[args] = vmsvga_fifo_read_raw(s);
             for (args = 0; args < SVGA_PIXMAP_SIZE(x, y, cursor.bpp); args ++)

[Qemu-devel] [PATCH resend] vmware_vga: Check cursor dimensions passed from guest to avoid buffer overflow

[Roland Dreier]

Check that the cursor dimensions passed from the guest for the
DEFINE_CURSOR command don't overflow the available space in the
cursor.image[] or cursor.mask[] arrays before copying data from the
guest into those arrays.

Signed-off-by: Roland Dreier <rolandd@cisco.com>
---
Hi Anthony,

as far as I can tell this seems to have slipped through the cracks.  I
think this is fairly important: it is a guest-triggerable stack smashing
attack in the worst case.

Thanks,
  Roland

 hw/vmware_vga.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index 7ab1c79..5e969ae 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -562,6 +562,13 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
             cursor.height = y = vmsvga_fifo_read(s);
             vmsvga_fifo_read(s);
             cursor.bpp = vmsvga_fifo_read(s);
+
+	    if (SVGA_BITMAP_SIZE(x, y) > sizeof cursor.mask ||
+		SVGA_PIXMAP_SIZE(x, y, cursor.bpp) > sizeof cursor.image) {
+		    args = SVGA_BITMAP_SIZE(x, y) + SVGA_PIXMAP_SIZE(x, y, cursor.bpp);
+		    goto badcmd;
+	    }
+
             for (args = 0; args < SVGA_BITMAP_SIZE(x, y); args ++)
                 cursor.mask[args] = vmsvga_fifo_read_raw(s);
             for (args = 0; args < SVGA_PIXMAP_SIZE(x, y, cursor.bpp); args ++)

Re: [Qemu-devel] [PATCH resend] vmware_vga: Check cursor dimensions passed from guest to avoid buffer overflow

[Anthony Liguori]

On 01/05/2010 10:43 PM, Roland Dreier wrote:
> Check that the cursor dimensions passed from the guest for the
> DEFINE_CURSOR command don't overflow the available space in the
> cursor.image[] or cursor.mask[] arrays before copying data from the
> guest into those arrays.
>
> Signed-off-by: Roland Dreier<rolandd@cisco.com>
>    

Applied.  Thanks.

Regards,

Anthony Liguori
> ---
> Hi Anthony,
>
> as far as I can tell this seems to have slipped through the cracks.  I
> think this is fairly important: it is a guest-triggerable stack smashing
> attack in the worst case.
>
> Thanks,
>    Roland
>
>   hw/vmware_vga.c |    7 +++++++
>   1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
> index 7ab1c79..5e969ae 100644
> --- a/hw/vmware_vga.c
> +++ b/hw/vmware_vga.c
> @@ -562,6 +562,13 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
>               cursor.height = y = vmsvga_fifo_read(s);
>               vmsvga_fifo_read(s);
>               cursor.bpp = vmsvga_fifo_read(s);
> +
> +	    if (SVGA_BITMAP_SIZE(x, y)>  sizeof cursor.mask ||
> +		SVGA_PIXMAP_SIZE(x, y, cursor.bpp)>  sizeof cursor.image) {
> +		    args = SVGA_BITMAP_SIZE(x, y) + SVGA_PIXMAP_SIZE(x, y, cursor.bpp);
> +		    goto badcmd;
> +	    }
> +
>               for (args = 0; args<  SVGA_BITMAP_SIZE(x, y); args ++)
>                   cursor.mask[args] = vmsvga_fifo_read_raw(s);
>               for (args = 0; args<  SVGA_PIXMAP_SIZE(x, y, cursor.bpp); args ++)
>
>
>
>