Re: [PATCH] ui/cursor: incomplete check for integer overflow in cursor_alloc

[Daniel P . Berrangé]

On Mon, May 08, 2023 at 04:18:13PM +0200, Mauro Matteo Cascella wrote:
> The cursor_alloc function still accepts a signed integer for both the cursor
> width and height. A specially crafted negative width/height could make 
> datasize
> wrap around and cause the next allocation to be 0, potentially leading to a
> heap buffer overflow. Modify QEMUCursor struct and cursor_alloc prototype to
> accept unsigned ints.
> 
I concur with Marc-Andre that there is no code path that can
actually trigger an overflow:


  hw/display/ati.c:        s->cursor = cursor_alloc(64, 64);
  hw/display/vhost-user-gpu.c:            s->current_cursor = cursor_alloc(64, 
64);
  hw/display/virtio-gpu.c:            s->current_cursor = cursor_alloc(64, 64);

Not exploitable as fixed size

  hw/display/qxl-render.c:    c = cursor_alloc(cursor->header.width, 
cursor->header.height);

Cursor header defined as:

  typedef struct SPICE_ATTR_PACKED QXLCursorHeader {
      uint64_t unique;
      uint16_t type;
      uint16_t width;
      uint16_t height;
      uint16_t hot_spot_x;
      uint16_t hot_spot_y;
  } QXLCursorHeader;

So no negative values can be passed to cursor_alloc()


  hw/display/vmware_vga.c:    qc = cursor_alloc(c->width, c->height);

Where 'c' is defined as:

  struct vmsvga_cursor_definition_s {
      uint32_t width;
      uint32_t height;
      int id;
      uint32_t bpp;
      int hot_x;
      int hot_y;
      uint32_t mask[1024];
      uint32_t image[4096];
  };

and is also already bounds checked:

            if (cursor.width > 256
                || cursor.height > 256
                || cursor.bpp > 32
                || SVGA_BITMAP_SIZE(x, y) > ARRAY_SIZE(cursor.mask)
                || SVGA_PIXMAP_SIZE(x, y, cursor.bpp)
                    > ARRAY_SIZE(cursor.image)) {
                    goto badcmd;
            }

> Fixes: CVE-2023-1601
> Fixes: fa892e9a ("ui/cursor: fix integer overflow in cursor_alloc 
> (CVE-2021-4206)")

Given there is no possible codepath that can overflow, CVE-2023-1601
looks invalid to me. It should be clsoed as not-a-bug and these two
Fixes lines removed.

> Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
> Reported-by: Jacek Halon <jacek.halon@gmail.com>
> ---
>  include/ui/console.h | 4 ++--
>  ui/cursor.c          | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)

Even though it isn't fixing a bug, the change itself still makes
sense, because there's no reason a negative width/height is ever
appropriate. This protects us against accidentally introducing
future bugs, so with the two CVE Fixes lines removed:

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


> 
> diff --git a/include/ui/console.h b/include/ui/console.h
> index 2a8fab091f..92a4d90a1b 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -144,13 +144,13 @@ typedef struct QemuUIInfo {
>  
>  /* cursor data format is 32bit RGBA */
>  typedef struct QEMUCursor {
> -    int                 width, height;
> +    uint32_t            width, height;
>      int                 hot_x, hot_y;
>      int                 refcount;
>      uint32_t            data[];
>  } QEMUCursor;
>  
> -QEMUCursor *cursor_alloc(int width, int height);
> +QEMUCursor *cursor_alloc(uint32_t width, uint32_t height);
>  QEMUCursor *cursor_ref(QEMUCursor *c);
>  void cursor_unref(QEMUCursor *c);
>  QEMUCursor *cursor_builtin_hidden(void);
> diff --git a/ui/cursor.c b/ui/cursor.c
> index 6fe67990e2..b5fcb64839 100644
> --- a/ui/cursor.c
> +++ b/ui/cursor.c
> @@ -90,7 +90,7 @@ QEMUCursor *cursor_builtin_left_ptr(void)
>      return cursor_parse_xpm(cursor_left_ptr_xpm);
>  }
>  
> -QEMUCursor *cursor_alloc(int width, int height)
> +QEMUCursor *cursor_alloc(uint32_t width, uint32_t height)
>  {
>      QEMUCursor *c;
>      size_t datasize = width * height * sizeof(uint32_t);
> -- 
> 2.40.1
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|