[PATCH 0/3] ui: avoid dynamic stack allocations

[PATCH 0/3] ui: avoid dynamic stack allocations

[Peter Maydell]

The codebase has very few VLAs, and if we can get rid of them all we
can make the compiler error on new additions.  This is a defensive
measure against security bugs where an on-stack dynamic allocation
isn't correctly size-checked (e.g.  CVE-2021-3527).

This patchset fixes some places in the spice and vnc UI frontends
that were doing on-stack allocations. For the vnc-enc-hextile
case we can make the array fixed size; for the other two places
we switch to a heap allocation.

Disclaimer: tested only with compile + make check, which doesn't
actually exercise the UI frontends.

thanks
-- PMM

Peter Maydell (2):
  ui/spice-display: Avoid dynamic stack allocation
  ui/vnc-enc-hextile: Use static rather than dynamic length stack array

Philippe Mathieu-Daudé (1):
  ui/vnc-enc-tight: Avoid dynamic stack allocation

 ui/vnc-enc-hextile-template.h |  8 +++++++-
 ui/spice-display.c            |  3 ++-
 ui/vnc-enc-tight.c            | 11 ++++++-----
 3 files changed, 15 insertions(+), 7 deletions(-)

-- 
2.34.1

[PATCH 1/3] ui/spice-display: Avoid dynamic stack allocation

[Peter Maydell]

Use an autofree heap allocation instead of a variable-length
array on the stack in qemu_spice_create_update().

The codebase has very few VLAs, and if we can get rid of them all we
can make the compiler error on new additions.  This is a defensive
measure against security bugs where an on-stack dynamic allocation
isn't correctly size-checked (e.g.  CVE-2021-3527).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
I was a little unsure about this allocation given that it's
in the display_refresh callback, but the code already does
a g_malloc() every time it calls qemu_spice_create_one_update()
so one more presumably won't hurt.
---
 ui/spice-display.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ui/spice-display.c b/ui/spice-display.c
index 3f3f8013d86..0e2fbfb17c1 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -189,7 +189,7 @@ static void qemu_spice_create_update(SimpleSpiceDisplay *ssd)
 {
     static const int blksize = 32;
     int blocks = DIV_ROUND_UP(surface_width(ssd->ds), blksize);
-    int dirty_top[blocks];
+    g_autofree int *dirty_top = NULL;
     int y, yoff1, yoff2, x, xoff, blk, bw;
     int bpp = surface_bytes_per_pixel(ssd->ds);
     uint8_t *guest, *mirror;
@@ -198,6 +198,7 @@ static void qemu_spice_create_update(SimpleSpiceDisplay *ssd)
         return;
     };
 
+    dirty_top = g_new(int, blocks);
     for (blk = 0; blk < blocks; blk++) {
         dirty_top[blk] = -1;
     }
-- 
2.34.1

Re: [PATCH 1/3] ui/spice-display: Avoid dynamic stack allocation

[Philippe Mathieu-Daudé]

On 18/8/23 17:10, Peter Maydell wrote:
> Use an autofree heap allocation instead of a variable-length
> array on the stack in qemu_spice_create_update().
> 
> The codebase has very few VLAs, and if we can get rid of them all we
> can make the compiler error on new additions.  This is a defensive
> measure against security bugs where an on-stack dynamic allocation
> isn't correctly size-checked (e.g.  CVE-2021-3527).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I was a little unsure about this allocation given that it's
> in the display_refresh callback, but the code already does
> a g_malloc() every time it calls qemu_spice_create_one_update()
> so one more presumably won't hurt.
> ---
>   ui/spice-display.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 2/3] ui/vnc-enc-hextile: Use static rather than dynamic length stack array

[Peter Maydell]

In the send_hextile_tile_* function we create a variable length array
data[].  In fact we know that the client_pf.bytes_per_pixel is at
most 4 (enforced by set_pixel_format()), so we can make the array a
compile-time fixed length of 1536 bytes.

The codebase has very few VLAs, and if we can get rid of them all we
can make the compiler error on new additions.  This is a defensive
measure against security bugs where an on-stack dynamic allocation
isn't correctly size-checked (e.g.  CVE-2021-3527).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 ui/vnc-enc-hextile-template.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/ui/vnc-enc-hextile-template.h b/ui/vnc-enc-hextile-template.h
index 0c56262afff..283c0eaefaf 100644
--- a/ui/vnc-enc-hextile-template.h
+++ b/ui/vnc-enc-hextile-template.h
@@ -7,6 +7,8 @@
 #define NAME BPP
 #endif
 
