| Message ID | 20220407081712.345609-1-mcascell@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] ui/cursor: fix integer overflow in cursor_alloc (CVE-2021-4206) | expand |
On Thu, Apr 7, 2022 at 12:23 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-2021-4206 > (the Starlabs advisory has 2022, I guess it's wrong then) Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > v3: > - fix CVE id (CVE-2021-4206 instead of CVE-2022-4206) > > hw/display/qxl-render.c | 7 +++++++ > hw/display/vmware_vga.c | 2 ++ > ui/cursor.c | 8 +++++++- > 3 files changed, 16 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/hw/display/vmware_vga.c b/hw/display/vmware_vga.c > index 98c83474ad..45d06cbe25 100644 > --- a/hw/display/vmware_vga.c > +++ b/hw/display/vmware_vga.c > @@ -515,6 +515,8 @@ static inline void vmsvga_cursor_define(struct > vmsvga_state_s *s, > int i, pixels; > > qc = cursor_alloc(c->width, c->height); > + assert(qc != NULL); > + > qc->hot_x = c->hot_x; > qc->hot_y = c->hot_y; > switch (c->bpp) { > diff --git a/ui/cursor.c b/ui/cursor.c > index 1d62ddd4d0..835f0802f9 100644 > --- a/ui/cursor.c > +++ b/ui/cursor.c > @@ -46,6 +46,8 @@ static QEMUCursor *cursor_parse_xpm(const char *xpm[]) > > /* parse pixel data */ > c = cursor_alloc(width, height); > + assert(c != NULL); > + > for (pixel = 0, y = 0; y < height; y++, line++) { > for (x = 0; x < height; x++, pixel++) { > idx = xpm[line][x]; > @@ -91,7 +93,11 @@ 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 > > >
On Thu, Apr 7, 2022 at 11:17 AM Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > > > > On Thu, Apr 7, 2022 at 12:23 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-2021-4206 > > > (the Starlabs advisory has 2022, I guess it's wrong then) Yep, that is wrong. I asked them to update the page. Thanks. >> Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com> > > > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > >> >> --- >> v3: >> - fix CVE id (CVE-2021-4206 instead of CVE-2022-4206) >> >> hw/display/qxl-render.c | 7 +++++++ >> hw/display/vmware_vga.c | 2 ++ >> ui/cursor.c | 8 +++++++- >> 3 files changed, 16 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/hw/display/vmware_vga.c b/hw/display/vmware_vga.c >> index 98c83474ad..45d06cbe25 100644 >> --- a/hw/display/vmware_vga.c >> +++ b/hw/display/vmware_vga.c >> @@ -515,6 +515,8 @@ static inline void vmsvga_cursor_define(struct vmsvga_state_s *s, >> int i, pixels; >> >> qc = cursor_alloc(c->width, c->height); >> + assert(qc != NULL); >> + >> qc->hot_x = c->hot_x; >> qc->hot_y = c->hot_y; >> switch (c->bpp) { >> diff --git a/ui/cursor.c b/ui/cursor.c >> index 1d62ddd4d0..835f0802f9 100644 >> --- a/ui/cursor.c >> +++ b/ui/cursor.c >> @@ -46,6 +46,8 @@ static QEMUCursor *cursor_parse_xpm(const char *xpm[]) >> >> /* parse pixel data */ >> c = cursor_alloc(width, height); >> + assert(c != NULL); >> + >> for (pixel = 0, y = 0; y < height; y++, line++) { >> for (x = 0; x < height; x++, pixel++) { >> idx = xpm[line][x]; >> @@ -91,7 +93,11 @@ 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 >> >> > > > -- > Marc-André Lureau
On Thu, 7 Apr 2022 at 10:21, Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > > > > On Thu, Apr 7, 2022 at 12:23 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-2021-4206 > > > (the Starlabs advisory has 2022, I guess it's wrong then) > >> Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com> > > > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Does this fix (or any of the other cursor-related stuff I've seen floating past) need to go into 7.0 ? (ie is it release-critical?) thanks -- PMM
On Thu, Apr 07, 2022 at 06:46:00PM +0100, Peter Maydell wrote: > On Thu, 7 Apr 2022 at 10:21, Marc-André Lureau > <marcandre.lureau@gmail.com> wrote: > > > > > > > > On Thu, Apr 7, 2022 at 12:23 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-2021-4206 > > > > > > (the Starlabs advisory has 2022, I guess it's wrong then) > > > >> Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com> > > > > > > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > Does this fix (or any of the other cursor-related stuff I've seen > floating past) need to go into 7.0 ? (ie is it release-critical?) Yes. The integer overflow can be triggered easily by guests. Hitting the double read race condition is harder but probably possible too. Pull request sent minutes ago. take care, Gerd
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/hw/display/vmware_vga.c b/hw/display/vmware_vga.c index 98c83474ad..45d06cbe25 100644 --- a/hw/display/vmware_vga.c +++ b/hw/display/vmware_vga.c @@ -515,6 +515,8 @@ static inline void vmsvga_cursor_define(struct vmsvga_state_s *s, int i, pixels; qc = cursor_alloc(c->width, c->height); + assert(qc != NULL); + qc->hot_x = c->hot_x; qc->hot_y = c->hot_y; switch (c->bpp) { diff --git a/ui/cursor.c b/ui/cursor.c index 1d62ddd4d0..835f0802f9 100644 --- a/ui/cursor.c +++ b/ui/cursor.c @@ -46,6 +46,8 @@ static QEMUCursor *cursor_parse_xpm(const char *xpm[]) /* parse pixel data */ c = cursor_alloc(width, height); + assert(c != NULL); + for (pixel = 0, y = 0; y < height; y++, line++) { for (x = 0; x < height; x++, pixel++) { idx = xpm[line][x]; @@ -91,7 +93,11 @@ 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;
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-2021-4206 Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com> --- v3: - fix CVE id (CVE-2021-4206 instead of CVE-2022-4206) hw/display/qxl-render.c | 7 +++++++ hw/display/vmware_vga.c | 2 ++ ui/cursor.c | 8 +++++++- 3 files changed, 16 insertions(+), 1 deletion(-)