| Message ID | 20200422100211.30614-5-kraxel@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | ramfb: a bunch of reverts and fixes | expand |
On 04/22/20 12:02, Gerd Hoffmann wrote: > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > hw/display/ramfb.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c > index fbe959147dc9..d1b1cb9bb294 100644 > --- a/hw/display/ramfb.c > +++ b/hw/display/ramfb.c > @@ -15,6 +15,7 @@ > #include "qapi/error.h" > #include "hw/loader.h" > #include "hw/display/ramfb.h" > +#include "hw/display/bochs-vbe.h" /* for limits */ > #include "ui/console.h" > #include "sysemu/reset.h" > > @@ -49,6 +50,11 @@ static DisplaySurface *ramfb_create_display_surface(int width, int height, > hwaddr size; > void *data; > > + if (width < 16 || width > VBE_DISPI_MAX_XRES || > + height < 16 || height > VBE_DISPI_MAX_YRES || Seems to make sense (I've checked the constants). > + format == 0 /* unknown format */) OK, this is from qemu_drm_format_to_pixman(). > + return NULL; > + > if (linesize == 0) { > linesize = width * PIXMAN_FORMAT_BPP(format) / 8; > } > I would suggest four more sanity checks: - 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: if (linesize > 0) { min_linesize = width * PIXMAN_FORMAT_BPP(format) / 8; if (linesize < min_linesize) { return NULL; } } May not be the best way to put it, but you get the idea. - We might want to put an upper bound on "linesize" too. I realize "(hwaddr)linesize * height" should be safe in this function, as the multiplication is done in uint64_t. But we also pass "linesize" to qemu_create_displaysurface_from(), and who knows how it is used for multiplication there. - in the ramfb_create_display_surface() function, we should change the type of the parameters "width", "height", and "linesize", from "int" to "uint32_t". In ramfb_fw_cfg_write(), we do take them as uint32_t from the guest, but then pass them as "int"s. And so the current state can produce negative values for any of "width", "height", and "linesize" -- and I'd rather not investigate where those lead. (The new checks catch a negative "width" and "height" already, but not "linesize".) Casting a negative "linesize" to (hwaddr) produces a big, ugly value, FWIW. Thanks Laszlo
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);
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
> > - 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
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 <lersek@redhat.com> Laszlo
Hi, > > 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. :) Well, in the unix world (x11, wayland) x=0,y=0 is the upper left corner. In the windows world x=0,y=0 is the lower left corner, in opengl too. If you don't handle this correctly your guest display might show up upside down ;) qxl uses negative strides to signal that. Looking at the code I see qxl handles this locally (grep for qxl_stride in qxl-render.c), it doesn't propagate into ui/console.c, so it should be safe to change the qemu_create_displaysurface_from() arguments to unsigned from qxl point of view. take care, Gerd
diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c index fbe959147dc9..d1b1cb9bb294 100644 --- a/hw/display/ramfb.c +++ b/hw/display/ramfb.c @@ -15,6 +15,7 @@ #include "qapi/error.h" #include "hw/loader.h" #include "hw/display/ramfb.h" +#include "hw/display/bochs-vbe.h" /* for limits */ #include "ui/console.h" #include "sysemu/reset.h" @@ -49,6 +50,11 @@ static DisplaySurface *ramfb_create_display_surface(int width, int height, hwaddr size; void *data; + if (width < 16 || width > VBE_DISPI_MAX_XRES || + height < 16 || height > VBE_DISPI_MAX_YRES || + format == 0 /* unknown format */) + return NULL; + if (linesize == 0) { linesize = width * PIXMAN_FORMAT_BPP(format) / 8; }
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- hw/display/ramfb.c | 6 ++++++ 1 file changed, 6 insertions(+)