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

[Laszlo Ersek]

On 04/27/20 13:11, Gerd Hoffmann wrote:
>>> -    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.

OK, thanks. I agree -- if it doesn't break QEMU, then we can let guests
break themselves.

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

OK. If pixman only accesses the "#" marks, then it should be OK.

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

Ugh... Upside down images???... Well, OK, I guess. :)

For the followup patch:

Acked-by: Laszlo Ersek <address@hidden>

Laszlo