[PATCH 0/2] Enable -Wvla, forbidding use of variable length arrays

[PATCH 0/2] Enable -Wvla, forbidding use of variable length arrays

[Peter Maydell]

For a while now I've had an on-and-off-again campaign to get rid of
the handful of uses of C variable-length-array syntax in our
codebase.  The rationale for this is that if the array size can be
controlled by the guest and we don't get the size limit checking
right, this is an easy to exploit security issue.  (An example
problem of this kind from the past is CVE-2021-3527).  Forbidding
them entirely is a defensive measure against further bugs of this
kind.

I submitted a bunch of patches to this effect last year, and
the result is we're now down to just a single use of VLAs, in
a test program. This patchset removes that last VLA usage,
and enables -Wvla in our warning options, so that we will catch
any future attempts to use this C feature.

thanks
-- PMM

Peter Maydell (2):
  tests/qtest/xlnx-versal-trng-test.c: Drop use of variable length array
  meson: Enable -Wvla

 meson.build                         |  1 +
 tests/qtest/xlnx-versal-trng-test.c | 19 +++++++++++--------
 2 files changed, 12 insertions(+), 8 deletions(-)

-- 
2.34.1

[PATCH 1/2] tests/qtest/xlnx-versal-trng-test.c: Drop use of variable length array

[Peter Maydell]

This test program is the last use of any variable length array in the
codebase.  If we can get rid of all uses of VLAs 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).

In this case the test code didn't even want a variable-sized
array, it was just accidentally using syntax that gave it one.
(The array size for C has to be an actual constant expression,
not just something that happens to be known to be constant...)

Remove the VLA usage.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 tests/qtest/xlnx-versal-trng-test.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/tests/qtest/xlnx-versal-trng-test.c b/tests/qtest/xlnx-versal-trng-test.c
index cef4e575bba..ba86f39d13c 100644
--- a/tests/qtest/xlnx-versal-trng-test.c
+++ b/tests/qtest/xlnx-versal-trng-test.c
@@ -298,10 +298,13 @@ static size_t trng_collect(uint32_t *rnd, size_t cnt)
     return i;
 }
 
+/* These tests all generate 512 bits of random data with the device */
+#define TEST_DATA_WORDS (512 / 32)
+
 static void trng_test_autogen(void)
 {
-    const size_t cnt = 512 / 32;
-    uint32_t rng[cnt], prng[cnt];
+    const size_t cnt = TEST_DATA_WORDS;
+    uint32_t rng[TEST_DATA_WORDS], prng[TEST_DATA_WORDS];
     size_t n;
 
     trng_reset();
@@ -343,8 +346,8 @@ static void trng_test_autogen(void)
 
 static void trng_test_oneshot(void)
 {
-    const size_t cnt = 512 / 32;
-    uint32_t rng[cnt];
+    const size_t cnt = TEST_DATA_WORDS;
+    uint32_t rng[TEST_DATA_WORDS];
     size_t n;
 
     trng_reset();
@@ -370,8 +373,8 @@ static void trng_test_oneshot(void)
 
 static void trng_test_per_str(void)
 {
-    const size_t cnt = 512 / 32;
-    uint32_t rng[cnt], prng[cnt];
+    const size_t cnt = TEST_DATA_WORDS;
+    uint32_t rng[TEST_DATA_WORDS], prng[TEST_DATA_WORDS];
     size_t n;
 
     trng_reset();
@@ -415,8 +418,8 @@ static void trng_test_forced_prng(void)
     const char *prop = "forced-prng";
     const uint64_t seed = 0xdeadbeefbad1bad0ULL;
 
-    const size_t cnt = 512 / 32;
-    uint32_t rng[cnt], prng[cnt];
+    const size_t cnt = TEST_DATA_WORDS;
+    uint32_t rng[TEST_DATA_WORDS], prng[TEST_DATA_WORDS];
     size_t n;
 
     trng_reset();
-- 
2.34.1

[PATCH 2/2] meson: Enable -Wvla

[Peter Maydell]

QEMU has historically used variable length arrays only very rarely.
Variable length arrays are a potential security issue where an
on-stack dynamic allocation isn't correctly size-checked, especially
when the size comes from the guest.  (An example problem of this kind
from the past is CVE-2021-3527).  Forbidding them entirely is a
defensive measure against further bugs of this kind.

Enable -Wvla to prevent any new uses from sneaking into the codebase.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 meson.build | 1 +
 1 file changed, 1 insertion(+)

diff --git a/meson.build b/meson.build
index d0329966f1b..385b8247073 100644
--- a/meson.build
+++ b/meson.build
@@ -601,6 +601,7 @@ warn_flags = [
   '-Wno-psabi',
   '-Wno-gnu-variable-sized-type-not-at-end',
   '-Wshadow=local',
+  '-Wvla',
 ]
 
 if host_os != 'darwin'
-- 
2.34.1

Re: [PATCH 1/2] tests/qtest/xlnx-versal-trng-test.c: Drop use of variable length array

[Thomas Huth]

On 25/01/2024 18.32, Peter Maydell wrote:
> This test program is the last use of any variable length array in the
> codebase.  If we can get rid of all uses of VLAs 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).
> 
> In this case the test code didn't even want a variable-sized
> array, it was just accidentally using syntax that gave it one.
> (The array size for C has to be an actual constant expression,
> not just something that happens to be known to be constant...)
> 
> Remove the VLA usage.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   tests/qtest/xlnx-versal-trng-test.c | 19 +++++++++++--------
>   1 file changed, 11 insertions(+), 8 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 2/2] meson: Enable -Wvla

[Thomas Huth]

On 25/01/2024 18.32, Peter Maydell wrote:
> QEMU has historically used variable length arrays only very rarely.
> Variable length arrays are a potential security issue where an
> on-stack dynamic allocation isn't correctly size-checked, especially
> when the size comes from the guest.  (An example problem of this kind
> from the past is CVE-2021-3527).  Forbidding them entirely is a
> defensive measure against further bugs of this kind.
> 
> Enable -Wvla to prevent any new uses from sneaking into the codebase.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   meson.build | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/meson.build b/meson.build
> index d0329966f1b..385b8247073 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -601,6 +601,7 @@ warn_flags = [
>     '-Wno-psabi',
>     '-Wno-gnu-variable-sized-type-not-at-end',
>     '-Wshadow=local',
> +  '-Wvla',
>   ]
>   
>   if host_os != 'darwin'

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 0/2] Enable -Wvla, forbidding use of variable length arrays

