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

[Philippe Mathieu-Daudé]

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