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

[Mauro Matteo Cascella]

On Tue, May 23, 2023 at 10:37 AM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> 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.

I can send v2 with s/uint32_t/uint16_t/ if you think it's a relevant change.

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

-- 
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0