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

[Gerd Hoffmann <kraxel@redhat.com>]

> > > 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