[PATCH 0/2] nvme: avoid dynamic stack allocations

[PATCH 0/2] nvme: avoid dynamic stack allocations

[Peter Maydell]

The QEMU codebase has very few C variable length arrays, 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).

We last had a go at this a few years ago, when Philippe wrote
patches for this:
https://patchew.org/QEMU/20210505211047.1496765-1-philmd@redhat.com/
Some of the fixes made it into the tree, but some didn't (either
because of lack of review or because review found some changes
that needed to be made). I'm going through the remainder as a
non-urgent Friday afternoon task...

This patchset deals with two VLAs in the NVME code.

thanks
-- PMM

Peter Maydell (1):
  hw/nvme: Avoid dynamic stack allocation

Philippe Mathieu-Daudé (1):
  hw/nvme: Use #define to avoid variable length array

 hw/nvme/ctrl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH 1/2] hw/nvme: Use #define to avoid variable length array

[Peter Maydell]

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

In nvme_map_sgl() we create an array segment[] whose size is the
'const int SEG_CHUNK_SIZE'.  Since this is C, rather than C++, a
"const int foo" is not a true constant, it's merely a variable with a
constant value, and so semantically segment[] is a variable-length
array.  Switch SEG_CHUNK_SIZE to a #define so that we can make the
segment[] array truly fixed-size, in the sense that it doesn't
trigger the -Wvla warning.

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: rebased (function has moved file), expand commit message
 based on discussion from previous version of patch]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/nvme/ctrl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 539d2735531..d99a6f5c9a2 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -1045,7 +1045,7 @@ static uint16_t nvme_map_sgl(NvmeCtrl *n, NvmeSg *sg, NvmeSglDescriptor sgl,
      * descriptors and segment chain) than the command transfer size, so it is
      * not bounded by MDTS.
      */
-    const int SEG_CHUNK_SIZE = 256;
+#define SEG_CHUNK_SIZE 256
 
     NvmeSglDescriptor segment[SEG_CHUNK_SIZE], *sgld, *last_sgld;
     uint64_t nsgld;
-- 
2.34.1

[PATCH 2/2] hw/nvme: Avoid dynamic stack allocation

[Peter Maydell]

Instead of using a variable-length array in nvme_map_prp(),
allocate on the stack with a g_autofree pointer.

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>
---
Using the approach suggested by RTH in review of Philippe's
original patch:
 https://patchew.org/QEMU/20210505211047.1496765-1-philmd@redhat.com/20210505211047.1496765-9-philmd@redhat.com/
---
 hw/nvme/ctrl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index d99a6f5c9a2..90687b168ae 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -894,7 +894,7 @@ static uint16_t nvme_map_prp(NvmeCtrl *n, NvmeSg *sg, uint64_t prp1,
     len -= trans_len;
     if (len) {
         if (len > n->page_size) {
-            uint64_t prp_list[n->max_prp_ents];
+            g_autofree uint64_t *prp_list = g_new(uint64_t, n->max_prp_ents);
             uint32_t nents, prp_trans;
             int i = 0;
 
-- 
2.34.1

Re: [PATCH 0/2] nvme: avoid dynamic stack allocations

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 1128 bytes --]

On Aug 11 18:47, Peter Maydell wrote:
> The QEMU codebase has very few C variable length arrays, 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).
> 
> We last had a go at this a few years ago, when Philippe wrote
> patches for this:
> https://patchew.org/QEMU/20210505211047.1496765-1-philmd@redhat.com/
> Some of the fixes made it into the tree, but some didn't (either
> because of lack of review or because review found some changes
> that needed to be made). I'm going through the remainder as a
> non-urgent Friday afternoon task...
> 
> This patchset deals with two VLAs in the NVME code.
> 
> thanks
> -- PMM
> 
> Peter Maydell (1):
>   hw/nvme: Avoid dynamic stack allocation
> 
> Philippe Mathieu-Daudé (1):
>   hw/nvme: Use #define to avoid variable length array
> 
>  hw/nvme/ctrl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> -- 
> 2.34.1
> 