+#define MAX_CLIENT_BPP 4
+
 static void CONCAT(send_hextile_tile_, NAME)(VncState *vs,
                                              int x, int y, int w, int h,
                                              void *last_bg_,
@@ -25,10 +27,13 @@ static void CONCAT(send_hextile_tile_, NAME)(VncState *vs,
     int bg_count = 0;
     int fg_count = 0;
     int flags = 0;
-    uint8_t data[(vs->client_pf.bytes_per_pixel + 2) * 16 * 16];
+    uint8_t data[(MAX_CLIENT_BPP + 2) * 16 * 16];
     int n_data = 0;
     int n_subtiles = 0;
 
+    /* Enforced by set_pixel_format() */
+    assert(vs->client_pf.bytes_per_pixel <= MAX_CLIENT_BPP);
+
     for (j = 0; j < h; j++) {
         for (i = 0; i < w; i++) {
             switch (n_colors) {
@@ -205,6 +210,7 @@ static void CONCAT(send_hextile_tile_, NAME)(VncState *vs,
     }
 }
 
+#undef MAX_CLIENT_BPP
 #undef NAME
 #undef pixel_t
 #undef CONCAT_I
-- 
2.34.1

Re: [PATCH 2/3] ui/vnc-enc-hextile: Use static rather than dynamic length stack array

[Philippe Mathieu-Daudé]

On 18/8/23 17:10, Peter Maydell wrote:
> In the send_hextile_tile_* function we create a variable length array
> data[].  In fact we know that the client_pf.bytes_per_pixel is at
> most 4 (enforced by set_pixel_format()), so we can make the array a
> compile-time fixed length of 1536 bytes.
> 
> The codebase has very few VLAs, and if we can get rid of them all we
> can make the compiler error on new additions.  This is a defensive
> measure against security bugs where an on-stack dynamic allocation
> isn't correctly size-checked (e.g.  CVE-2021-3527).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   ui/vnc-enc-hextile-template.h | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 2/3] ui/vnc-enc-hextile: Use static rather than dynamic length stack array

[Marc-André Lureau]

Hi

On Fri, Aug 18, 2023 at 7:11 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> In the send_hextile_tile_* function we create a variable length array
> data[].  In fact we know that the client_pf.bytes_per_pixel is at
> most 4 (enforced by set_pixel_format()), so we can make the array a
> compile-time fixed length of 1536 bytes.
>
> The codebase has very few VLAs, and if we can get rid of them all we
> can make the compiler error on new additions.  This is a defensive
> measure against security bugs where an on-stack dynamic allocation
> isn't correctly size-checked (e.g.  CVE-2021-3527).
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  ui/vnc-enc-hextile-template.h | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/ui/vnc-enc-hextile-template.h b/ui/vnc-enc-hextile-template.h
> index 0c56262afff..283c0eaefaf 100644
> --- a/ui/vnc-enc-hextile-template.h
> +++ b/ui/vnc-enc-hextile-template.h
> @@ -7,6 +7,8 @@
>  #define NAME BPP
>  #endif
>
> +#define MAX_CLIENT_BPP 4
> +

BPP is more often used to mean bits per pixel. Do you mind renaming
MAX_BYTES_PER_PIXEL ?


>  static void CONCAT(send_hextile_tile_, NAME)(VncState *vs,
>                                               int x, int y, int w, int h,
>                                               void *last_bg_,
> @@ -25,10 +27,13 @@ static void CONCAT(send_hextile_tile_, NAME)(VncState *vs,
>      int bg_count = 0;
>      int fg_count = 0;
>      int flags = 0;
> -    uint8_t data[(vs->client_pf.bytes_per_pixel + 2) * 16 * 16];
> +    uint8_t data[(MAX_CLIENT_BPP + 2) * 16 * 16];
>      int n_data = 0;
>      int n_subtiles = 0;
>
> +    /* Enforced by set_pixel_format() */
> +    assert(vs->client_pf.bytes_per_pixel <= MAX_CLIENT_BPP);
> +
>      for (j = 0; j < h; j++) {
>          for (i = 0; i < w; i++) {
>              switch (n_colors) {
> @@ -205,6 +210,7 @@ static void CONCAT(send_hextile_tile_, NAME)(VncState *vs,
>      }
>  }
>
> +#undef MAX_CLIENT_BPP
>  #undef NAME
>  #undef pixel_t
>  #undef CONCAT_I
> --
> 2.34.1
>
>


-- 
Marc-André Lureau

[PATCH 3/3] ui/vnc-enc-tight: Avoid dynamic stack allocation

[Peter Maydell]

From: Philippe Mathieu-Daudé <philmd@redhat.com>

Use autofree heap allocation instead of variable-length
array on the stack.

The codebase has very few VLAs, and if we can get rid of them all we
can make the compiler error on new additions.  This is a defensive
measure against security bugs where an on-stack dynamic allocation
isn't correctly size-checked (e.g.  CVE-2021-3527).

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
[PMM: expanded commit message]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 ui/vnc-enc-tight.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/ui/vnc-enc-tight.c b/ui/vnc-enc-tight.c
index ee853dcfcb8..41f559eb837 100644
--- a/ui/vnc-enc-tight.c
+++ b/ui/vnc-enc-tight.c
@@ -1097,13 +1097,13 @@ static int send_palette_rect(VncState *vs, int x, int y,
     switch (vs->client_pf.bytes_per_pixel) {
     case 4:
     {
-        size_t old_offset, offset;
-        uint32_t header[palette_size(palette)];
+        size_t old_offset, offset, palette_sz = palette_size(palette);
+        g_autofree uint32_t *header = g_new(uint32_t, palette_sz);
         struct palette_cb_priv priv = { vs, (uint8_t *)header };
 
         old_offset = vs->output.offset;
         palette_iter(palette, write_palette, &priv);
-        vnc_write(vs, header, sizeof(header));
+        vnc_write(vs, header, palette_sz * sizeof(uint32_t));
 
         if (vs->tight->pixel24) {
             tight_pack24(vs, vs->output.buffer + old_offset, colors, &offset);
@@ -1115,11 +1115,12 @@ static int send_palette_rect(VncState *vs, int x, int y,
     }
     case 2:
     {
-        uint16_t header[palette_size(palette)];
+        size_t palette_sz = palette_size(palette);
+        g_autofree uint16_t *header = g_new(uint16_t, palette_sz);
         struct palette_cb_priv priv = { vs, (uint8_t *)header };
 
         palette_iter(palette, write_palette, &priv);
-        vnc_write(vs, header, sizeof(header));
+        vnc_write(vs, header, palette_sz * sizeof(uint16_t));
         tight_encode_indexed_rect16(vs->tight->tight.buffer, w * h, palette);
         break;
     }
-- 
2.34.1

Re: [PATCH 3/3] ui/vnc-enc-tight: Avoid dynamic stack allocation

[Francisco Iglesias]

On [2023 Aug 18] Fri 16:10:57, Peter Maydell wrote:
> From: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> Use autofree heap allocation instead of variable-length
> array on the stack.
> 
> The codebase has very few VLAs, and if we can get rid of them all we
> can make the compiler error on new additions.  This is a defensive
> measure against security bugs where an on-stack dynamic allocation
> isn't correctly size-checked (e.g.  CVE-2021-3527).
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> [PMM: expanded commit message]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>

> ---
>  ui/vnc-enc-tight.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/ui/vnc-enc-tight.c b/ui/vnc-enc-tight.c
> index ee853dcfcb8..41f559eb837 100644
> --- a/ui/vnc-enc-tight.c
> +++ b/ui/vnc-enc-tight.c
> @@ -1097,13 +1097,13 @@ static int send_palette_rect(VncState *vs, int x, int y,
>      switch (vs->client_pf.bytes_per_pixel) {
>      case 4:
>      {
> -        size_t old_offset, offset;
> -        uint32_t header[palette_size(palette)];
> +        size_t old_offset, offset, palette_sz = palette_size(palette);
> +        g_autofree uint32_t *header = g_new(uint32_t, palette_sz);
>          struct palette_cb_priv priv = { vs, (uint8_t *)header };
>  
>          old_offset = vs->output.offset;
>          palette_iter(palette, write_palette, &priv);
> -        vnc_write(vs, header, sizeof(header));
> +        vnc_write(vs, header, palette_sz * sizeof(uint32_t));
>  
>          if (vs->tight->pixel24) {
>              tight_pack24(vs, vs->output.buffer + old_offset, colors, &offset);
> @@ -1115,11 +1115,12 @@ static int send_palette_rect(VncState *vs, int x, int y,
>      }
>      case 2:
>      {
> -        uint16_t header[palette_size(palette)];
> +        size_t palette_sz = palette_size(palette);
> +        g_autofree uint16_t *header = g_new(uint16_t, palette_sz);
>          struct palette_cb_priv priv = { vs, (uint8_t *)header };
>  
>          palette_iter(palette, write_palette, &priv);
> -        vnc_write(vs, header, sizeof(header));
> +        vnc_write(vs, header, palette_sz * sizeof(uint16_t));
>          tight_encode_indexed_rect16(vs->tight->tight.buffer, w * h, palette);
>          break;
>      }
> -- 
> 2.34.1
> 
>