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
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:
>
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
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
+-- 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
© 2016 - 2024 Red Hat, Inc.