[PATCH] ati-vga: Fix checks in ati_2d_blt() to avoid crash

[PATCH] ati-vga: Fix checks in ati_2d_blt() to avoid crash

[BALATON Zoltan]

In some corner cases (that never happen during normal operation but a
malicious guest could program wrong values) pixman functions were
called with parameters that result in a crash. Fix this and add more
checks to disallow such cases.

Reported-by: Ziming Zhang <ezrakiez@gmail.com>
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/display/ati_2d.c | 37 ++++++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
index 42e82311eb..23a8ae0cd8 100644
--- a/hw/display/ati_2d.c
+++ b/hw/display/ati_2d.c
@@ -53,12 +53,20 @@ void ati_2d_blt(ATIVGAState *s)
             s->vga.vbe_start_addr, surface_data(ds), surface_stride(ds),
             surface_bits_per_pixel(ds),
             (s->regs.dp_mix & GMC_ROP3_MASK) >> 16);
-    int dst_x = (s->regs.dp_cntl & DST_X_LEFT_TO_RIGHT ?
-                 s->regs.dst_x : s->regs.dst_x + 1 - s->regs.dst_width);
-    int dst_y = (s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM ?
-                 s->regs.dst_y : s->regs.dst_y + 1 - s->regs.dst_height);
+    unsigned dst_x = (s->regs.dp_cntl & DST_X_LEFT_TO_RIGHT ?
+                      s->regs.dst_x : s->regs.dst_x + 1 - s->regs.dst_width);
+    unsigned dst_y = (s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM ?
+                      s->regs.dst_y : s->regs.dst_y + 1 - s->regs.dst_height);
     int bpp = ati_bpp_from_datatype(s);
+    if (!bpp) {
+        qemu_log_mask(LOG_GUEST_ERROR, "Invalid bpp\n");
+        return;
+    }
     int dst_stride = DEFAULT_CNTL ? s->regs.dst_pitch : s->regs.default_pitch;
+    if (!dst_stride) {
+        qemu_log_mask(LOG_GUEST_ERROR, "Zero dest pitch\n");
+        return;
+    }
     uint8_t *dst_bits = s->vga.vram_ptr + (DEFAULT_CNTL ?
                         s->regs.dst_offset : s->regs.default_offset);
 
@@ -82,12 +90,16 @@ void ati_2d_blt(ATIVGAState *s)
     switch (s->regs.dp_mix & GMC_ROP3_MASK) {
     case ROP3_SRCCOPY:
     {
-        int src_x = (s->regs.dp_cntl & DST_X_LEFT_TO_RIGHT ?
-                     s->regs.src_x : s->regs.src_x + 1 - s->regs.dst_width);
-        int src_y = (s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM ?
-                     s->regs.src_y : s->regs.src_y + 1 - s->regs.dst_height);
+        unsigned src_x = (s->regs.dp_cntl & DST_X_LEFT_TO_RIGHT ?
+                       s->regs.src_x : s->regs.src_x + 1 - s->regs.dst_width);
+        unsigned src_y = (s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM ?
+                       s->regs.src_y : s->regs.src_y + 1 - s->regs.dst_height);
         int src_stride = DEFAULT_CNTL ?
                          s->regs.src_pitch : s->regs.default_pitch;
+        if (!src_stride) {
+            qemu_log_mask(LOG_GUEST_ERROR, "Zero source pitch\n");
+            return;
+        }
         uint8_t *src_bits = s->vga.vram_ptr + (DEFAULT_CNTL ?
                             s->regs.src_offset : s->regs.default_offset);
 
@@ -137,8 +149,10 @@ void ati_2d_blt(ATIVGAState *s)
                                     dst_y * surface_stride(ds),
                                     s->regs.dst_height * surface_stride(ds));
         }
-        s->regs.dst_x += s->regs.dst_width;
-        s->regs.dst_y += s->regs.dst_height;
+        s->regs.dst_x = (s->regs.dp_cntl & DST_X_LEFT_TO_RIGHT ?
+                         dst_x + s->regs.dst_width : dst_x);
+        s->regs.dst_y = (s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM ?
+                         dst_y + s->regs.dst_height : dst_y);
         break;
     }
     case ROP3_PATCOPY:
@@ -179,7 +193,8 @@ void ati_2d_blt(ATIVGAState *s)
                                     dst_y * surface_stride(ds),
                                     s->regs.dst_height * surface_stride(ds));
         }
-        s->regs.dst_y += s->regs.dst_height;
+        s->regs.dst_y = (s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM ?
+                         dst_y + s->regs.dst_height : dst_y);
         break;
     }
     default:
-- 
2.21.1

