Hi
On Fri, Aug 18, 2023 at 7:11 PM Peter Maydell <peter.mayd...@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.mayd...@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
