* [PATCH] ui/cursor: fix integer overflow in cursor_alloc (CVE-2022-4206)
@ 2022-04-05 10:32 Mauro Matteo Cascella
2022-04-05 10:55 ` Marc-André Lureau
2022-04-05 11:56 ` Peter Maydell
0 siblings, 2 replies; 6+ messages in thread
From: Mauro Matteo Cascella @ 2022-04-05 10:32 UTC (permalink / raw)
To: qemu-devel; +Cc: mcascell, kraxel
Prevent potential integer overflow by limiting 'width' and 'height' to
512x512. Also change 'datasize' type to size_t. Refer to security
advisory https://starlabs.sg/advisories/22-4206/ for more information.
Fixes: CVE-2022-4206
Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
---
hw/display/qxl-render.c | 7 +++++++
ui/cursor.c | 12 +++++++++++-
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/hw/display/qxl-render.c b/hw/display/qxl-render.c
index d28849b121..dc3c4edd05 100644
--- a/hw/display/qxl-render.c
+++ b/hw/display/qxl-render.c
@@ -247,6 +247,13 @@ static QEMUCursor *qxl_cursor(PCIQXLDevice *qxl, QXLCursor *cursor,
size_t size;
c = cursor_alloc(cursor->header.width, cursor->header.height);
+
+ if (!c) {
+ qxl_set_guest_bug(qxl, "%s: cursor %ux%u alloc error", __func__,
+ cursor->header.width, cursor->header.height);
+ goto fail;
+ }
+
c->hot_x = cursor->header.hot_spot_x;
c->hot_y = cursor->header.hot_spot_y;
switch (cursor->header.type) {
diff --git a/ui/cursor.c b/ui/cursor.c
index 1d62ddd4d0..7cfb08a030 100644
--- a/ui/cursor.c
+++ b/ui/cursor.c
@@ -46,6 +46,13 @@ static QEMUCursor *cursor_parse_xpm(const char *xpm[])
/* parse pixel data */
c = cursor_alloc(width, height);
+
+ if (!c) {
+ fprintf(stderr, "%s: cursor %ux%u alloc error\n",
+ __func__, width, height);
+ return NULL;
+ }
+
for (pixel = 0, y = 0; y < height; y++, line++) {
for (x = 0; x < height; x++, pixel++) {
idx = xpm[line][x];
@@ -91,7 +98,10 @@ QEMUCursor *cursor_builtin_left_ptr(void)
QEMUCursor *cursor_alloc(int width, int height)
{
QEMUCursor *c;
- int datasize = width * height * sizeof(uint32_t);
+ size_t datasize = width * height * sizeof(uint32_t);
+
+ if (width > 512 || height > 512)
+ return NULL;
c = g_malloc0(sizeof(QEMUCursor) + datasize);
c->width = width;
--
2.35.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ui/cursor: fix integer overflow in cursor_alloc (CVE-2022-4206)
2022-04-05 10:32 [PATCH] ui/cursor: fix integer overflow in cursor_alloc (CVE-2022-4206) Mauro Matteo Cascella
@ 2022-04-05 10:55 ` Marc-André Lureau
2022-04-05 11:10 ` Gerd Hoffmann
2022-04-05 11:56 ` Peter Maydell
1 sibling, 1 reply; 6+ messages in thread
From: Marc-André Lureau @ 2022-04-05 10:55 UTC (permalink / raw)
To: Mauro Matteo Cascella; +Cc: QEMU, Gerd Hoffmann
[-- Attachment #1: Type: text/plain, Size: 2426 bytes --]
Hi
On Tue, Apr 5, 2022 at 2:43 PM Mauro Matteo Cascella <mcascell@redhat.com>
wrote:
> Prevent potential integer overflow by limiting 'width' and 'height' to
> 512x512. Also change 'datasize' type to size_t. Refer to security
> advisory https://starlabs.sg/advisories/22-4206/ for more information.
>
> Fixes: CVE-2022-4206
> Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
> ---
> hw/display/qxl-render.c | 7 +++++++
> ui/cursor.c | 12 +++++++++++-
> 2 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/hw/display/qxl-render.c b/hw/display/qxl-render.c
> index d28849b121..dc3c4edd05 100644
> --- a/hw/display/qxl-render.c
> +++ b/hw/display/qxl-render.c
> @@ -247,6 +247,13 @@ static QEMUCursor *qxl_cursor(PCIQXLDevice *qxl,
> QXLCursor *cursor,
> size_t size;
>
> c = cursor_alloc(cursor->header.width, cursor->header.height);
> +
> + if (!c) {
> + qxl_set_guest_bug(qxl, "%s: cursor %ux%u alloc error", __func__,
> + cursor->header.width, cursor->header.height);
> + goto fail;
> + }
> +
> c->hot_x = cursor->header.hot_spot_x;
> c->hot_y = cursor->header.hot_spot_y;
> switch (cursor->header.type) {
> diff --git a/ui/cursor.c b/ui/cursor.c
> index 1d62ddd4d0..7cfb08a030 100644
> --- a/ui/cursor.c
> +++ b/ui/cursor.c
> @@ -46,6 +46,13 @@ static QEMUCursor *cursor_parse_xpm(const char *xpm[])
>
> /* parse pixel data */
> c = cursor_alloc(width, height);
> +
> + if (!c) {
> + fprintf(stderr, "%s: cursor %ux%u alloc error\n",
> + __func__, width, height);
> + return NULL;
> + }
>
I think you could simply abort() in this function. It is used with static
data (ui/cursor*.xpm)
+
> for (pixel = 0, y = 0; y < height; y++, line++) {
> for (x = 0; x < height; x++, pixel++) {
> idx = xpm[line][x];
> @@ -91,7 +98,10 @@ QEMUCursor *cursor_builtin_left_ptr(void)
> QEMUCursor *cursor_alloc(int width, int height)
> {
> QEMUCursor *c;
> - int datasize = width * height * sizeof(uint32_t);
> + size_t datasize = width * height * sizeof(uint32_t);
> +
> + if (width > 512 || height > 512)
> + return NULL;
>
> c = g_malloc0(sizeof(QEMUCursor) + datasize);
> c->width = width;
> --
> 2.35.1
>
>
>
lgtm otherwise
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 3449 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ui/cursor: fix integer overflow in cursor_alloc (CVE-2022-4206)
2022-04-05 10:55 ` Marc-André Lureau
@ 2022-04-05 11:10 ` Gerd Hoffmann
2022-04-05 14:47 ` Mauro Matteo Cascella
0 siblings, 1 reply; 6+ messages in thread
From: Gerd Hoffmann @ 2022-04-05 11:10 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: Mauro Matteo Cascella, QEMU
> > +++ b/ui/cursor.c
> > @@ -46,6 +46,13 @@ static QEMUCursor *cursor_parse_xpm(const char *xpm[])
> >
> > /* parse pixel data */
> > c = cursor_alloc(width, height);
> > +
> > + if (!c) {
> > + fprintf(stderr, "%s: cursor %ux%u alloc error\n",
> > + __func__, width, height);
> > + return NULL;
> > + }
> >
>
> I think you could simply abort() in this function. It is used with static
> data (ui/cursor*.xpm)
Yes, that should never happen.
Missing: vmsvga_cursor_define() calls cursor_alloc() with guest-supplied
values too.
take care,
Gerd
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ui/cursor: fix integer overflow in cursor_alloc (CVE-2022-4206)
2022-04-05 10:32 [PATCH] ui/cursor: fix integer overflow in cursor_alloc (CVE-2022-4206) Mauro Matteo Cascella
2022-04-05 10:55 ` Marc-André Lureau
@ 2022-04-05 11:56 ` Peter Maydell
1 sibling, 0 replies; 6+ messages in thread
From: Peter Maydell @ 2022-04-05 11:56 UTC (permalink / raw)
To: Mauro Matteo Cascella; +Cc: qemu-devel, kraxel
On Tue, 5 Apr 2022 at 11:50, Mauro Matteo Cascella <mcascell@redhat.com> wrote:
>
> Prevent potential integer overflow by limiting 'width' and 'height' to
> 512x512. Also change 'datasize' type to size_t. Refer to security
> advisory https://starlabs.sg/advisories/22-4206/ for more information.
>
> Fixes: CVE-2022-4206
> Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
> diff --git a/ui/cursor.c b/ui/cursor.c
> index 1d62ddd4d0..7cfb08a030 100644
> --- a/ui/cursor.c
> +++ b/ui/cursor.c
> @@ -46,6 +46,13 @@ static QEMUCursor *cursor_parse_xpm(const char *xpm[])
>
> /* parse pixel data */
> c = cursor_alloc(width, height);
> +
> + if (!c) {
> + fprintf(stderr, "%s: cursor %ux%u alloc error\n",
> + __func__, width, height);
> + return NULL;
Side note, we should probably clean up the error handling in
this file to not be "print to stderr" at some point...
> + }
> +
> for (pixel = 0, y = 0; y < height; y++, line++) {
> for (x = 0; x < height; x++, pixel++) {
> idx = xpm[line][x];
> @@ -91,7 +98,10 @@ QEMUCursor *cursor_builtin_left_ptr(void)
> QEMUCursor *cursor_alloc(int width, int height)
> {
> QEMUCursor *c;
> - int datasize = width * height * sizeof(uint32_t);
> + size_t datasize = width * height * sizeof(uint32_t);
> +
> + if (width > 512 || height > 512)
> + return NULL;
Coding style requires braces on if statements.
thanks
-- PMM
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ui/cursor: fix integer overflow in cursor_alloc (CVE-2022-4206)
2022-04-05 11:10 ` Gerd Hoffmann
@ 2022-04-05 14:47 ` Mauro Matteo Cascella
2022-04-06 6:24 ` Gerd Hoffmann
0 siblings, 1 reply; 6+ messages in thread
From: Mauro Matteo Cascella @ 2022-04-05 14:47 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: Marc-André Lureau, QEMU
On Tue, Apr 5, 2022 at 1:10 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> > > +++ b/ui/cursor.c
> > > @@ -46,6 +46,13 @@ static QEMUCursor *cursor_parse_xpm(const char *xpm[])
> > >
> > > /* parse pixel data */
> > > c = cursor_alloc(width, height);
> > > +
> > > + if (!c) {
> > > + fprintf(stderr, "%s: cursor %ux%u alloc error\n",
> > > + __func__, width, height);
> > > + return NULL;
> > > + }
> > >
> >
> > I think you could simply abort() in this function. It is used with static
> > data (ui/cursor*.xpm)
>
> Yes, that should never happen.
>
> Missing: vmsvga_cursor_define() calls cursor_alloc() with guest-supplied
> values too.
I skipped that because the check (cursor.width > 256 || cursor.height
> 256) is already done in vmsvga_fifo_run before calling
vmsvga_cursor_define. You want me to add another check in
vmsvga_cursor_define and return NULL if cursor_alloc fails?
> take care,
> Gerd
>
--
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ui/cursor: fix integer overflow in cursor_alloc (CVE-2022-4206)
2022-04-05 14:47 ` Mauro Matteo Cascella
@ 2022-04-06 6:24 ` Gerd Hoffmann
0 siblings, 0 replies; 6+ messages in thread
From: Gerd Hoffmann @ 2022-04-06 6:24 UTC (permalink / raw)
To: Mauro Matteo Cascella; +Cc: Marc-André Lureau, QEMU
On Tue, Apr 05, 2022 at 04:47:18PM +0200, Mauro Matteo Cascella wrote:
> On Tue, Apr 5, 2022 at 1:10 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> > > > +++ b/ui/cursor.c
> > > > @@ -46,6 +46,13 @@ static QEMUCursor *cursor_parse_xpm(const char *xpm[])
> > > >
> > > > /* parse pixel data */
> > > > c = cursor_alloc(width, height);
> > > > +
> > > > + if (!c) {
> > > > + fprintf(stderr, "%s: cursor %ux%u alloc error\n",
> > > > + __func__, width, height);
> > > > + return NULL;
> > > > + }
> > > >
> > >
> > > I think you could simply abort() in this function. It is used with static
> > > data (ui/cursor*.xpm)
> >
> > Yes, that should never happen.
> >
> > Missing: vmsvga_cursor_define() calls cursor_alloc() with guest-supplied
> > values too.
>
> I skipped that because the check (cursor.width > 256 || cursor.height
> > 256) is already done in vmsvga_fifo_run before calling
> vmsvga_cursor_define. You want me to add another check in
> vmsvga_cursor_define and return NULL if cursor_alloc fails?
No check required then.
Maybe add an 'assert(c != NULL)' to clarify this should never happen
b/c those call sites never call cursor_alloc() with width/height being
too big (your choice, applies to both vmsvga and ui/cursor.c).
take care,
Gerd
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-04-06 6:29 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-05 10:32 [PATCH] ui/cursor: fix integer overflow in cursor_alloc (CVE-2022-4206) Mauro Matteo Cascella
2022-04-05 10:55 ` Marc-André Lureau
2022-04-05 11:10 ` Gerd Hoffmann
2022-04-05 14:47 ` Mauro Matteo Cascella
2022-04-06 6:24 ` Gerd Hoffmann
2022-04-05 11:56 ` Peter Maydell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.