Re: [PATCH 4/5] ramfb: add sanity checks to ramfb_create_display_surface

[Gerd Hoffmann]

> > -    size = (hwaddr)linesize * height;
> > -    data = cpu_physical_memory_map(addr, &size, false);
> > -    if (size != (hwaddr)linesize * height) {
> > -        cpu_physical_memory_unmap(data, size, 0, 0);
> > +    mapsize = size = stride * (height - 1) + linesize;
> > +    data = cpu_physical_memory_map(addr, &mapsize, false);
> > +    if (size != mapsize) {
> > +        cpu_physical_memory_unmap(data, mapsize, 0, 0);
> >          return NULL;
> >      }
> >  
> >      surface = qemu_create_displaysurface_from(width, height,
> > -                                              format, linesize, data);
> > +                                              format, stride, data);
> >      pixman_image_set_destroy_function(surface->image,
> >                                        ramfb_unmap_display_surface, NULL);
> >  
> > 
> 
> I don't understand two things about this patch:
> 
> - "stride" can still be smaller than "linesize" (scanlines can still
> overlap). Why is that not a problem?

Why it should be?  It is the guests choice.  Not a very useful one, but
hey, if the guest prefers it that way we are at your service ...

We only must make sure our size calculations are correct.  The patch
does that.  I think we can also outlaw stride < linesize if you are
happier with that alternative approach.  I doubt we have any guests
relying on this working.

> - assuming "stride > linesize" (i.e., strictly larger), we don't seem to
> map the last complete stride, and that seems to be intentional. Is that
> safe with regard to qemu_create_displaysurface_from()? Ultimately the
> stride is passed to pixman_image_create_bits(), and the underlying
> "data" may not be large enough for that. What am I missing?

Lets take a real-world example.  Wayland rounds up width and height to
multiples of 64 (probably for tiling on modern GPUs).  So with 800x600
you get an allocation of 832x640, like this:

     ###########**   <- y 0
     ###########**
     ###########**
     ###########**
     ###########**   <- y 600
     *************   <- y 640

     ^         ^ ^----- x 832
     |         +------- x 800
     +----------------- x 0

where "#" is image data and "*" is unused padding space.  Pixman will
access all "#", so we are mapping the region from the first "#" to the
last "#", including the unused padding on each scanline, except for the
last scanline.  Any unused scanlines at the bottom are excluded too
(ramfb doesn't even know whenever they exist).

The unused padding is only mapped because it is the easiest way to
handle things, not because we need it.  Also the padding is typically
*alot* smaller than PAGE_SIZE, so we couldn't exclude it from the
mapping even if we would like to ;)

> Hm... bonus question: qemu_create_displaysurface_from() still accepts
> "linesize" as a signed int. (Not sure about pixman_image_create_bits().)
> Should we do something specific to prevent that value from being
> negative? The guest gives us a uint32_t.

Not fully sure we can do that without breaking something, might be a
negative stride is used for upside down images (last scanline comes
first in memory).

take care,
  Gerd