| Message ID | 20230818151057.1541189-3-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ui: avoid dynamic stack allocations | expand |
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>
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 > >
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
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(-)