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

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

[Mauro Matteo Cascella]

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>
Reported-by: Jacek Halon <jacek.halon@gmail.com>
---
 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;
     size_t datasize = width * height * sizeof(uint32_t);
-- 
2.40.1

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

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 2274 bytes --]

Hi

On Mon, May 8, 2023 at 6:21 PM Mauro Matteo Cascella <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>
> Reported-by: Jacek Halon <jacek.halon@gmail.com>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

It looks like this is not exploitable, QXL code uses u16 types, and 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;
>      size_t datasize = width * height * sizeof(uint32_t);
> --
> 2.40.1
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 3243 bytes --]

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

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

[Mauro Matteo Cascella]

On Mon, May 22, 2023 at 8:55 PM Philippe Mathieu-Daudé
<philmd@linaro.org> 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?
>
> > 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?

We currently ensure width/height are less than 512 in cursor_alloc.

Checking for positive values is unnecessary if we make width/height
unsigned, isn't it?

> 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
>
--
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0

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

[Gerd Hoffmann]

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

Modern physical hardware typically uses 512x512 sprites (even if only a
fraction of that is actually needed and >90% are just transparent pixels).

take care,
  Gerd

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

[Daniel P. Berrangé]

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



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

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

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

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

[Philippe Mathieu-Daudé]

On 23/5/23 14:57, Mauro Matteo Cascella wrote:
> 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.

Specifying the word size doesn't really add any (security) value IMHO.

I'll stop bikeshedding here.

Regards,

Phil.

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

[Mauro Matteo Cascella]

On Tue, May 23, 2023 at 4:07 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> On 23/5/23 14:57, Mauro Matteo Cascella wrote:
> > 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.
>
> Specifying the word size doesn't really add any (security) value IMHO.

No security benefit, I know, it just seems more reasonable given what
Gerd said about 512x512 sprites.

> I'll stop bikeshedding here.
>
> Regards,
>
> Phil.
>
--
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0

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

[Michael Tokarev]

08.05.2023 17:18, Mauro Matteo Cascella 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)")

Looks like -stable material too?

Thanks,

/mjt

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

[Mauro Matteo Cascella]

On Mon, May 8, 2023 at 4:20 PM Mauro Matteo Cascella
<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>
> Reported-by: Jacek Halon <jacek.halon@gmail.com>

Addendum:
Reported-by: Yair Mizrahi <yairh33@gmail.com>
Reported-by: Elsayed El-Refa'ei <e.elrefaei99@gmail.com>

> ---
>  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;
>      size_t datasize = width * height * sizeof(uint32_t);
> --
> 2.40.1
>


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

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

[Daniel P. Berrangé]

On Mon, May 08, 2023 at 04:18:13PM +0200, Mauro Matteo Cascella 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.
> 
I concur with Marc-Andre that there is no code path that can
actually trigger an overflow:


  hw/display/ati.c:        s->cursor = cursor_alloc(64, 64);
  hw/display/vhost-user-gpu.c:            s->current_cursor = cursor_alloc(64, 64);
  hw/display/virtio-gpu.c:            s->current_cursor = cursor_alloc(64, 64);

Not exploitable as fixed size

  hw/display/qxl-render.c:    c = cursor_alloc(cursor->header.width, cursor->header.height);

Cursor header defined as:

  typedef struct SPICE_ATTR_PACKED QXLCursorHeader {
      uint64_t unique;
      uint16_t type;
      uint16_t width;
      uint16_t height;
      uint16_t hot_spot_x;
      uint16_t hot_spot_y;
  } QXLCursorHeader;

So no negative values can be passed to cursor_alloc()


  hw/display/vmware_vga.c:    qc = cursor_alloc(c->width, c->height);

Where 'c' is defined as:

  struct vmsvga_cursor_definition_s {
      uint32_t width;
      uint32_t height;
      int id;
      uint32_t bpp;
      int hot_x;
      int hot_y;
      uint32_t mask[1024];
      uint32_t image[4096];
  };

and is also already bounds checked:

            if (cursor.width > 256
                || cursor.height > 256
                || cursor.bpp > 32
                || SVGA_BITMAP_SIZE(x, y) > ARRAY_SIZE(cursor.mask)
                || SVGA_PIXMAP_SIZE(x, y, cursor.bpp)
                    > ARRAY_SIZE(cursor.image)) {
                    goto badcmd;
            }

> Fixes: CVE-2023-1601
> Fixes: fa892e9a ("ui/cursor: fix integer overflow in cursor_alloc (CVE-2021-4206)")

Given there is no possible codepath that can overflow, CVE-2023-1601
looks invalid to me. It should be clsoed as not-a-bug and these two
Fixes lines removed.

> Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
> Reported-by: Jacek Halon <jacek.halon@gmail.com>
> ---
>  include/ui/console.h | 4 ++--
>  ui/cursor.c          | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)

Even though it isn't fixing a bug, the change itself still makes
sense, because there's no reason a negative width/height is ever
appropriate. This protects us against accidentally introducing
future bugs, so with the two CVE Fixes lines removed:

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


> 
> 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;
>      size_t datasize = width * height * sizeof(uint32_t);
> -- 
> 2.40.1
> 
> 

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

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

[Mauro Matteo Cascella]

On Tue, May 23, 2023 at 10:16 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Mon, May 08, 2023 at 04:18:13PM +0200, Mauro Matteo Cascella 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.
> >
> I concur with Marc-Andre that there is no code path that can
> actually trigger an overflow:
>
>
>   hw/display/ati.c:        s->cursor = cursor_alloc(64, 64);
>   hw/display/vhost-user-gpu.c:            s->current_cursor = cursor_alloc(64, 64);
>   hw/display/virtio-gpu.c:            s->current_cursor = cursor_alloc(64, 64);
>
> Not exploitable as fixed size
>
>   hw/display/qxl-render.c:    c = cursor_alloc(cursor->header.width, cursor->header.height);
>
> Cursor header defined as:
>
>   typedef struct SPICE_ATTR_PACKED QXLCursorHeader {
>       uint64_t unique;
>       uint16_t type;
>       uint16_t width;
>       uint16_t height;
>       uint16_t hot_spot_x;
>       uint16_t hot_spot_y;
>   } QXLCursorHeader;
>
> So no negative values can be passed to cursor_alloc()
>
>
>   hw/display/vmware_vga.c:    qc = cursor_alloc(c->width, c->height);
>
> Where 'c' is defined as:
>
>   struct vmsvga_cursor_definition_s {
>       uint32_t width;
>       uint32_t height;
>       int id;
>       uint32_t bpp;
>       int hot_x;
>       int hot_y;
>       uint32_t mask[1024];
>       uint32_t image[4096];
>   };
>
> and is also already bounds checked:
>
>             if (cursor.width > 256
>                 || cursor.height > 256
>                 || cursor.bpp > 32
>                 || SVGA_BITMAP_SIZE(x, y) > ARRAY_SIZE(cursor.mask)
>                 || SVGA_PIXMAP_SIZE(x, y, cursor.bpp)
>                     > ARRAY_SIZE(cursor.image)) {
>                     goto badcmd;
>             }
>
> > Fixes: CVE-2023-1601
> > Fixes: fa892e9a ("ui/cursor: fix integer overflow in cursor_alloc (CVE-2021-4206)")
>
> Given there is no possible codepath that can overflow, CVE-2023-1601
> looks invalid to me. It should be clsoed as not-a-bug and these two
> Fixes lines removed.

I think you can tweak the original PoC [1] to trigger this bug.
Setting width/height to 0x80000000 (versus 0x8000) should do the
trick. You should be able to overflow datasize while bypassing the
sanity check (width > 512 || height > 512) as width/height are signed
prior to this patch. I haven't tested it, though.

[1] https://github.com/star-sg/CVE/blob/master/CVE-2021-4206/poc.c
[2] https://starlabs.sg/advisories/21/21-4206/


> > Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
> > Reported-by: Jacek Halon <jacek.halon@gmail.com>
> > ---
> >  include/ui/console.h | 4 ++--
> >  ui/cursor.c          | 2 +-
> >  2 files changed, 3 insertions(+), 3 deletions(-)
>
> Even though it isn't fixing a bug, the change itself still makes
> sense, because there's no reason a negative width/height is ever
> appropriate. This protects us against accidentally introducing
> future bugs, so with the two CVE Fixes lines removed:
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>
>
> >
> > 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;
> >      size_t datasize = width * height * sizeof(uint32_t);
> > --
> > 2.40.1
> >
> >
>
> 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 :|
>


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

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

[Daniel P. Berrangé]

On Tue, May 23, 2023 at 02:50:09PM +0200, Mauro Matteo Cascella wrote:
> On Tue, May 23, 2023 at 10:16 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Mon, May 08, 2023 at 04:18:13PM +0200, Mauro Matteo Cascella 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.
> > >
> > I concur with Marc-Andre that there is no code path that can
> > actually trigger an overflow:
> >
> >
> >   hw/display/ati.c:        s->cursor = cursor_alloc(64, 64);
> >   hw/display/vhost-user-gpu.c:            s->current_cursor = cursor_alloc(64, 64);
> >   hw/display/virtio-gpu.c:            s->current_cursor = cursor_alloc(64, 64);
> >
> > Not exploitable as fixed size
> >
> >   hw/display/qxl-render.c:    c = cursor_alloc(cursor->header.width, cursor->header.height);
> >
> > Cursor header defined as:
> >
> >   typedef struct SPICE_ATTR_PACKED QXLCursorHeader {
> >       uint64_t unique;
> >       uint16_t type;
> >       uint16_t width;
> >       uint16_t height;
> >       uint16_t hot_spot_x;
> >       uint16_t hot_spot_y;
> >   } QXLCursorHeader;
> >
> > So no negative values can be passed to cursor_alloc()

