Re: [PATCH-for-5.0 1/2] hw/display/sm501: Avoid heap overflow in sm501_2d_operation()

[BALATON Zoltan]

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 <address@hidden>

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 <address@hidden>
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> ---
> 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();