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

[Philippe Mathieu-Daudé]

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?

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