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

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

[Philippe Mathieu-Daudé]

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

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

Cc: qemu-stable@nongnu.org
Fixes: 07d8a50cb0e ("sm501: add 2D engine copyrect support")
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1786026
Reported-by: Zhang Zi Ming <1015138407@qq.com>
Acked-by: BALATON Zoltan <balaton@eik.bme.hu>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Since v1: Reword description & add Zoltan's A-b.

Test to verify this bug:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg695421.html

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.

Anyway as this code is not used in conjunction with a hypervisor,
it is not a 'security bug' as described in
https://www.qemu.org/contribute/security-process/ and
https://www.qemu.org/docs/master/system/security.html#non-virtualization-use-case
---
 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

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

[Peter Maydell]

On Mon, 13 Apr 2020 at 23:01, Philippe Mathieu-Daudé <f4bug@amsat.org> 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().
>
> Add a simple check to avoid the heap overflow.

> 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;
> +    }

This does fix an issue, but I have a feeling that there are
other possible guest register value combinations that might
cause us to index off one end or the other of the local_mem.

The SM501 datasheet is entirely unhelpful on this question, but
my suggestion is that we should convert the code so that instead
of operating directly on pointers into the middle of the local_mem
buffer all the accesses to local_mem go via functions which mask
off the high bits of the index. That effectively means that the
behaviour if we index off the end of the graphics memory is
that we just wrap round to the start of it. It should be fairly
easy to be confident that the code isn't accessing off the end
of the array and it might even be what the hardware actually does
(since it would correspond to 'use low bits of the address to
index the ram, ignore high bits')...

thanks
-- PMM

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

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2907 bytes --]

On Wed, 15 Apr 2020, Peter Maydell wrote:
> On Mon, 13 Apr 2020 at 23:01, Philippe Mathieu-Daudé <f4bug@amsat.org> 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().
>>
>> Add a simple check to avoid the heap overflow.
>
>> 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;
>> +    }
>
> This does fix an issue, but I have a feeling that there are
> other possible guest register value combinations that might
> cause us to index off one end or the other of the local_mem.

That's what I've meant by it should be reimplemented eventually to fix all 
possible problems but could not do that before 5.0. Since this is existing 
bug ever since this device is first committed not patching it now is 
probably not a big deal if this is not considered a security problem. (And 
if it is then all the abort() calls are probably a problem too although 
less serious.)

> The SM501 datasheet is entirely unhelpful on this question, but
> my suggestion is that we should convert the code so that instead
> of operating directly on pointers into the middle of the local_mem
> buffer all the accesses to local_mem go via functions which mask
> off the high bits of the index. That effectively means that the
> behaviour if we index off the end of the graphics memory is
> that we just wrap round to the start of it. It should be fairly
> easy to be confident that the code isn't accessing off the end
> of the array and it might even be what the hardware actually does
> (since it would correspond to 'use low bits of the address to
> index the ram, ignore high bits')...

Does that make it even slower than it is already? I think it should rather 
be changed to do what I've done in ati_2d.c and call optimised functions 
to do the blit operation instead of implementing it directly. Then we'll 
need checking parameters to avoid overflows. I may try to do that 
eventually but don't know when will I have time for that so if there's 
anyone who submits a patch fixing it some way before that that's OK too.

I also know about these missing ops that could be fixed:

sm501: rop3 mode with rop 99 is not supported.
sm501: rop3 mode with rop ee is not supported.

Regards,
BALATON Zoltan

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

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 3023 bytes --]