Thanks Peter,

Applied to nvme-next!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/2] nvme: avoid dynamic stack allocations

[Peter Maydell]

On Mon, 14 Aug 2023 at 08:09, Klaus Jensen <its@irrelevant.dk> wrote:
>
> On Aug 11 18:47, Peter Maydell wrote:
> > The QEMU codebase has very few C variable length arrays, 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).
> >
> > We last had a go at this a few years ago, when Philippe wrote
> > patches for this:
> > https://patchew.org/QEMU/20210505211047.1496765-1-philmd@redhat.com/
> > Some of the fixes made it into the tree, but some didn't (either
> > because of lack of review or because review found some changes
> > that needed to be made). I'm going through the remainder as a
> > non-urgent Friday afternoon task...
> >
> > This patchset deals with two VLAs in the NVME code.
> >
> > thanks
> > -- PMM
> >
> > Peter Maydell (1):
> >   hw/nvme: Avoid dynamic stack allocation
> >
> > Philippe Mathieu-Daudé (1):
> >   hw/nvme: Use #define to avoid variable length array
> >
> >  hw/nvme/ctrl.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > --
> > 2.34.1
> >
>
> Thanks Peter,
>
> Applied to nvme-next!


Hi Klaus -- did these patches get lost? They don't seem to
have appeared in master yet.

thanks
-- PMM

Re: [PATCH 0/2] nvme: avoid dynamic stack allocations

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 1582 bytes --]

On Sep 12 15:15, Peter Maydell wrote:
> On Mon, 14 Aug 2023 at 08:09, Klaus Jensen <its@irrelevant.dk> wrote:
> >
> > On Aug 11 18:47, Peter Maydell wrote:
> > > The QEMU codebase has very few C variable length arrays, 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).
> > >
> > > We last had a go at this a few years ago, when Philippe wrote
> > > patches for this:
> > > https://patchew.org/QEMU/20210505211047.1496765-1-philmd@redhat.com/
> > > Some of the fixes made it into the tree, but some didn't (either
> > > because of lack of review or because review found some changes
> > > that needed to be made). I'm going through the remainder as a
> > > non-urgent Friday afternoon task...
> > >
> > > This patchset deals with two VLAs in the NVME code.
> > >
> > > thanks
> > > -- PMM
> > >
> > > Peter Maydell (1):
> > >   hw/nvme: Avoid dynamic stack allocation
> > >
> > > Philippe Mathieu-Daudé (1):
> > >   hw/nvme: Use #define to avoid variable length array
> > >
> > >  hw/nvme/ctrl.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > --
> > > 2.34.1
> > >
> >
> > Thanks Peter,
> >
> > Applied to nvme-next!
> 
> 
> Hi Klaus -- did these patches get lost? They don't seem to
> have appeared in master yet.
> 
> thanks
> -- PMM

Oh. I never sent the pull - I'll do that right away! Thanks for the
reminder!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/2] nvme: avoid dynamic stack allocations

[Philippe Mathieu-Daudé]

On 11/8/23 19:47, Peter Maydell wrote:
> The QEMU codebase has very few C variable length arrays, 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).
> 
> We last had a go at this a few years ago, when Philippe wrote
> patches for this:
> https://patchew.org/QEMU/20210505211047.1496765-1-philmd@redhat.com/
> Some of the fixes made it into the tree, but some didn't (either
> because of lack of review or because review found some changes
> that needed to be made). I'm going through the remainder as a
> non-urgent Friday afternoon task...

Thanks for refreshing this, I totally forgot about it :/

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

> This patchset deals with two VLAs in the NVME code.
> 
> thanks
> -- PMM
> 
> Peter Maydell (1):
>    hw/nvme: Avoid dynamic stack allocation
> 
> Philippe Mathieu-Daudé (1):
>    hw/nvme: Use #define to avoid variable length array
> 
>   hw/nvme/ctrl.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>