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

[BALATON Zoltan]

On Wed, 15 Apr 2020, Peter Maydell wrote:
> 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.

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