On Wed, 15 Apr 2020, BALATON Zoltan wrote:
> On Wed, 15 Apr 2020, Peter Maydell wrote:
>> On Mon, 13 Apr 2020 at 23:01, Philippe Mathieu-Daudé <f4bug@amsat.org> 
>> 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().
>>> 
>>> Add a simple check to avoid the heap overflow.
>> 
>>> 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;
>>> +    }
>> 
>> This does fix an issue, but I have a feeling that there are
>> other possible guest register value combinations that might
>> cause us to index off one end or the other of the local_mem.
>
> That's what I've meant by it should be reimplemented eventually to fix all 
> possible problems but could not do that before 5.0. Since this is existing 
> bug ever since this device is first committed not patching it now is probably 
> not a big deal if this is not considered a security problem. (And if it is 
> then all the abort() calls are probably a problem too although less serious.)
>
>> The SM501 datasheet is entirely unhelpful on this question, but
>> my suggestion is that we should convert the code so that instead
>> of operating directly on pointers into the middle of the local_mem
>> buffer all the accesses to local_mem go via functions which mask
>> off the high bits of the index. That effectively means that the
>> behaviour if we index off the end of the graphics memory is
>> that we just wrap round to the start of it. It should be fairly
>> easy to be confident that the code isn't accessing off the end
>> of the array and it might even be what the hardware actually does
>> (since it would correspond to 'use low bits of the address to
>> index the ram, ignore high bits')...
>
> Does that make it even slower than it is already? I think it should rather be 
> changed to do what I've done in ati_2d.c and call optimised functions to do 
> the blit operation instead of implementing it directly. Then we'll need

As blits are common operation in several video cards, such as sm501, 
cirrus and ati-vga at least maybe we could also split off some common 
helpers to have one implementation of these which could be secured and 
optimised once and not have to fix it in every device separately. I don't 
volunteer to do that by maybe there's someone who wants to try that?

Regards,
BALATON Zoltan

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

[Gerd Hoffmann]

> > > The SM501 datasheet is entirely unhelpful on this question, but
> > > my suggestion is that we should convert the code so that instead
> > > of operating directly on pointers into the middle of the local_mem
> > > buffer all the accesses to local_mem go via functions which mask
> > > off the high bits of the index. That effectively means that the
> > > behaviour if we index off the end of the graphics memory is
> > > that we just wrap round to the start of it. It should be fairly
> > > easy to be confident that the code isn't accessing off the end
> > > of the array and it might even be what the hardware actually does
> > > (since it would correspond to 'use low bits of the address to
> > > index the ram, ignore high bits')...
> > 
> > Does that make it even slower than it is already? I think it should
> > rather be changed to do what I've done in ati_2d.c and call optimised
> > functions to do the blit operation instead of implementing it directly.
> > Then we'll need
> 
> As blits are common operation in several video cards, such as sm501, cirrus
> and ati-vga at least maybe we could also split off some common helpers to
> have one implementation of these which could be secured and optimised once
> and not have to fix it in every device separately. I don't volunteer to do
> that by maybe there's someone who wants to try that?

cirrus stopped using pointers years ago, exactly for the reasons
outlined above.  Conversion was pretty straight forward.

commit 026aeffcb4752054830ba203020ed6eb05bcaba8
Author: Gerd Hoffmann <kraxel@redhat.com>
Date:   Wed Mar 15 11:47:52 2017 +0100

    cirrus: stop passing around dst pointers in the blitter
    
    Instead pass around the address (aka offset into vga memory).  Calculate
    the pointer in the rop_* functions, after applying the mask to the
    address, to make sure the address stays within the valid range.

Drawback of this approach is that it can't be used in case you offload
all your raster ops to pixman, so the basic idea doesn't help much for
ati.  Didn't check sm501.

take care,
  Gerd

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

[Peter Maydell]

On Tue, 21 Apr 2020 at 10:16, Gerd Hoffmann <kraxel@redhat.com> wrote:
> cirrus stopped using pointers years ago, exactly for the reasons
> outlined above.  Conversion was pretty straight forward.
>
> commit 026aeffcb4752054830ba203020ed6eb05bcaba8
> Author: Gerd Hoffmann <kraxel@redhat.com>
> Date:   Wed Mar 15 11:47:52 2017 +0100
>
>     cirrus: stop passing around dst pointers in the blitter
>
>     Instead pass around the address (aka offset into vga memory).  Calculate
>     the pointer in the rop_* functions, after applying the mask to the
>     address, to make sure the address stays within the valid range.

Aha, thanks for bringing up the prior art. (Did anybody benchmark
whether there was a noticeable performance impact for that cirrus
change? My guess is that there wouldn't be much/any because the memory
operations will dominate and you get to do the masking operation more
or less for free, but guesses are notoriously unreliable when it
comes to performance :-) )

thanks
-- PMM

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

[Gerd Hoffmann]

On Tue, Apr 21, 2020 at 10:25:49AM +0100, Peter Maydell wrote:
> On Tue, 21 Apr 2020 at 10:16, Gerd Hoffmann <kraxel@redhat.com> wrote:
> > cirrus stopped using pointers years ago, exactly for the reasons
> > outlined above.  Conversion was pretty straight forward.
> >
> > commit 026aeffcb4752054830ba203020ed6eb05bcaba8
> > Author: Gerd Hoffmann <kraxel@redhat.com>
> > Date:   Wed Mar 15 11:47:52 2017 +0100
> >
> >     cirrus: stop passing around dst pointers in the blitter
> >
> >     Instead pass around the address (aka offset into vga memory).  Calculate
> >     the pointer in the rop_* functions, after applying the mask to the
> >     address, to make sure the address stays within the valid range.
> 
> Aha, thanks for bringing up the prior art. (Did anybody benchmark
> whether there was a noticeable performance impact for that cirrus
> change? My guess is that there wouldn't be much/any because the memory
> operations will dominate and you get to do the masking operation more
> or less for free, but guesses are notoriously unreliable when it
> comes to performance :-) )

In case of the cirrus the first problem is finding an guest which is
old enough that it actually uses the blitter ;)

So, in 99% of the cases the difference is zero due to the blitter not
being used by the guest.  And, no, I don't have numbers for the
remaining 1%.

take care,
  Gerd