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
> > <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?
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
--
|: 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 :|