[Richard Henderson]

On 1/26/24 03:32, Peter Maydell wrote:
> 
> Peter Maydell (2):
>    tests/qtest/xlnx-versal-trng-test.c: Drop use of variable length array
>    meson: Enable -Wvla

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 1/2] tests/qtest/xlnx-versal-trng-test.c: Drop use of variable length array

[Zhao Liu]

On Thu, Jan 25, 2024 at 05:32:10PM +0000, Peter Maydell wrote:
> Date: Thu, 25 Jan 2024 17:32:10 +0000
> From: Peter Maydell <peter.maydell@linaro.org>
> Subject: [PATCH 1/2] tests/qtest/xlnx-versal-trng-test.c: Drop use of
>  variable length array
> X-Mailer: git-send-email 2.34.1
> 
> This test program is the last use of any variable length array in the
> codebase.  If we can get rid of all uses of VLAs 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).
> 
> In this case the test code didn't even want a variable-sized
> array, it was just accidentally using syntax that gave it one.
> (The array size for C has to be an actual constant expression,
> not just something that happens to be known to be constant...)
> 
> Remove the VLA usage.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  tests/qtest/xlnx-versal-trng-test.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

> 
> diff --git a/tests/qtest/xlnx-versal-trng-test.c b/tests/qtest/xlnx-versal-trng-test.c
> index cef4e575bba..ba86f39d13c 100644
> --- a/tests/qtest/xlnx-versal-trng-test.c
> +++ b/tests/qtest/xlnx-versal-trng-test.c
> @@ -298,10 +298,13 @@ static size_t trng_collect(uint32_t *rnd, size_t cnt)
>      return i;
>  }
>  
> +/* These tests all generate 512 bits of random data with the device */
> +#define TEST_DATA_WORDS (512 / 32)
> +
>  static void trng_test_autogen(void)
>  {
> -    const size_t cnt = 512 / 32;
> -    uint32_t rng[cnt], prng[cnt];
> +    const size_t cnt = TEST_DATA_WORDS;
> +    uint32_t rng[TEST_DATA_WORDS], prng[TEST_DATA_WORDS];
>      size_t n;
>  
>      trng_reset();
> @@ -343,8 +346,8 @@ static void trng_test_autogen(void)
>  
>  static void trng_test_oneshot(void)
>  {
> -    const size_t cnt = 512 / 32;
> -    uint32_t rng[cnt];
> +    const size_t cnt = TEST_DATA_WORDS;
> +    uint32_t rng[TEST_DATA_WORDS];
>      size_t n;
>  
>      trng_reset();
> @@ -370,8 +373,8 @@ static void trng_test_oneshot(void)
>  
>  static void trng_test_per_str(void)
>  {
> -    const size_t cnt = 512 / 32;
> -    uint32_t rng[cnt], prng[cnt];
> +    const size_t cnt = TEST_DATA_WORDS;
> +    uint32_t rng[TEST_DATA_WORDS], prng[TEST_DATA_WORDS];
>      size_t n;
>  
>      trng_reset();
> @@ -415,8 +418,8 @@ static void trng_test_forced_prng(void)
>      const char *prop = "forced-prng";
>      const uint64_t seed = 0xdeadbeefbad1bad0ULL;
>  
> -    const size_t cnt = 512 / 32;
> -    uint32_t rng[cnt], prng[cnt];
> +    const size_t cnt = TEST_DATA_WORDS;
> +    uint32_t rng[TEST_DATA_WORDS], prng[TEST_DATA_WORDS];
>      size_t n;
>  
>      trng_reset();
> -- 
> 2.34.1
> 
> 

Re: [PATCH 2/2] meson: Enable -Wvla

[Zhao Liu]

On Thu, Jan 25, 2024 at 05:32:11PM +0000, Peter Maydell wrote:
> Date: Thu, 25 Jan 2024 17:32:11 +0000
> From: Peter Maydell <peter.maydell@linaro.org>
> Subject: [PATCH 2/2] meson: Enable -Wvla
> X-Mailer: git-send-email 2.34.1
> 
> QEMU has historically used variable length arrays only very rarely.
> Variable length arrays are a potential security issue where an
> on-stack dynamic allocation isn't correctly size-checked, especially
> when the size comes from the guest.  (An example problem of this kind
> from the past is CVE-2021-3527).  Forbidding them entirely is a
> defensive measure against further bugs of this kind.
> 
> Enable -Wvla to prevent any new uses from sneaking into the codebase.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  meson.build | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

> 
> diff --git a/meson.build b/meson.build
> index d0329966f1b..385b8247073 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -601,6 +601,7 @@ warn_flags = [
>    '-Wno-psabi',
>    '-Wno-gnu-variable-sized-type-not-at-end',
>    '-Wshadow=local',
> +  '-Wvla',
>  ]
>  
>  if host_os != 'darwin'
> -- 
> 2.34.1
> 
> 

Re: [PATCH 0/2] Enable -Wvla, forbidding use of variable length arrays

[Thomas Huth]

On 25/01/2024 18.32, Peter Maydell wrote:
> For a while now I've had an on-and-off-again campaign to get rid of
> the handful of uses of C variable-length-array syntax in our
> codebase.  The rationale for this is that if the array size can be
> controlled by the guest and we don't get the size limit checking
> right, this is an easy to exploit security issue.  (An example
> problem of this kind from the past is CVE-2021-3527).  Forbidding
> them entirely is a defensive measure against further bugs of this
> kind.
> 
> I submitted a bunch of patches to this effect last year, and
> the result is we're now down to just a single use of VLAs, in
> a test program. This patchset removes that last VLA usage,
> and enables -Wvla in our warning options, so that we will catch
> any future attempts to use this C feature.
> 
> thanks
> -- PMM
> 
> Peter Maydell (2):
>    tests/qtest/xlnx-versal-trng-test.c: Drop use of variable length array
>    meson: Enable -Wvla
> 
>   meson.build                         |  1 +
>   tests/qtest/xlnx-versal-trng-test.c | 19 +++++++++++--------
>   2 files changed, 12 insertions(+), 8 deletions(-)

There's still a vla left in the ppc kvm code:

  https://gitlab.com/thuth/qemu/-/jobs/6063230079#L2005

../target/ppc/kvm.c: In function ‘kvmppc_save_htab’:
../target/ppc/kvm.c:2691:5: error: ISO C90 forbids variable length array 
‘buf’ [-Werror=vla]
  2691 |     uint8_t buf[bufsize];
       |     ^~~~~~~
../target/ppc/kvm.c: In function ‘kvmppc_read_hptes’:
../target/ppc/kvm.c:2773:9: error: ISO C90 forbids variable length array 
‘buf’ [-Werror=vla]
  2773 |         char buf[sizeof(*hdr) + m * HASH_PTE_SIZE_64];
       |         ^~~~
cc1: all warnings being treated as errors

  Thomas

Re: [PATCH 0/2] Enable -Wvla, forbidding use of variable length arrays

[Peter Maydell]

On Wed, 31 Jan 2024 at 14:56, Thomas Huth <thuth@redhat.com> wrote:
> There's still a vla left in the ppc kvm code:
>
>   https://gitlab.com/thuth/qemu/-/jobs/6063230079#L2005
>
> ../target/ppc/kvm.c: In function ‘kvmppc_save_htab’:
> ../target/ppc/kvm.c:2691:5: error: ISO C90 forbids variable length array
> ‘buf’ [-Werror=vla]
>   2691 |     uint8_t buf[bufsize];
>        |     ^~~~~~~
> ../target/ppc/kvm.c: In function ‘kvmppc_read_hptes’:
> ../target/ppc/kvm.c:2773:9: error: ISO C90 forbids variable length array
> ‘buf’ [-Werror=vla]
>   2773 |         char buf[sizeof(*hdr) + m * HASH_PTE_SIZE_64];
>        |         ^~~~
> cc1: all warnings being treated as errors

Thanks for catching that -- it being in code built only on
ppc hosts I missed it.

kvm_ppc_save_htab() is called twice, and in both cases the
bufsize passed in is MAX_KVM_BUF_SIZE. So we could drop
that argument and have the buf[] array always be MAX_KVM_BUF_SIZE.

kvmppc_read_hptes() does this:
        int m = n < HPTES_PER_GROUP ? n : HPTES_PER_GROUP;
        char buf[sizeof(*hdr) + m * HASH_PTE_SIZE_64];

HPTES_PER_GROUP is 8 and HASH_PTE_SIZE_64 is 16, so we aren't
saving many bytes of stack by trying to make the buf smaller
based on the value of n. So we could have the buf always
be [sizeof(*hdr) + HPTES_PER_GROUP * HASH_PTE_SIZE_64].

thanks
-- PMM