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 <mcasc...@redhat.com>
> Reported-by: Jacek Halon <jacek.ha...@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é <berra...@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 :|
