Re: [PATCH] ui/cursor: incomplete check for integer overflow in cursor_alloc

[Mauro Matteo Cascella]

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