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é <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()
> >
> > > 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
--
|: 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 :|
