[PATCH] block/vpc: Avoid dynamic stack allocation

[PATCH] block/vpc: 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. Here we don't expect the bitmap size to be enormous, and
since we're about to read/write it to disk the overhead of the
allocation should be fine.

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>
---
 block/vpc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index 3810a601a38..ceb87dd3d8e 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -510,7 +510,7 @@ get_image_offset(BlockDriverState *bs, uint64_t offset, bool write, int *err)
        miss sparse read optimization, but it's not a problem in terms of
        correctness. */
     if (write && (s->last_bitmap_offset != bitmap_offset)) {
-        uint8_t bitmap[s->bitmap_size];
+        g_autofree uint8_t *bitmap = g_malloc(s->bitmap_size);
         int r;
 
         s->last_bitmap_offset = bitmap_offset;
@@ -558,7 +558,7 @@ alloc_block(BlockDriverState *bs, int64_t offset)
     int64_t bat_offset;
     uint32_t index, bat_value;
     int ret;
-    uint8_t bitmap[s->bitmap_size];
+    g_autofree uint8_t *bitmap = g_malloc(s->bitmap_size);
 
     /* Check if sector_num is valid */
     if ((offset < 0) || (offset > bs->total_sectors * BDRV_SECTOR_SIZE)) {
-- 
2.34.1

Re: [PATCH] block/vpc: Avoid dynamic stack allocation

[Francisco Iglesias]

On [2023 Aug 11] Fri 18:52:29, Peter Maydell wrote:
> From: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> Use autofree heap allocation instead of variable-length array on the
> stack. Here we don't expect the bitmap size to be enormous, and
> since we're about to read/write it to disk the overhead of the
> allocation should be fine.
> 
> 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>

> ---
>  block/vpc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block/vpc.c b/block/vpc.c
> index 3810a601a38..ceb87dd3d8e 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -510,7 +510,7 @@ get_image_offset(BlockDriverState *bs, uint64_t offset, bool write, int *err)
>         miss sparse read optimization, but it's not a problem in terms of
>         correctness. */
>      if (write && (s->last_bitmap_offset != bitmap_offset)) {
> -        uint8_t bitmap[s->bitmap_size];
> +        g_autofree uint8_t *bitmap = g_malloc(s->bitmap_size);
>          int r;
>  
>          s->last_bitmap_offset = bitmap_offset;
> @@ -558,7 +558,7 @@ alloc_block(BlockDriverState *bs, int64_t offset)
>      int64_t bat_offset;
>      uint32_t index, bat_value;
>      int ret;
> -    uint8_t bitmap[s->bitmap_size];
> +    g_autofree uint8_t *bitmap = g_malloc(s->bitmap_size);
>  
>      /* Check if sector_num is valid */
>      if ((offset < 0) || (offset > bs->total_sectors * BDRV_SECTOR_SIZE)) {
> -- 
> 2.34.1
> 
> 

Re: [PATCH] block/vpc: Avoid dynamic stack allocation

[Kevin Wolf]

Am 11.08.2023 um 19:52 hat Peter Maydell geschrieben:
> From: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> Use autofree heap allocation instead of variable-length array on the
> stack. Here we don't expect the bitmap size to be enormous, and
> since we're about to read/write it to disk the overhead of the
> allocation should be fine.
> 
> 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>

Thanks, applied to the block branch.

Kevin