diff mbox

[v3] hw/display/qxl: fix signed to unsigned comparison

Message ID 1390227620-23771-1-git-send-email-alevy@redhat.com
State New
Headers show

Commit Message

Alon Levy Jan. 20, 2014, 2:20 p.m. UTC
Several small signedness / overflow corrections to qxl_create_guest_primary:
1. use 64 bit unsigned for size to avoid overflow possible from two 32
bit multiplicants.
2. correct sign for requested_height
3. add a more verbose error message when setting guest bug state (which
causes a complete guess blackout until reset, so it helps if it is
verbose).

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/display/qxl.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

Peter Maydell Jan. 20, 2014, 2:34 p.m. UTC | #1
On 20 January 2014 14:20, Alon Levy <alevy@redhat.com> wrote:
> Several small signedness / overflow corrections to qxl_create_guest_primary:
> 1. use 64 bit unsigned for size to avoid overflow possible from two 32
> bit multiplicants.
> 2. correct sign for requested_height
> 3. add a more verbose error message when setting guest bug state (which
> causes a complete guess blackout until reset, so it helps if it is
> verbose).
>
> Signed-off-by: Alon Levy <alevy@redhat.com>
> ---
>  hw/display/qxl.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index e4f172e..ba752b5 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -1360,14 +1360,15 @@ static void qxl_create_guest_primary(PCIQXLDevice *qxl, int loadvm,
>  {
>      QXLDevSurfaceCreate surface;
>      QXLSurfaceCreate *sc = &qxl->guest_primary.surface;
> -    int size;
> -    int requested_height = le32_to_cpu(sc->height);
> +    uint64_t size;
> +    uint32_t requested_height = le32_to_cpu(sc->height);
>      int requested_stride = le32_to_cpu(sc->stride);
>
>      size = abs(requested_stride) * requested_height;

This is still doing a 32 x 32 multiply. You need to
make at least one side of the multiply use a 64 bit type
(via cast or by having it be a 64 bit type).

You're also still potentially passing minimum-integer
into abs. That should probably just be caught before we
do the abs() since it will always mean the size is too
large.

thanks
-- PMM
diff mbox

Patch

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index e4f172e..ba752b5 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -1360,14 +1360,15 @@  static void qxl_create_guest_primary(PCIQXLDevice *qxl, int loadvm,
 {
     QXLDevSurfaceCreate surface;
     QXLSurfaceCreate *sc = &qxl->guest_primary.surface;
-    int size;
-    int requested_height = le32_to_cpu(sc->height);
+    uint64_t size;
+    uint32_t requested_height = le32_to_cpu(sc->height);
     int requested_stride = le32_to_cpu(sc->stride);
 
     size = abs(requested_stride) * requested_height;
     if (size > qxl->vgamem_size) {
-        qxl_set_guest_bug(qxl, "%s: requested primary larger then framebuffer"
-                               " size", __func__);
+        qxl_set_guest_bug(qxl, "%s: requested primary larger than framebuffer"
+                               " size %" PRIu64 " > %" PRIu32, __func__, size,
+                               qxl->vgamem_size);
         return;
     }