On Mon, May 22, 2023 at 8:55 PM Philippe Mathieu-Daudé <phi...@linaro.org> 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? > > > 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?
We currently ensure width/height are less than 512 in cursor_alloc. Checking for positive values is unnecessary if we make width/height unsigned, isn't it? > 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 > -- Mauro Matteo Cascella Red Hat Product Security PGP-Key ID: BB3410B0
