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