[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 4/5] ramfb: add sanity checks to ramfb_create_display_surface
From: |
Laszlo Ersek |
Subject: |
Re: [PATCH 4/5] ramfb: add sanity checks to ramfb_create_display_surface |
Date: |
Fri, 24 Apr 2020 16:42:20 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 |
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 <address@hidden>
> 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 <address@hidden>
> ---
> 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
[PATCH 3/5] ramfb: don't update RAMFBState on errors, Gerd Hoffmann, 2020/04/22
[PATCH 2/5] Revert "hw/display/ramfb: lock guest resolution after it's set", Gerd Hoffmann, 2020/04/22
[PATCH 1/5] Revert "hw/display/ramfb: initialize fw-config space with xres/ yres", Gerd Hoffmann, 2020/04/22