Re: [PATCH 3/3] hw/display/artist: rewrite vram access mode handling

[Gerd Hoffmann]

On Tue, Jan 25, 2022 at 05:29:53PM +0100, Sven Schnelle wrote:
> Gerd Hoffmann <kraxel@redhat.com> writes:
> 
> >   Hi,
> >
> >> +static void artist_vram_write4(ARTISTState *s, struct vram_buffer *buf,
> >> +                               uint32_t offset, uint32_t data)
> >
> > You should pass around offsets not pointers.  An offset can trivially be
> > checked whenever it is within the valid range (i.e. smaller than vram
> > size), or it can be masked to strip off high bits when accessing virtual
> > vram.  You need that for robustness and security reasons (i.e. make sure
> > the guest can't write to host memory by tricking your get_vram_offset
> > calculations).
> 
> I'm not sure i understand the problem. get_vram_offset() returns an
> offset, which is passed to artist_vram_write4() which itself doesn't
> do anything on the buffer. artist_rop8() in the end accesses the buffer,
> and that function checks whether it's < buf->size. Can you elaborate
> a bit more? Maybe it's just so obvious that i don't see it.

Oops.  Sorry,  Didn't look carefully enough.  You are passing on both
buffer and offset, that is fine.  Missed that because the offset comes
after the line wrap, and also not enough coffee ...

I just had a flashback to the series of CVEs we had for the cirrus
blitter, which used to calculate pointers from buffer + offset, then
passed on those pointers to other functions which where unable to
sanity-check those pointers because all context was gone.

take care,
  Gerd