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

[Philippe Mathieu-Daudé]

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

>> 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();
>>

Thanks Zoltan for the review,

Phil.