On 23/5/23 10:09, Daniel P. Berrangé wrote:
On Mon, May 22, 2023 at 08:55:02PM +0200, Philippe Mathieu-Daudé wrote:
On 9/5/23 09:13, Marc-André Lureau wrote:
Hi
On Mon, May 8, 2023 at 6:21 PM Mauro Matteo Cascella
<mcasc...@redhat.com <mailto:mcasc...@redhat.com>> 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.
Fixes: CVE-2023-1601
Fixes: fa892e9a ("ui/cursor: fix integer overflow in cursor_alloc
(CVE-2021-4206)")
Signed-off-by: Mauro Matteo Cascella <mcasc...@redhat.com
<mailto:mcasc...@redhat.com>>
Reported-by: Jacek Halon <jacek.ha...@gmail.com
<mailto:jacek.ha...@gmail.com>>
Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com
<mailto:marcandre.lur...@redhat.com>>
It looks like this is not exploitable, QXL code uses u16 types, and
0xffff * 0xffff * 4 still overflows on 32-bit host, right?
cursor_alloc() will reject 0xffff:
if (width > 512 || height > 512) {
return NULL;
}
I hadn't looked at the source file (the 'datasize' assignation
made me incorrectly think it'd be use before sanitized).
Still I wonder why can't we use a simple 'unsigned' type instead
of a uint32_t, but I won't insist.
VMWare VGA checks for values > 256. Other paths use fixed size.
---
include/ui/console.h | 4 ++--
ui/cursor.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
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;
Can't we check width/height > 0 && <= SOME_LIMIT_THAT_MAKES_SENSE?
Maybe a 16K * 16K cursor is future proof and safe enough.
size_t datasize = width * height * sizeof(uint32_t);
-------------------------------^
-- 2.40.1
--
Marc-André Lureau
With regards,
Daniel
