Re: [PATCH] ui/cursor: fix integer overflow in cursor_alloc (CVE-2022-4206)

[Peter Maydell]

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