* [PATCH 0/2] nvme: avoid dynamic stack allocations
@ 2023-08-11 17:47 Peter Maydell
2023-08-11 17:47 ` [PATCH 1/2] hw/nvme: Use #define to avoid variable length array Peter Maydell
` (3 more replies)
0 siblings, 4 replies; 7+ messages in thread
From: Peter Maydell @ 2023-08-11 17:47 UTC (permalink / raw
To: qemu-devel
Cc: qemu-block, Philippe Mathieu-Daudé, Keith Busch,
Klaus Jensen
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] hw/nvme: Use #define to avoid variable length array
2023-08-11 17:47 [PATCH 0/2] nvme: avoid dynamic stack allocations Peter Maydell
@ 2023-08-11 17:47 ` Peter Maydell
2023-08-11 17:47 ` [PATCH 2/2] hw/nvme: Avoid dynamic stack allocation Peter Maydell
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Peter Maydell @ 2023-08-11 17:47 UTC (permalink / raw
To: qemu-devel
Cc: qemu-block, Philippe Mathieu-Daudé, Keith Busch,
Klaus Jensen
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] hw/nvme: Avoid dynamic stack allocation
2023-08-11 17:47 [PATCH 0/2] nvme: avoid dynamic stack allocations Peter Maydell
2023-08-11 17:47 ` [PATCH 1/2] hw/nvme: Use #define to avoid variable length array Peter Maydell
@ 2023-08-11 17:47 ` Peter Maydell
2023-08-14 7:09 ` [PATCH 0/2] nvme: avoid dynamic stack allocations Klaus Jensen
2023-08-16 9:47 ` Philippe Mathieu-Daudé
3 siblings, 0 replies; 7+ messages in thread
From: Peter Maydell @ 2023-08-11 17:47 UTC (permalink / raw
To: qemu-devel
Cc: qemu-block, Philippe Mathieu-Daudé, Keith Busch,
Klaus Jensen
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 0/2] nvme: avoid dynamic stack allocations
2023-08-11 17:47 [PATCH 0/2] nvme: avoid dynamic stack allocations Peter Maydell
2023-08-11 17:47 ` [PATCH 1/2] hw/nvme: Use #define to avoid variable length array Peter Maydell
2023-08-11 17:47 ` [PATCH 2/2] hw/nvme: Avoid dynamic stack allocation Peter Maydell
@ 2023-08-14 7:09 ` Klaus Jensen
2023-09-12 14:15 ` Peter Maydell
2023-08-16 9:47 ` Philippe Mathieu-Daudé
3 siblings, 1 reply; 7+ messages in thread
From: Klaus Jensen @ 2023-08-14 7:09 UTC (permalink / raw
To: Peter Maydell
Cc: qemu-devel, qemu-block, Philippe Mathieu-Daudé, Keith Busch
[-- 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 0/2] nvme: avoid dynamic stack allocations
2023-08-11 17:47 [PATCH 0/2] nvme: avoid dynamic stack allocations Peter Maydell
` (2 preceding siblings ...)
2023-08-14 7:09 ` [PATCH 0/2] nvme: avoid dynamic stack allocations Klaus Jensen
@ 2023-08-16 9:47 ` Philippe Mathieu-Daudé
3 siblings, 0 replies; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-08-16 9:47 UTC (permalink / raw
To: Peter Maydell, qemu-devel; +Cc: qemu-block, Keith Busch, Klaus Jensen
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(-)
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 0/2] nvme: avoid dynamic stack allocations
2023-08-14 7:09 ` [PATCH 0/2] nvme: avoid dynamic stack allocations Klaus Jensen
@ 2023-09-12 14:15 ` Peter Maydell
2023-09-12 14:19 ` Klaus Jensen
0 siblings, 1 reply; 7+ messages in thread
From: Peter Maydell @ 2023-09-12 14:15 UTC (permalink / raw
To: Klaus Jensen
Cc: qemu-devel, qemu-block, Philippe Mathieu-Daudé, Keith Busch
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 0/2] nvme: avoid dynamic stack allocations
2023-09-12 14:15 ` Peter Maydell
@ 2023-09-12 14:19 ` Klaus Jensen
0 siblings, 0 replies; 7+ messages in thread
From: Klaus Jensen @ 2023-09-12 14:19 UTC (permalink / raw
To: Peter Maydell
Cc: qemu-devel, qemu-block, Philippe Mathieu-Daudé, Keith Busch
[-- 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-09-12 14:21 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-11 17:47 [PATCH 0/2] nvme: avoid dynamic stack allocations Peter Maydell
2023-08-11 17:47 ` [PATCH 1/2] hw/nvme: Use #define to avoid variable length array Peter Maydell
2023-08-11 17:47 ` [PATCH 2/2] hw/nvme: Avoid dynamic stack allocation Peter Maydell
2023-08-14 7:09 ` [PATCH 0/2] nvme: avoid dynamic stack allocations Klaus Jensen
2023-09-12 14:15 ` Peter Maydell
2023-09-12 14:19 ` Klaus Jensen
2023-08-16 9:47 ` Philippe Mathieu-Daudé
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.