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

[Laszlo Ersek <lersek@redhat.com>]

On 04/23/20 13:41, Gerd Hoffmann wrote:
>   Hi,
> 
>> - if "linesize" is nonzero, make sure it is a whole multiple of the
>> required word size (?)
>>
>> - if "linesize" is nonzero, make sure it is not bogus with relation to
>> "width". I'm thinking something like:
> 
> Yep, the whole linesize is a bit bogus, noticed after sending out this
> series, I have a followup patch for that (see below).
> 
> take care,
>   Gerd
> 
> From 154dcff73dc533fc95c88bd960ed65108af6c734 Mon Sep 17 00:00:00 2001
> From: Gerd Hoffmann <kraxel@redhat.com>
> Date: Wed, 22 Apr 2020 12:07:59 +0200
> Subject: [PATCH] ramfb: fix size calculation
> 
> size calculation isn't correct with guest-supplied stride, the last
> display line isn't accounted for correctly.
> 
> For the typical case of stride > linesize (add padding) we error on the
> safe side (calculated size is larger than actual size).
> 
> With stride < linesize (scanlines overlap) the calculated size is
> smaller than the actual size though so our guest memory mapping might
> end up being too small.
> 
> While being at it also fix ramfb_create_display_surface to use hwaddr
> for the parameters.  That way all calculation are done with hwaddr type
> and we can't get funny effects from type castings.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/display/ramfb.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
> index be884c9ea837..928d74d10bc7 100644
> --- a/hw/display/ramfb.c
> +++ b/hw/display/ramfb.c
> @@ -44,10 +44,10 @@ static void ramfb_unmap_display_surface(pixman_image_t *image, void *unused)
>  
>  static DisplaySurface *ramfb_create_display_surface(int width, int height,
>                                                      pixman_format_code_t format,
> -                                                    int linesize, uint64_t addr)
> +                                                    hwaddr stride, hwaddr addr)
>  {
>      DisplaySurface *surface;
> -    hwaddr size;
> +    hwaddr size, mapsize, linesize;
>      void *data;
>  
>      if (width < 16 || width > VBE_DISPI_MAX_XRES ||
> @@ -55,19 +55,20 @@ static DisplaySurface *ramfb_create_display_surface(int width, int height,
>          format == 0 /* unknown format */)
>          return NULL;
>  
> -    if (linesize == 0) {
> -        linesize = width * PIXMAN_FORMAT_BPP(format) / 8;
> +    linesize = width * PIXMAN_FORMAT_BPP(format) / 8;
> +    if (stride == 0) {
> +        stride = linesize;
>      }
>  
> -    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?

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

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.

Thanks
Laszlo