mbox series

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

Message ID 20240125173211.1786196-1-peter.maydell@linaro.org
Headers show
Series Enable -Wvla, forbidding use of variable length arrays | expand

Message

Peter Maydell Jan. 25, 2024, 5:32 p.m. UTC 
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(-)

Comments

Richard Henderson Jan. 27, 2024, 3:41 a.m. UTC | #1 
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~
Thomas Huth Jan. 31, 2024, 2:55 p.m. UTC | #2 
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
Peter Maydell Jan. 31, 2024, 8 p.m. UTC | #3 
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