On Tue, May 23, 2023 at 10:16 AM Daniel P. Berrangé <berra...@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 <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 :|
>
--
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0
