* [PATCH-for-5.0 0/2] hw/display/sm501: Avoid heap overflow in sm501_2d_operation()
@ 2020-04-11 9:14 Philippe Mathieu-Daudé
2020-04-11 9:14 ` [PATCH-for-5.0 1/2] " Philippe Mathieu-Daudé
2020-04-11 9:14 ` [PATCH-for-5.0 2/2] qtest: Test the Drawing Engine of the SM501 companion Philippe Mathieu-Daudé
0 siblings, 2 replies; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-04-11 9:14 UTC (permalink / raw)
To: qemu-devel, BALATON Zoltan
Cc: Laurent Vivier, Thomas Huth, Magnus Damm,
Philippe Mathieu-Daudé,
Zhang Zi Ming, qemu-ppc, Paolo Bonzini
I once setup a Bugzilla 'Component Watching' rule on 'QEMU + CVE',
and recently found a notification for BZ#1786026 about a heap
overflow in sm501_2d_operation():
https://bugzilla.redhat.com/show_bug.cgi?id=1786026
As this is from december I suppose there was some embargo that
recently expired. Apparently there is a CVE assigned but the
information about it is private.
I'm not sure the upstream community is already aware of this
problem, but since we are in hard freeze and the bug can easily
be avoided, I believe a 3-lines patch is appropriate.
Philippe Mathieu-Daudé (2):
hw/display/sm501: Avoid heap overflow in sm501_2d_operation()
qtest: Test the Drawing Engine of the SM501 companion
hw/display/sm501.c | 6 ++
tests/qtest/sm501-test.c | 106 +++++++++++++++++++++++++++++++++++
tests/qtest/Makefile.include | 2 +
3 files changed, 114 insertions(+)
create mode 100644 tests/qtest/sm501-test.c
--
2.21.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH-for-5.0 1/2] hw/display/sm501: Avoid heap overflow in sm501_2d_operation()
2020-04-11 9:14 [PATCH-for-5.0 0/2] hw/display/sm501: Avoid heap overflow in sm501_2d_operation() Philippe Mathieu-Daudé
@ 2020-04-11 9:14 ` Philippe Mathieu-Daudé
2020-04-11 18:05 ` BALATON Zoltan
2020-04-11 9:14 ` [PATCH-for-5.0 2/2] qtest: Test the Drawing Engine of the SM501 companion Philippe Mathieu-Daudé
1 sibling, 1 reply; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-04-11 9:14 UTC (permalink / raw)
To: qemu-devel, BALATON Zoltan
Cc: Laurent Vivier, Thomas Huth, Magnus Damm,
Philippe Mathieu-Daudé,
Zhang Zi Ming, qemu-ppc, Paolo Bonzini
Zhang Zi Ming reported a heap overflow in the Drawing Engine of
the SM501 companion chip model, in particular in the COPY_AREA()
macro in sm501_2d_operation().
As I have no idea what this code is supposed to do, add a simple
check to avoid the heap overflow. This fixes:
=================================================================
==20518==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7f6f4c3fffff at pc 0x55b1e1d358f0 bp 0x7ffce464dfb0 sp 0x7ffce464dfa8
READ of size 1 at 0x7f6f4c3fffff thread T0
#0 0x55b1e1d358ef in sm501_2d_operation hw/display/sm501.c:788:13
#1 0x55b1e1d32c38 in sm501_2d_engine_write hw/display/sm501.c:1466:13
#2 0x55b1e0cd19d8 in memory_region_write_accessor memory.c:483:5
#3 0x55b1e0cd1404 in access_with_adjusted_size memory.c:544:18
#4 0x55b1e0ccfb9d in memory_region_dispatch_write memory.c:1476:16
#5 0x55b1e0ae55a8 in flatview_write_continue exec.c:3125:23
#6 0x55b1e0ad3e87 in flatview_write exec.c:3165:14
#7 0x55b1e0ad3a24 in address_space_write exec.c:3256:18
0x7f6f4c3fffff is located 4194303 bytes to the right of 4194304-byte region [0x7f6f4bc00000,0x7f6f4c000000)
allocated by thread T0 here:
#0 0x55b1e0a6e715 in __interceptor_posix_memalign (ppc64-softmmu/qemu-system-ppc64+0x19c0715)
#1 0x55b1e31c1482 in qemu_try_memalign util/oslib-posix.c:189:11
#2 0x55b1e31c168c in qemu_memalign util/oslib-posix.c:205:27
#3 0x55b1e11a00b3 in spapr_reallocate_hpt hw/ppc/spapr.c:1560:23
#4 0x55b1e11a0ce4 in spapr_setup_hpt hw/ppc/spapr.c:1593:5
#5 0x55b1e11c2fba in spapr_machine_reset hw/ppc/spapr.c:1644:9
#6 0x55b1e1368b01 in qemu_system_reset softmmu/vl.c:1391:9
#7 0x55b1e1375af3 in qemu_init softmmu/vl.c:4436:5
#8 0x55b1e2fc8a59 in main softmmu/main.c:48:5
#9 0x7f6f8150bf42 in __libc_start_main (/lib64/libc.so.6+0x23f42)
SUMMARY: AddressSanitizer: heap-buffer-overflow hw/display/sm501.c:788:13 in sm501_2d_operation
Shadow bytes around the buggy address:
0x0fee69877fa0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0fee69877fb0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0fee69877fc0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0fee69877fd0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0fee69877fe0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0fee69877ff0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa[fa]
0x0fee69878000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0fee69878010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0fee69878020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0fee69878030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0fee69878040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Poisoned by user: f7
ASan internal: fe
==20518==ABORTING
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1786026
Reported-by: Zhang Zi Ming <1015138407@qq.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Per the links on
https://bugzilla.redhat.com/show_bug.cgi?id=1808510 there is probably
a CVE assigned to this, but I don't have access to the information,
https://bugzilla.redhat.com/show_bug.cgi?id=1786593 only show:
You are not authorized to access bug #1786593.
Most likely the bug has been restricted for internal development processes and we cannot grant access.
---
hw/display/sm501.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index de0ab9d977..902acb3875 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -726,6 +726,12 @@ static void sm501_2d_operation(SM501State *s)
int crt = (s->dc_crt_control & SM501_DC_CRT_CONTROL_SEL) ? 1 : 0;
int fb_len = get_width(s, crt) * get_height(s, crt) * get_bpp(s, crt);
+ if (rtl && (src_x < operation_width || src_y < operation_height)) {
+ qemu_log_mask(LOG_GUEST_ERROR, "sm501: Illegal RTL address (%i, %i)\n",
+ src_x, src_y);
+ return;
+ }
+
if (addressing != 0x0) {
printf("%s: only XY addressing is supported.\n", __func__);
abort();
--
2.21.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH-for-5.0 2/2] qtest: Test the Drawing Engine of the SM501 companion
2020-04-11 9:14 [PATCH-for-5.0 0/2] hw/display/sm501: Avoid heap overflow in sm501_2d_operation() Philippe Mathieu-Daudé
2020-04-11 9:14 ` [PATCH-for-5.0 1/2] " Philippe Mathieu-Daudé
@ 2020-04-11 9:14 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-04-11 9:14 UTC (permalink / raw)
To: qemu-devel, BALATON Zoltan
Cc: Laurent Vivier, Thomas Huth, Magnus Damm,
Philippe Mathieu-Daudé,
Zhang Zi Ming, qemu-ppc, Paolo Bonzini
Run some PCI commands to call the COPY_AREA() macro in
sm501_2d_operation(), and verify that there is no more
overflow as reported in BZ#1786026 [*].
The SM501 is used by the R2D-PLUS and aCube Sam460ex
machines, but since it is a PCI card and we already have
an easy way to test PCI daughter cards on the sPAPR
machine, test it there.
[*] https://bugzilla.redhat.com/show_bug.cgi?id=1786026
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
tests/qtest/sm501-test.c | 106 +++++++++++++++++++++++++++++++++++
tests/qtest/Makefile.include | 2 +
2 files changed, 108 insertions(+)
create mode 100644 tests/qtest/sm501-test.c
diff --git a/tests/qtest/sm501-test.c b/tests/qtest/sm501-test.c
new file mode 100644
index 0000000000..79bf7c2c21
--- /dev/null
+++ b/tests/qtest/sm501-test.c
@@ -0,0 +1,106 @@
+/*
+ * QEMU test for the SM501 companion
+ *
+ * SPDX-FileCopyrightText: 2020 Philippe Mathieu-Daudé <f4bug@amsat.org>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#include "qemu/osdep.h"
+#include "qemu/units.h"
+#include "qemu-common.h"
+#include "libqtest.h"
+#include "libqos/libqos-spapr.h"
+#include "hw/pci/pci_ids.h"
+
+typedef struct {
+ QOSState *qs;
+ QPCIDevice *dev;
+ QPCIBar bar;
+} PciSm501State;
+
+static void save_fn(QPCIDevice *dev, int devfn, void *data)
+{
+ *((QPCIDevice **)data) = dev;
+}
+
+static void sm501_init(PciSm501State *s)
+{
+ uint64_t barsize;
+
+ s->dev = NULL;
+ qpci_device_foreach(s->qs->pcibus, PCI_VENDOR_ID_SILICON_MOTION,
+ PCI_DEVICE_ID_SM501, save_fn, &s->dev);
+ g_assert(s->dev != NULL);
+
+ qpci_device_enable(s->dev);
+
+ /* BAR#0: VRAM, BAR#1: MMIO registers */
+ s->bar = qpci_iomap(s->dev, 1, &barsize);
+ g_assert_cmpuint(barsize, ==, 2 * MiB);
+}
+
+static void sm501_deinit(PciSm501State *s)
+{
+ g_free(s->dev);
+}
+
+static uint32_t sm501_read(PciSm501State *s, uint64_t off)
+{
+ uint32_t val;
+
+ s->dev->bus->memread(s->dev->bus, s->bar.addr + off, &val, sizeof(val));
+
+ return val;
+}
+
+static void sm501_write(PciSm501State *s, uint64_t off, uint32_t val)
+{
+ s->dev->bus->memwrite(s->dev->bus, s->bar.addr + off, &val, sizeof(val));
+}
+
+static void sm501_check_device_id(PciSm501State *s)
+{
+ g_assert_cmphex(sm501_read(s, 0x60) >> 16, ==, 0x501); /* DEVICEID reg */
+}
+
+/*
+ * Try to reproduce the heap overflow reported in
+ * https://bugzilla.redhat.com/show_bug.cgi?id=1786026
+ */
+static void test_sm501_2d_drawing_engine_op(void)
+{
+ PciSm501State s;
+
+ s.qs = qtest_spapr_boot("-machine pseries -device sm501");
+
+ sm501_init(&s);
+ sm501_check_device_id(&s);
+
+ /*
+ * Commands listed in BZ#1786026 to access
+ * COPY_AREA() in sm501_2d_operation().
+ */
+ sm501_write(&s, 0x100000, 0x0); /* src: (x, y) = (0, 0) */
+ sm501_write(&s, 0x100004, 0x0); /* dst: (x, y) = (0, 0) */
+ sm501_write(&s, 0x100008, 0x00100010); /* dim: height = width = 16 */
+ sm501_write(&s, 0x100010, 0x00100010); /* pitch: height = width = 16 */
+ sm501_write(&s, 0x10000c, 0xcc000088); /* ctrl: op = copy area, RTL */
+
+ /* If the overflow occured, the next call will fail. */
+ sm501_check_device_id(&s);
+
+ sm501_deinit(&s);
+
+ qtest_shutdown(s.qs);
+}
+
+int main(int argc, char *argv[])
+{
+ g_test_init(&argc, &argv, NULL);
+
+ if (!strcmp(qtest_get_arch(), "ppc64")) {
+ qtest_add_func("spapr/sm501_2d_op", test_sm501_2d_drawing_engine_op);
+ }
+
+ return g_test_run();
+}
diff --git a/tests/qtest/Makefile.include b/tests/qtest/Makefile.include
index 9e5a51d033..7ec894a7a9 100644
--- a/tests/qtest/Makefile.include
+++ b/tests/qtest/Makefile.include
@@ -109,6 +109,7 @@ check-qtest-ppc64-$(CONFIG_VGA) += display-vga-test
check-qtest-ppc64-y += numa-test
check-qtest-ppc64-$(CONFIG_IVSHMEM_DEVICE) += ivshmem-test
check-qtest-ppc64-y += cpu-plug-test
+check-qtest-ppc64-$(CONFIG_SM501) += sm501-test
check-qtest-sh4-$(CONFIG_ISA_TESTDEV) = endianness-test
@@ -253,6 +254,7 @@ tests/qtest/pflash-cfi02$(EXESUF): tests/qtest/pflash-cfi02-test.o
tests/qtest/endianness-test$(EXESUF): tests/qtest/endianness-test.o
tests/qtest/prom-env-test$(EXESUF): tests/qtest/prom-env-test.o $(libqos-obj-y)
tests/qtest/rtas-test$(EXESUF): tests/qtest/rtas-test.o $(libqos-spapr-obj-y)
+tests/qtest/sm501-test$(EXESUF): tests/qtest/sm501-test.o $(libqos-spapr-obj-y)
tests/qtest/fdc-test$(EXESUF): tests/qtest/fdc-test.o
tests/qtest/ide-test$(EXESUF): tests/qtest/ide-test.o $(libqos-pc-obj-y)
tests/qtest/ahci-test$(EXESUF): tests/qtest/ahci-test.o $(libqos-pc-obj-y) qemu-img$(EXESUF)
--
2.21.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH-for-5.0 1/2] hw/display/sm501: Avoid heap overflow in sm501_2d_operation()
2020-04-11 9:14 ` [PATCH-for-5.0 1/2] " Philippe Mathieu-Daudé
@ 2020-04-11 18:05 ` BALATON Zoltan
2020-04-11 19:44 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 9+ messages in thread
From: BALATON Zoltan @ 2020-04-11 18:05 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Laurent Vivier, Thomas Huth, Magnus Damm, qemu-devel,
Zhang Zi Ming, qemu-ppc, Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 5166 bytes --]
On Sat, 11 Apr 2020, Philippe Mathieu-Daudé wrote:
> Zhang Zi Ming reported a heap overflow in the Drawing Engine of
> the SM501 companion chip model, in particular in the COPY_AREA()
> macro in sm501_2d_operation().
>
> As I have no idea what this code is supposed to do, add a simple
> check to avoid the heap overflow. This fixes:
As the function name says it performs a 2D blitter operation. The code had
no bounds checking at all and there are easier ways to crash it by writing
any unimplemented register for which it has abort() calls in the device
implementation. I'm not sure this patch fixes all possible overflows but
at least plugs this particular one so why not.
Acked-by: BALATON Zoltan <balaton@eik.bme.hu>
Otherwise this device emulation should be rewritten sometimes but I had
not time for that.
Regards,
BALATON Zoltan
> =================================================================
> ==20518==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7f6f4c3fffff at pc 0x55b1e1d358f0 bp 0x7ffce464dfb0 sp 0x7ffce464dfa8
> READ of size 1 at 0x7f6f4c3fffff thread T0
> #0 0x55b1e1d358ef in sm501_2d_operation hw/display/sm501.c:788:13
> #1 0x55b1e1d32c38 in sm501_2d_engine_write hw/display/sm501.c:1466:13
> #2 0x55b1e0cd19d8 in memory_region_write_accessor memory.c:483:5
> #3 0x55b1e0cd1404 in access_with_adjusted_size memory.c:544:18
> #4 0x55b1e0ccfb9d in memory_region_dispatch_write memory.c:1476:16
> #5 0x55b1e0ae55a8 in flatview_write_continue exec.c:3125:23
> #6 0x55b1e0ad3e87 in flatview_write exec.c:3165:14
> #7 0x55b1e0ad3a24 in address_space_write exec.c:3256:18
>
> 0x7f6f4c3fffff is located 4194303 bytes to the right of 4194304-byte region [0x7f6f4bc00000,0x7f6f4c000000)
> allocated by thread T0 here:
> #0 0x55b1e0a6e715 in __interceptor_posix_memalign (ppc64-softmmu/qemu-system-ppc64+0x19c0715)
> #1 0x55b1e31c1482 in qemu_try_memalign util/oslib-posix.c:189:11
> #2 0x55b1e31c168c in qemu_memalign util/oslib-posix.c:205:27
> #3 0x55b1e11a00b3 in spapr_reallocate_hpt hw/ppc/spapr.c:1560:23
> #4 0x55b1e11a0ce4 in spapr_setup_hpt hw/ppc/spapr.c:1593:5
> #5 0x55b1e11c2fba in spapr_machine_reset hw/ppc/spapr.c:1644:9
> #6 0x55b1e1368b01 in qemu_system_reset softmmu/vl.c:1391:9
> #7 0x55b1e1375af3 in qemu_init softmmu/vl.c:4436:5
> #8 0x55b1e2fc8a59 in main softmmu/main.c:48:5
> #9 0x7f6f8150bf42 in __libc_start_main (/lib64/libc.so.6+0x23f42)
>
> SUMMARY: AddressSanitizer: heap-buffer-overflow hw/display/sm501.c:788:13 in sm501_2d_operation
> Shadow bytes around the buggy address:
> 0x0fee69877fa0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> 0x0fee69877fb0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> 0x0fee69877fc0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> 0x0fee69877fd0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> 0x0fee69877fe0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> =>0x0fee69877ff0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa[fa]
> 0x0fee69878000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0x0fee69878010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0x0fee69878020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0x0fee69878030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0x0fee69878040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> Shadow byte legend (one shadow byte represents 8 application bytes):
> Addressable: 00
> Partially addressable: 01 02 03 04 05 06 07
> Heap left redzone: fa
> Freed heap region: fd
> Poisoned by user: f7
> ASan internal: fe
> ==20518==ABORTING
>
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1786026
> Reported-by: Zhang Zi Ming <1015138407@qq.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> Per the links on
> https://bugzilla.redhat.com/show_bug.cgi?id=1808510 there is probably
> a CVE assigned to this, but I don't have access to the information,
> https://bugzilla.redhat.com/show_bug.cgi?id=1786593 only show:
>
> You are not authorized to access bug #1786593.
> Most likely the bug has been restricted for internal development processes and we cannot grant access.
> ---
> hw/display/sm501.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index de0ab9d977..902acb3875 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -726,6 +726,12 @@ static void sm501_2d_operation(SM501State *s)
> int crt = (s->dc_crt_control & SM501_DC_CRT_CONTROL_SEL) ? 1 : 0;
> int fb_len = get_width(s, crt) * get_height(s, crt) * get_bpp(s, crt);
>
> + if (rtl && (src_x < operation_width || src_y < operation_height)) {
> + qemu_log_mask(LOG_GUEST_ERROR, "sm501: Illegal RTL address (%i, %i)\n",
> + src_x, src_y);
> + return;
> + }
> +
> if (addressing != 0x0) {
> printf("%s: only XY addressing is supported.\n", __func__);
> abort();
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH-for-5.0 1/2] hw/display/sm501: Avoid heap overflow in sm501_2d_operation()
2020-04-11 18:05 ` BALATON Zoltan
@ 2020-04-11 19:44 ` Philippe Mathieu-Daudé
2020-04-11 21:36 ` Peter Maydell
0 siblings, 1 reply; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-04-11 19:44 UTC (permalink / raw)
To: BALATON Zoltan
Cc: Laurent Vivier, Thomas Huth, Sebastian Bauer, Magnus Damm,
qemu-devel, qemu-stable, Zhang Zi Ming, qemu-ppc, Paolo Bonzini,
Aurelien Jarno
[Cc'ing Sebastian Bauer & Aurelien Jarno because I'm not sure the
problem was introduced by commit debc7e7dad1 or 07d8a50cb0e).
On 4/11/20 8:05 PM, BALATON Zoltan wrote:
> On Sat, 11 Apr 2020, Philippe Mathieu-Daudé wrote:
>> Zhang Zi Ming reported a heap overflow in the Drawing Engine of
>> the SM501 companion chip model, in particular in the COPY_AREA()
>> macro in sm501_2d_operation().
>>
>> As I have no idea what this code is supposed to do, add a simple
>> check to avoid the heap overflow. This fixes:
>
> As the function name says it performs a 2D blitter operation. The code
> had no bounds checking at all and there are easier ways to crash it by
> writing any unimplemented register for which it has abort() calls in the
> device implementation. I'm not sure this patch fixes all possible
> overflows but at least plugs this particular one so why not.
Buffer overflows are security issues because they allow attacker to
arbitrarily write data in the process memory, and eventually take
control of it. When attacker takes control, it can access underlying
private data.
While aborts are pointless to take control, they can still lead to
denial of service (i.e. keeping cloud provider hw busy while rebooting VMs).
I am not sure why 'security researchers' care about the R2D-PLUS and
aCube Sam460ex machines... I suppose they get money each time they
report a such issue.
>
> Acked-by: BALATON Zoltan <balaton@eik.bme.hu>
>
> Otherwise this device emulation should be rewritten sometimes but I had
> not time for that.
It is pointless to implement features we'll never use, so if you are
happy with this fix as it, I'm happy too :) No need to rewrite a device
that works for our use cases.
>
> Regards,
> BALATON Zoltan
>
>> =================================================================
>> ==20518==ERROR: AddressSanitizer: heap-buffer-overflow on address
>> 0x7f6f4c3fffff at pc 0x55b1e1d358f0 bp 0x7ffce464dfb0 sp 0x7ffce464dfa8
>> READ of size 1 at 0x7f6f4c3fffff thread T0
>> #0 0x55b1e1d358ef in sm501_2d_operation hw/display/sm501.c:788:13
>> #1 0x55b1e1d32c38 in sm501_2d_engine_write
>> hw/display/sm501.c:1466:13
>> #2 0x55b1e0cd19d8 in memory_region_write_accessor memory.c:483:5
>> #3 0x55b1e0cd1404 in access_with_adjusted_size memory.c:544:18
>> #4 0x55b1e0ccfb9d in memory_region_dispatch_write memory.c:1476:16
>> #5 0x55b1e0ae55a8 in flatview_write_continue exec.c:3125:23
>> #6 0x55b1e0ad3e87 in flatview_write exec.c:3165:14
>> #7 0x55b1e0ad3a24 in address_space_write exec.c:3256:18
>>
>> 0x7f6f4c3fffff is located 4194303 bytes to the right of 4194304-byte
>> region [0x7f6f4bc00000,0x7f6f4c000000)
>> allocated by thread T0 here:
>> #0 0x55b1e0a6e715 in __interceptor_posix_memalign
>> (ppc64-softmmu/qemu-system-ppc64+0x19c0715)
>> #1 0x55b1e31c1482 in qemu_try_memalign util/oslib-posix.c:189:11
>> #2 0x55b1e31c168c in qemu_memalign util/oslib-posix.c:205:27
>> #3 0x55b1e11a00b3 in spapr_reallocate_hpt hw/ppc/spapr.c:1560:23
>> #4 0x55b1e11a0ce4 in spapr_setup_hpt hw/ppc/spapr.c:1593:5
>> #5 0x55b1e11c2fba in spapr_machine_reset hw/ppc/spapr.c:1644:9
>> #6 0x55b1e1368b01 in qemu_system_reset softmmu/vl.c:1391:9
>> #7 0x55b1e1375af3 in qemu_init softmmu/vl.c:4436:5
>> #8 0x55b1e2fc8a59 in main softmmu/main.c:48:5
>> #9 0x7f6f8150bf42 in __libc_start_main (/lib64/libc.so.6+0x23f42)
>>
>> SUMMARY: AddressSanitizer: heap-buffer-overflow
>> hw/display/sm501.c:788:13 in sm501_2d_operation
>> Shadow bytes around the buggy address:
>> 0x0fee69877fa0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>> 0x0fee69877fb0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>> 0x0fee69877fc0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>> 0x0fee69877fd0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>> 0x0fee69877fe0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>> =>0x0fee69877ff0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa[fa]
>> 0x0fee69878000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> 0x0fee69878010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> 0x0fee69878020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> 0x0fee69878030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> 0x0fee69878040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> Shadow byte legend (one shadow byte represents 8 application bytes):
>> Addressable: 00
>> Partially addressable: 01 02 03 04 05 06 07
>> Heap left redzone: fa
>> Freed heap region: fd
>> Poisoned by user: f7
>> ASan internal: fe
>> ==20518==ABORTING
>>
I forgot here:
Cc: qemu-stable@nongnu.org
>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1786026
>> Reported-by: Zhang Zi Ming <1015138407@qq.com>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> Per the links on
>> https://bugzilla.redhat.com/show_bug.cgi?id=1808510 there is probably
>> a CVE assigned to this, but I don't have access to the information,
>> https://bugzilla.redhat.com/show_bug.cgi?id=1786593 only show:
>>
>> You are not authorized to access bug #1786593.
>> Most likely the bug has been restricted for internal development
>> processes and we cannot grant access.
>> ---
>> hw/display/sm501.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
>> index de0ab9d977..902acb3875 100644
>> --- a/hw/display/sm501.c
>> +++ b/hw/display/sm501.c
>> @@ -726,6 +726,12 @@ static void sm501_2d_operation(SM501State *s)
>> int crt = (s->dc_crt_control & SM501_DC_CRT_CONTROL_SEL) ? 1 : 0;
>> int fb_len = get_width(s, crt) * get_height(s, crt) * get_bpp(s,
>> crt);
>>
>> + if (rtl && (src_x < operation_width || src_y < operation_height)) {
>> + qemu_log_mask(LOG_GUEST_ERROR, "sm501: Illegal RTL address
>> (%i, %i)\n",
>> + src_x, src_y);
>> + return;
>> + }
>> +
>> if (addressing != 0x0) {
>> printf("%s: only XY addressing is supported.\n", __func__);
>> abort();
>>
Thanks Zoltan for the review,
Phil.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH-for-5.0 1/2] hw/display/sm501: Avoid heap overflow in sm501_2d_operation()
2020-04-11 19:44 ` Philippe Mathieu-Daudé
@ 2020-04-11 21:36 ` Peter Maydell
2020-04-12 20:53 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 9+ messages in thread
From: Peter Maydell @ 2020-04-11 21:36 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Laurent Vivier, Thomas Huth, Sebastian Bauer, Magnus Damm,
QEMU Developers, Zhang Zi Ming, qemu-ppc, Paolo Bonzini,
qemu-stable, Aurelien Jarno
On Sat, 11 Apr 2020 at 20:45, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Buffer overflows are security issues because they allow attacker to
> arbitrarily write data in the process memory, and eventually take
> control of it. When attacker takes control, it can access underlying
> private data.
Note that for QEMU our security boundary is "VMs using KVM"; so
buffer overflows are a security issue in code and devices that
you can use in a KVM setup (including pluggable devices like
PCI devices) but not devices you can only use in a TCG setup
(where they're just bugs, though obviously ones we should
fix sooner rather than later).
thanks
-- PMM
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH-for-5.0 1/2] hw/display/sm501: Avoid heap overflow in sm501_2d_operation()
2020-04-11 21:36 ` Peter Maydell
@ 2020-04-12 20:53 ` Philippe Mathieu-Daudé
2020-04-12 20:57 ` Peter Maydell
0 siblings, 1 reply; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-04-12 20:53 UTC (permalink / raw)
To: Peter Maydell
Cc: Laurent Vivier, Thomas Huth, Sebastian Bauer, Magnus Damm,
QEMU Developers, Zhang Zi Ming, qemu-ppc, Paolo Bonzini,
qemu-stable, Aurelien Jarno
On 4/11/20 11:36 PM, Peter Maydell wrote:
> On Sat, 11 Apr 2020 at 20:45, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Buffer overflows are security issues because they allow attacker to
>> arbitrarily write data in the process memory, and eventually take
>> control of it. When attacker takes control, it can access underlying
>> private data.
>
> Note that for QEMU our security boundary is "VMs using KVM"; so
> buffer overflows are a security issue in code and devices that
> you can use in a KVM setup (including pluggable devices like
> PCI devices) but not devices you can only use in a TCG setup
> (where they're just bugs, though obviously ones we should
> fix sooner rather than later).
"VMs using KVM" as security boundary is very clear, thanks.
Note 1: This this doesn't appear on the QEMU security process
description: https://www.qemu.org/contribute/security-process/
Note 2: If a reported bug is not in security boundary, it should be
reported as a bug to mainstream QEMU, to give the community a chance to
fix it.
Regards,
Phil.
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH-for-5.0 1/2] hw/display/sm501: Avoid heap overflow in sm501_2d_operation()
2020-04-12 20:53 ` Philippe Mathieu-Daudé
@ 2020-04-12 20:57 ` Peter Maydell
2020-04-12 21:02 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 9+ messages in thread
From: Peter Maydell @ 2020-04-12 20:57 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Laurent Vivier, Thomas Huth, Sebastian Bauer, Magnus Damm,
QEMU Developers, Zhang Zi Ming, qemu-ppc, Paolo Bonzini,
qemu-stable, Aurelien Jarno
On Sun, 12 Apr 2020 at 21:53, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> "VMs using KVM" as security boundary is very clear, thanks.
>
> Note 1: This this doesn't appear on the QEMU security process
> description: https://www.qemu.org/contribute/security-process/
It's part of the list of how to decide whether an issue is
security sensitive:
"Is QEMU used in conjunction with a hypervisor (as opposed
to TCG binary translation)?"
We also document it in the user manuals now (a relatively
recent improvement):
https://www.qemu.org/docs/master/system/security.html#non-virtualization-use-case
> Note 2: If a reported bug is not in security boundary, it should be
> reported as a bug to mainstream QEMU, to give the community a chance to
> fix it.
Yes; bugs are still bugs.
thanks
-- PMM
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH-for-5.0 1/2] hw/display/sm501: Avoid heap overflow in sm501_2d_operation()
2020-04-12 20:57 ` Peter Maydell
@ 2020-04-12 21:02 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-04-12 21:02 UTC (permalink / raw)
To: Peter Maydell
Cc: Laurent Vivier, Thomas Huth, Sebastian Bauer, Magnus Damm,
QEMU Developers, Zhang Zi Ming, qemu-ppc, Paolo Bonzini,
qemu-stable, Aurelien Jarno
On 4/12/20 10:57 PM, Peter Maydell wrote:
> On Sun, 12 Apr 2020 at 21:53, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> "VMs using KVM" as security boundary is very clear, thanks.
>>
>> Note 1: This this doesn't appear on the QEMU security process
>> description: https://www.qemu.org/contribute/security-process/
>
> It's part of the list of how to decide whether an issue is
> security sensitive:
> "Is QEMU used in conjunction with a hypervisor (as opposed
> to TCG binary translation)?"
Indeed I missed this. This bug correctly matches the example described:
"The ‘generic-sdhci’ interface, instead, had only one user
in ‘Xilinx Zynq Baseboard emulation’ (hw/arm/xilinx_zynq.c).
Xilinx Zynq is a programmable systems on chip (SoC) device.
While QEMU does emulate this device, in practice it is used
to facilitate cross-platform developmental efforts, i.e. QEMU
is used to write programs for the SoC device. In such developer
environments, it is generally assumed that the guest is trusted."
>
> We also document it in the user manuals now (a relatively
> recent improvement):
> https://www.qemu.org/docs/master/system/security.html#non-virtualization-use-case
>
>> Note 2: If a reported bug is not in security boundary, it should be
>> reported as a bug to mainstream QEMU, to give the community a chance to
>> fix it.
>
> Yes; bugs are still bugs.
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-04-12 21:03 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-11 9:14 [PATCH-for-5.0 0/2] hw/display/sm501: Avoid heap overflow in sm501_2d_operation() Philippe Mathieu-Daudé
2020-04-11 9:14 ` [PATCH-for-5.0 1/2] " Philippe Mathieu-Daudé
2020-04-11 18:05 ` BALATON Zoltan
2020-04-11 19:44 ` Philippe Mathieu-Daudé
2020-04-11 21:36 ` Peter Maydell
2020-04-12 20:53 ` Philippe Mathieu-Daudé
2020-04-12 20:57 ` Peter Maydell
2020-04-12 21:02 ` Philippe Mathieu-Daudé
2020-04-11 9:14 ` [PATCH-for-5.0 2/2] qtest: Test the Drawing Engine of the SM501 companion Philippe Mathieu-Daudé
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.