> >
> > > Fixes: CVE-2023-1601
> > > Fixes: fa892e9a ("ui/cursor: fix integer overflow in cursor_alloc (CVE-2021-4206)")
> >
> > Given there is no possible codepath that can overflow, CVE-2023-1601
> > looks invalid to me. It should be clsoed as not-a-bug and these two
> > Fixes lines removed.
> 
> I think you can tweak the original PoC [1] to trigger this bug.
> Setting width/height to 0x80000000 (versus 0x8000) should do the
> trick. You should be able to overflow datasize while bypassing the
> sanity check (width > 512 || height > 512) as width/height are signed
> prior to this patch. I haven't tested it, though.

The QXLCursorHeader  width/height fields are uint16_t, so 0x80000000
will get truncated. No matter what value the guest sets, when we
interpret this in qxl_cursor when calling cursor_alloc, the value
will be in the range 0-65535, as that's the bounds of uint16_t.

We'll pass this unsigned value to cursor_alloc() which converts from
uint16_t, to (signed) int. 'int' is larger than uint16_t, so the
result will still be positive in the range 0-65535, and so the sanity
check > 512 will fire and protect us.

I still see no bug, let alone a CVE.


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

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

[Mauro Matteo Cascella]

On Tue, May 23, 2023 at 3:03 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Tue, May 23, 2023 at 02:50:09PM +0200, Mauro Matteo Cascella wrote:
> > On Tue, May 23, 2023 at 10:16 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> > >
> > > On Mon, May 08, 2023 at 04:18:13PM +0200, Mauro Matteo Cascella 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.
> > > >
> > > I concur with Marc-Andre that there is no code path that can
> > > actually trigger an overflow:
> > >
> > >
> > >   hw/display/ati.c:        s->cursor = cursor_alloc(64, 64);
> > >   hw/display/vhost-user-gpu.c:            s->current_cursor = cursor_alloc(64, 64);
> > >   hw/display/virtio-gpu.c:            s->current_cursor = cursor_alloc(64, 64);
> > >
> > > Not exploitable as fixed size
> > >
> > >   hw/display/qxl-render.c:    c = cursor_alloc(cursor->header.width, cursor->header.height);
> > >
> > > Cursor header defined as:
> > >
> > >   typedef struct SPICE_ATTR_PACKED QXLCursorHeader {
> > >       uint64_t unique;
> > >       uint16_t type;
> > >       uint16_t width;
> > >       uint16_t height;
> > >       uint16_t hot_spot_x;
> > >       uint16_t hot_spot_y;
> > >   } QXLCursorHeader;
> > >
> > > So no negative values can be passed to cursor_alloc()
>
> > >
> > > > Fixes: CVE-2023-1601
> > > > Fixes: fa892e9a ("ui/cursor: fix integer overflow in cursor_alloc (CVE-2021-4206)")
> > >
> > > Given there is no possible codepath that can overflow, CVE-2023-1601
> > > looks invalid to me. It should be clsoed as not-a-bug and these two
> > > Fixes lines removed.
> >
> > I think you can tweak the original PoC [1] to trigger this bug.
> > Setting width/height to 0x80000000 (versus 0x8000) should do the
> > trick. You should be able to overflow datasize while bypassing the
> > sanity check (width > 512 || height > 512) as width/height are signed
> > prior to this patch. I haven't tested it, though.
>
> The QXLCursorHeader  width/height fields are uint16_t, so 0x80000000
> will get truncated. No matter what value the guest sets, when we
> interpret this in qxl_cursor when calling cursor_alloc, the value
> will be in the range 0-65535, as that's the bounds of uint16_t.
>
> We'll pass this unsigned value to cursor_alloc() which converts from
> uint16_t, to (signed) int. 'int' is larger than uint16_t, so the
> result will still be positive in the range 0-65535, and so the sanity
> check > 512 will fire and protect us.

Oh, you are right! Then yes, feel free to drop the two 'Fixes' lines.
This is more of a hardening bug than a real security issue. I'll
reject the newly assigned CVE.

Thanks,

> I still see no bug, let alone a CVE.
>
>
> 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 :|
>
--
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0