| Message ID | 20230508141813.1086562-1-mcascell@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ui/cursor: incomplete check for integer overflow in cursor_alloc | expand |
Hi On Mon, May 8, 2023 at 6:21 PM Mauro Matteo Cascella <mcascell@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 <mcascell@redhat.com> > Reported-by: Jacek Halon <jacek.halon@gmail.com> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> It looks like this is not exploitable, QXL code uses u16 types, and 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; > size_t datasize = width * height * sizeof(uint32_t); > -- > 2.40.1 > > >
08.05.2023 17:18, 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. > > Fixes: CVE-2023-1601 > Fixes: fa892e9a ("ui/cursor: fix integer overflow in cursor_alloc (CVE-2021-4206)") Looks like -stable material too? Thanks, /mjt
On Mon, May 8, 2023 at 4:20 PM Mauro Matteo Cascella <mcascell@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 <mcascell@redhat.com> > Reported-by: Jacek Halon <jacek.halon@gmail.com> Addendum: Reported-by: Yair Mizrahi <yairh33@gmail.com> Reported-by: Elsayed El-Refa'ei <e.elrefaei99@gmail.com> > --- > 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; > size_t datasize = width * height * sizeof(uint32_t); > -- > 2.40.1 > -- Mauro Matteo Cascella Red Hat Product Security PGP-Key ID: BB3410B0
On 9/5/23 09:13, Marc-André Lureau wrote: > Hi > > On Mon, May 8, 2023 at 6:21 PM Mauro Matteo Cascella > <mcascell@redhat.com <mailto:mcascell@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 <mcascell@redhat.com > <mailto:mcascell@redhat.com>> > Reported-by: Jacek Halon <jacek.halon@gmail.com > <mailto:jacek.halon@gmail.com>> > > > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com > <mailto:marcandre.lureau@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? 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
On Mon, May 22, 2023 at 8:55 PM Philippe Mathieu-Daudé <philmd@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 > > <mcascell@redhat.com <mailto:mcascell@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 <mcascell@redhat.com > > <mailto:mcascell@redhat.com>> > > Reported-by: Jacek Halon <jacek.halon@gmail.com > > <mailto:jacek.halon@gmail.com>> > > > > > > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com > > <mailto:marcandre.lureau@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
> > -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. Modern physical hardware typically uses 512x512 sprites (even if only a fraction of that is actually needed and >90% are just transparent pixels). take care, Gerd
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 > > <mcascell@redhat.com <mailto:mcascell@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 <mcascell@redhat.com > > <mailto:mcascell@redhat.com>> > > Reported-by: Jacek Halon <jacek.halon@gmail.com > > <mailto:jacek.halon@gmail.com>> > > > > > > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com > > <mailto:marcandre.lureau@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; } > > > 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
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
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 >>> <mcascell@redhat.com <mailto:mcascell@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 <mcascell@redhat.com >>> <mailto:mcascell@redhat.com>> >>> Reported-by: Jacek Halon <jacek.halon@gmail.com >>> <mailto:jacek.halon@gmail.com>> >>> >>> >>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com >>> <mailto:marcandre.lureau@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
On Tue, May 23, 2023 at 10:16 AM Daniel P. Berrangé <berrange@redhat.com> wrote: > > 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. I think you can tweak the original PoC [1] to trigger this bug. Setting width/height to 0x80000000 (versus 0x8000) should do the trick. You should be able to overflow datasize while bypassing the sanity check (width > 512 || height > 512) as width/height are signed prior to this patch. I haven't tested it, though. [1] https://github.com/star-sg/CVE/blob/master/CVE-2021-4206/poc.c [2] https://starlabs.sg/advisories/21/21-4206/ > > 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 :| > -- Mauro Matteo Cascella Red Hat Product Security PGP-Key ID: BB3410B0
On Tue, May 23, 2023 at 10:37 AM Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > 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 > >>> <mcascell@redhat.com <mailto:mcascell@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 <mcascell@redhat.com > >>> <mailto:mcascell@redhat.com>> > >>> Reported-by: Jacek Halon <jacek.halon@gmail.com > >>> <mailto:jacek.halon@gmail.com>> > >>> > >>> > >>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com > >>> <mailto:marcandre.lureau@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. I can send v2 with s/uint32_t/uint16_t/ if you think it's a relevant change. > >> > >>> 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 >
On Tue, May 23, 2023 at 02:50:09PM +0200, Mauro Matteo Cascella wrote: > On Tue, May 23, 2023 at 10:16 AM Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > 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() > > > > > 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. > > I think you can tweak the original PoC [1] to trigger this bug. > Setting width/height to 0x80000000 (versus 0x8000) should do the > trick. You should be able to overflow datasize while bypassing the > sanity check (width > 512 || height > 512) as width/height are signed > prior to this patch. I haven't tested it, though. The QXLCursorHeader width/height fields are uint16_t, so 0x80000000 will get truncated. No matter what value the guest sets, when we interpret this in qxl_cursor when calling cursor_alloc, the value will be in the range 0-65535, as that's the bounds of uint16_t. We'll pass this unsigned value to cursor_alloc() which converts from uint16_t, to (signed) int. 'int' is larger than uint16_t, so the result will still be positive in the range 0-65535, and so the sanity check > 512 will fire and protect us. I still see no bug, let alone a CVE. With regards, Daniel
On 23/5/23 14:57, Mauro Matteo Cascella wrote: > On Tue, May 23, 2023 at 10:37 AM Philippe Mathieu-Daudé > <philmd@linaro.org> wrote: >> >> 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 >>>>> <mcascell@redhat.com <mailto:mcascell@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 <mcascell@redhat.com >>>>> <mailto:mcascell@redhat.com>> >>>>> Reported-by: Jacek Halon <jacek.halon@gmail.com >>>>> <mailto:jacek.halon@gmail.com>> >>>>> >>>>> >>>>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com >>>>> <mailto:marcandre.lureau@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. > > I can send v2 with s/uint32_t/uint16_t/ if you think it's a relevant change. Specifying the word size doesn't really add any (security) value IMHO. I'll stop bikeshedding here. Regards, Phil.
On Tue, May 23, 2023 at 3:03 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Tue, May 23, 2023 at 02:50:09PM +0200, Mauro Matteo Cascella wrote: > > On Tue, May 23, 2023 at 10:16 AM Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > > > 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() > > > > > > > > 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. > > > > I think you can tweak the original PoC [1] to trigger this bug. > > Setting width/height to 0x80000000 (versus 0x8000) should do the > > trick. You should be able to overflow datasize while bypassing the > > sanity check (width > 512 || height > 512) as width/height are signed > > prior to this patch. I haven't tested it, though. > > The QXLCursorHeader width/height fields are uint16_t, so 0x80000000 > will get truncated. No matter what value the guest sets, when we > interpret this in qxl_cursor when calling cursor_alloc, the value > will be in the range 0-65535, as that's the bounds of uint16_t. > > We'll pass this unsigned value to cursor_alloc() which converts from > uint16_t, to (signed) int. 'int' is larger than uint16_t, so the > result will still be positive in the range 0-65535, and so the sanity > check > 512 will fire and protect us. Oh, you are right! Then yes, feel free to drop the two 'Fixes' lines. This is more of a hardening bug than a real security issue. I'll reject the newly assigned CVE. Thanks, > I still see no bug, let alone a CVE. > > > 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 :| > -- Mauro Matteo Cascella Red Hat Product Security PGP-Key ID: BB3410B0
On Tue, May 23, 2023 at 4:07 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > On 23/5/23 14:57, Mauro Matteo Cascella wrote: > > On Tue, May 23, 2023 at 10:37 AM Philippe Mathieu-Daudé > > <philmd@linaro.org> wrote: > >> > >> 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 > >>>>> <mcascell@redhat.com <mailto:mcascell@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 <mcascell@redhat.com > >>>>> <mailto:mcascell@redhat.com>> > >>>>> Reported-by: Jacek Halon <jacek.halon@gmail.com > >>>>> <mailto:jacek.halon@gmail.com>> > >>>>> > >>>>> > >>>>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com > >>>>> <mailto:marcandre.lureau@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. > > > > I can send v2 with s/uint32_t/uint16_t/ if you think it's a relevant change. > > Specifying the word size doesn't really add any (security) value IMHO. No security benefit, I know, it just seems more reasonable given what Gerd said about 512x512 sprites. > I'll stop bikeshedding here. > > Regards, > > Phil. > -- Mauro Matteo Cascella Red Hat Product Security PGP-Key ID: BB3410B0
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);
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 <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(-)