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