Re: [PATCH] ati-vga: Fix checks in ati_2d_blt() to avoid crash

[Philippe Mathieu-Daudé]

On 4/6/20 10:34 PM, BALATON Zoltan wrote:
> In some corner cases (that never happen during normal operation but a
> malicious guest could program wrong values) pixman functions were
> called with parameters that result in a crash. Fix this and add more
> checks to disallow such cases.

(Fair) question on IRC. Is this patch fixing CVE-2020-24352?

Public on August 14, 2020

Description

An out-of-bounds memory access flaw was found in the ATI VGA device
implementation of the QEMU emulator. This flaw occurs in the
ati_2d_blt() routine while handling MMIO write operations through the
ati_mm_write() callback. A malicious guest could use this flaw to crash
the QEMU process on the host, resulting in a denial of service.

This points to a BZ#1847385 which is private:

"You are not authorized to access bug #1847385.

Most likely the bug has been restricted for internal development
processes and we cannot grant access."

https://bugzilla.redhat.com/show_bug.cgi?id=1847385

Maybe we could improve the security process, when a CVE embargo
expires, the public statement could point at the commit(s) fixing
the bug.

> 
> Reported-by: Ziming Zhang <ezrakiez@gmail.com>

I'm not sure this is the correct CVE because CVE-2020-24352
is said reported by Yi Ren from the Alibaba Cloud Intelligence
Security Team.

Thanks,

Phil.

> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/display/ati_2d.c | 37 ++++++++++++++++++++++++++-----------
>  1 file changed, 26 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
> index 42e82311eb..23a8ae0cd8 100644
> --- a/hw/display/ati_2d.c
> +++ b/hw/display/ati_2d.c
> @@ -53,12 +53,20 @@ void ati_2d_blt(ATIVGAState *s)
>              s->vga.vbe_start_addr, surface_data(ds), surface_stride(ds),
>              surface_bits_per_pixel(ds),
>              (s->regs.dp_mix & GMC_ROP3_MASK) >> 16);
> -    int dst_x = (s->regs.dp_cntl & DST_X_LEFT_TO_RIGHT ?
> -                 s->regs.dst_x : s->regs.dst_x + 1 - s->regs.dst_width);
> -    int dst_y = (s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM ?
> -                 s->regs.dst_y : s->regs.dst_y + 1 - s->regs.dst_height);
> +    unsigned dst_x = (s->regs.dp_cntl & DST_X_LEFT_TO_RIGHT ?
> +                      s->regs.dst_x : s->regs.dst_x + 1 - s->regs.dst_width);
> +    unsigned dst_y = (s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM ?
> +                      s->regs.dst_y : s->regs.dst_y + 1 - s->regs.dst_height);
>      int bpp = ati_bpp_from_datatype(s);
> +    if (!bpp) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "Invalid bpp\n");
> +        return;
> +    }
>      int dst_stride = DEFAULT_CNTL ? s->regs.dst_pitch : s->regs.default_pitch;
> +    if (!dst_stride) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "Zero dest pitch\n");
> +        return;
> +    }
>      uint8_t *dst_bits = s->vga.vram_ptr + (DEFAULT_CNTL ?
>                          s->regs.dst_offset : s->regs.default_offset);
>  
> @@ -82,12 +90,16 @@ void ati_2d_blt(ATIVGAState *s)
>      switch (s->regs.dp_mix & GMC_ROP3_MASK) {
>      case ROP3_SRCCOPY:
>      {
> -        int src_x = (s->regs.dp_cntl & DST_X_LEFT_TO_RIGHT ?
> -                     s->regs.src_x : s->regs.src_x + 1 - s->regs.dst_width);
> -        int src_y = (s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM ?
> -                     s->regs.src_y : s->regs.src_y + 1 - s->regs.dst_height);
> +        unsigned src_x = (s->regs.dp_cntl & DST_X_LEFT_TO_RIGHT ?
> +                       s->regs.src_x : s->regs.src_x + 1 - s->regs.dst_width);
> +        unsigned src_y = (s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM ?
> +                       s->regs.src_y : s->regs.src_y + 1 - s->regs.dst_height);
>          int src_stride = DEFAULT_CNTL ?
>                           s->regs.src_pitch : s->regs.default_pitch;
> +        if (!src_stride) {
> +            qemu_log_mask(LOG_GUEST_ERROR, "Zero source pitch\n");
> +            return;
> +        }
>          uint8_t *src_bits = s->vga.vram_ptr + (DEFAULT_CNTL ?
>                              s->regs.src_offset : s->regs.default_offset);
>  
> @@ -137,8 +149,10 @@ void ati_2d_blt(ATIVGAState *s)
>                                      dst_y * surface_stride(ds),
>                                      s->regs.dst_height * surface_stride(ds));
>          }
> -        s->regs.dst_x += s->regs.dst_width;
> -        s->regs.dst_y += s->regs.dst_height;
> +        s->regs.dst_x = (s->regs.dp_cntl & DST_X_LEFT_TO_RIGHT ?
> +                         dst_x + s->regs.dst_width : dst_x);
> +        s->regs.dst_y = (s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM ?
> +                         dst_y + s->regs.dst_height : dst_y);
>          break;
>      }
>      case ROP3_PATCOPY:
> @@ -179,7 +193,8 @@ void ati_2d_blt(ATIVGAState *s)
>                                      dst_y * surface_stride(ds),
>                                      s->regs.dst_height * surface_stride(ds));
>          }
> -        s->regs.dst_y += s->regs.dst_height;
> +        s->regs.dst_y = (s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM ?
> +                         dst_y + s->regs.dst_height : dst_y);
>          break;
>      }
>      default:
> 

