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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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?

> 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