Re: [PATCH] ati-vga: Fix checks in ati_2d_blt() to avoid crash

[BALATON Zoltan via]

[-- Attachment #1: Type: text/plain, Size: 1961 bytes --]

On Sat, 22 Aug 2020, Philippe Mathieu-Daudé wrote:
> On 4/6/20 10:34 PM, BALATON Zoltan wrote:
>> In some corner cases (that never happen during normal operation but a
>> malicious guest could program wrong values) pixman functions were
>> called with parameters that result in a crash. Fix this and add more
>> checks to disallow such cases.
>
> (Fair) question on IRC. Is this patch fixing CVE-2020-24352?
>
> Public on August 14, 2020
>
> Description
>
> An out-of-bounds memory access flaw was found in the ATI VGA device
> implementation of the QEMU emulator. This flaw occurs in the
> ati_2d_blt() routine while handling MMIO write operations through the
> ati_mm_write() callback. A malicious guest could use this flaw to crash
> the QEMU process on the host, resulting in a denial of service.

Probably this patch does not fix all possible malicious register writes a 
guest could do. This was fixing problems reported earlier but then I got 
some more reports around 5.1.0 freeze about some more overruns which I 
could not yet look at and nobody else was fixing it either so it's 
possible some bugs are still left in the checks.

However this is hardly security critical as ati-vga is experimental and 
not fully implemented yet so anyone using it will likely get other 
problems (such as drivers not loading) before a guest could exploit this. 
I think QEMU only considers bugs in parts that are used for virtualisation 
via KVM as security problems so maybe this does not even need a CVE and 
could be normally reported/discussed on the mailing list.

Basically what needs to be done is go through the checks again to verify 
that we don't pass params to pixman or set_dirty that result in access 
outside the video ram area. Probably there's still an off by one error or 
some other mistake. I'll eventually may try to fix it but if anyone is 
sending a patch earlier that's welcome. I don't have much time for QEMU 
now.

Regards,
BALATON Zoltan

Re: [PATCH] ati-vga: Fix checks in ati_2d_blt() to avoid crash

[Michael Tokarev]

22.08.2020 15:31, Philippe Mathieu-Daudé пишет:
> On 4/6/20 10:34 PM, BALATON Zoltan wrote:
>> In some corner cases (that never happen during normal operation but a
>> malicious guest could program wrong values) pixman functions were
>> called with parameters that result in a crash. Fix this and add more
>> checks to disallow such cases.
> 
> (Fair) question on IRC. Is this patch fixing CVE-2020-24352?

Actually this is CVE-2020-11869. Almost of the same level as
CVE-2020-24352, I think, especialy having in mind the experimental
state of ati-vga.

CVE-2020-24352 is still muddy.

/mjt

Re: [PATCH] ati-vga: Fix checks in ati_2d_blt() to avoid crash

[P J P]

[-- Attachment #1: Type: text/plain, Size: 721 bytes --]

+-- On Sat, 22 Aug 2020, Philippe Mathieu-Daudé wrote --+
| This points to a BZ#1847385 which is private:
| "You are not authorized to access bug #1847385.
| https://bugzilla.redhat.com/show_bug.cgi?id=1847385

CVE-2020-24352:
  -> https://bugzilla.redhat.com/show_bug.cgi?id=1847584

This is the pubic bug.

| Maybe we could improve the security process, when a CVE embargo
| expires, the public statement could point at the commit(s) fixing
| the bug.

Yes, we generally tag/log upstream fixes against the CVE bugs. It seems 
missing in this case, maybe because fix was sent upstream latter. Will fix it.


Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D