[Qemu-devel] should we try to stop using variable length arrays?

[Qemu-devel] should we try to stop using variable length arrays?

[Peter Maydell]

Currently QEMU has 9 uses of variable length arrays
(found using -Wvla):

hw/block/dataplane/virtio-blk.c:62:25: warning: variable length array
used [-Wvla]
    unsigned long bitmap[BITS_TO_LONGS(nvqs)];
                        ^
hw/i386/multiboot.c:364:18: warning: variable length array used [-Wvla]
    char kcmdline[strlen(kernel_filename) + strlen(kernel_cmdline) + 2];
                 ^
hw/i386/xen/xen-hvm.c:618:25: warning: variable length array used [-Wvla]
    unsigned long bitmap[DIV_ROUND_UP(npages, width)];
                        ^
hw/intc/xics.c:604:18: warning: variable length array used [-Wvla]
    uint8_t flags[ics->nr_irqs];
                 ^
hw/net/fsl_etsec/rings.c:383:18: warning: variable length array used [-Wvla]
    uint8_t  padd[etsec->rx_padding];
                 ^
hw/ppc/pnv.c:130:26: warning: variable length array used [-Wvla]
    uint32_t servers_prop[smt_threads];
                         ^
hw/ppc/spapr.c:162:26: warning: variable length array used [-Wvla]
    uint32_t servers_prop[smt_threads];
                         ^
hw/ppc/spapr.c:163:27: warning: variable length array used [-Wvla]
    uint32_t gservers_prop[smt_threads * 2];
                          ^
linux-user/syscall.c:3478:23: warning: variable length array used [-Wvla]
    struct sembuf sops[nsops];
                      ^

Should we be looking to get rid of these and turn on the -Wvla
warning? I know the Linux kernel has recently decided to do this
(some rationale at the start of https://lwn.net/Articles/749064/).
Now that doesn't necessarily apply to us as a userspace program,
but on the other hand if any of these were allowing the guest to
determine the size of an on-stack array that would not be great.
(The linux-user one is bogus in that way, though not a security issue
as the guest code there has full control anyway.)

Opinions? I admit that to some extent this is just my sense of
tidiness thinking that if we only have a handful of uses of
something we should squash that down to zero :-)

thanks
-- PMM

Re: [Qemu-devel] should we try to stop using variable length arrays?

[Eric Blake]

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

On 2/7/19 1:30 PM, Peter Maydell wrote:
> Currently QEMU has 9 uses of variable length arrays
> (found using -Wvla):
> 

> 
> Should we be looking to get rid of these and turn on the -Wvla
> warning? I know the Linux kernel has recently decided to do this
> (some rationale at the start of https://lwn.net/Articles/749064/).
> Now that doesn't necessarily apply to us as a userspace program,

But systemd-journal is a userspace program bit by VLA:
https://www.openwall.com/lists/oss-security/2019/01/09/3

So the gnulib project recently switched to making it easier to disable VLA:
https://lists.gnu.org/archive/html/bug-gnulib/2019-01/msg00110.html

> but on the other hand if any of these were allowing the guest to
> determine the size of an on-stack array that would not be great.
> (The linux-user one is bogus in that way, though not a security issue
> as the guest code there has full control anyway.)
> 
> Opinions? I admit that to some extent this is just my sense of
> tidiness thinking that if we only have a handful of uses of
> something we should squash that down to zero :-)

I'm all for removing it. (Hmm, I should update BiteSizedTasks to call
out general compiler-driven cleanups, calling out both -Wshadow and
-Wvla as separate subtasks in that category)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] should we try to stop using variable length arrays?

[Eric Blake]

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

On 2/7/19 1:30 PM, Peter Maydell wrote:
> Currently QEMU has 9 uses of variable length arrays
> (found using -Wvla):
> 

> 
> Should we be looking to get rid of these and turn on the -Wvla
> warning? I know the Linux kernel has recently decided to do this
> (some rationale at the start of https://lwn.net/Articles/749064/).

A big part of that article was a discussion on how hard it is to write
an eval-once macro MAX() that does not trigger a VLA, and that the
kernel settled on a second macro for use in constant contexts.  Do we
want to revist my patch that would add eval-once MAX() and constant-use
MAX_CONST(), to make it easier to avoid accidental VLAs?
 https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg00727.html

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] should we try to stop using variable length arrays?

[Philippe Mathieu-Daudé]

On 2/7/19 8:30 PM, Peter Maydell wrote:
> Currently QEMU has 9 uses of variable length arrays
> (found using -Wvla):
> 
> hw/block/dataplane/virtio-blk.c:62:25: warning: variable length array
> used [-Wvla]
>     unsigned long bitmap[BITS_TO_LONGS(nvqs)];
>                         ^
> hw/i386/multiboot.c:364:18: warning: variable length array used [-Wvla]
>     char kcmdline[strlen(kernel_filename) + strlen(kernel_cmdline) + 2];
>                  ^
> hw/i386/xen/xen-hvm.c:618:25: warning: variable length array used [-Wvla]
>     unsigned long bitmap[DIV_ROUND_UP(npages, width)];
>                         ^
> hw/intc/xics.c:604:18: warning: variable length array used [-Wvla]
>     uint8_t flags[ics->nr_irqs];
>                  ^
> hw/net/fsl_etsec/rings.c:383:18: warning: variable length array used [-Wvla]
>     uint8_t  padd[etsec->rx_padding];
>                  ^
> hw/ppc/pnv.c:130:26: warning: variable length array used [-Wvla]
>     uint32_t servers_prop[smt_threads];
>                          ^
> hw/ppc/spapr.c:162:26: warning: variable length array used [-Wvla]
>     uint32_t servers_prop[smt_threads];
>                          ^
> hw/ppc/spapr.c:163:27: warning: variable length array used [-Wvla]
>     uint32_t gservers_prop[smt_threads * 2];
>                           ^
> linux-user/syscall.c:3478:23: warning: variable length array used [-Wvla]
>     struct sembuf sops[nsops];
>                       ^
> 
> Should we be looking to get rid of these and turn on the -Wvla
> warning? I know the Linux kernel has recently decided to do this

Yes please!

> (some rationale at the start of https://lwn.net/Articles/749064/).
> Now that doesn't necessarily apply to us as a userspace program,
> but on the other hand if any of these were allowing the guest to
> determine the size of an on-stack array that would not be great.
> (The linux-user one is bogus in that way, though not a security issue
> as the guest code there has full control anyway.)

The manpage says "[The semaphore] limit should not be raised above 1000".

> 
> Opinions? I admit that to some extent this is just my sense of
> tidiness thinking that if we only have a handful of uses of
> something we should squash that down to zero :-)

None of these case have strong justification to use the stack rather
than the heap (and the rest of the codebase heavily uses the heap).

> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] should we try to stop using variable length arrays?

[Stefan Hajnoczi]

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

On Thu, Feb 07, 2019 at 07:30:59PM +0000, Peter Maydell wrote:
> Should we be looking to get rid of these and turn on the -Wvla
> warning? I know the Linux kernel has recently decided to do this
> (some rationale at the start of https://lwn.net/Articles/749064/).

I'm in favor of avoiding variable-length arrays just so reviewers don't
need to catch instances where the size comes from the untrusted guest.

Stefan

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

Re: [Qemu-devel] should we try to stop using variable length arrays?

[Daniel P. Berrangé]

On Thu, Feb 07, 2019 at 07:30:59PM +0000, Peter Maydell wrote:
> Currently QEMU has 9 uses of variable length arrays
> (found using -Wvla):
> 
> hw/block/dataplane/virtio-blk.c:62:25: warning: variable length array
> used [-Wvla]
>     unsigned long bitmap[BITS_TO_LONGS(nvqs)];
>                         ^
> hw/i386/multiboot.c:364:18: warning: variable length array used [-Wvla]
>     char kcmdline[strlen(kernel_filename) + strlen(kernel_cmdline) + 2];
>                  ^
> hw/i386/xen/xen-hvm.c:618:25: warning: variable length array used [-Wvla]
>     unsigned long bitmap[DIV_ROUND_UP(npages, width)];
>                         ^
> hw/intc/xics.c:604:18: warning: variable length array used [-Wvla]
>     uint8_t flags[ics->nr_irqs];
>                  ^
> hw/net/fsl_etsec/rings.c:383:18: warning: variable length array used [-Wvla]
>     uint8_t  padd[etsec->rx_padding];
>                  ^
> hw/ppc/pnv.c:130:26: warning: variable length array used [-Wvla]
>     uint32_t servers_prop[smt_threads];
>                          ^
> hw/ppc/spapr.c:162:26: warning: variable length array used [-Wvla]
>     uint32_t servers_prop[smt_threads];
>                          ^
> hw/ppc/spapr.c:163:27: warning: variable length array used [-Wvla]
>     uint32_t gservers_prop[smt_threads * 2];
>                           ^
> linux-user/syscall.c:3478:23: warning: variable length array used [-Wvla]
>     struct sembuf sops[nsops];
>                       ^
> 
> Should we be looking to get rid of these and turn on the -Wvla
> warning? I know the Linux kernel has recently decided to do this
> (some rationale at the start of https://lwn.net/Articles/749064/).
> Now that doesn't necessarily apply to us as a userspace program,
> but on the other hand if any of these were allowing the guest to
> determine the size of an on-stack array that would not be great.
> (The linux-user one is bogus in that way, though not a security issue
> as the guest code there has full control anyway.)
> 
> Opinions? I admit that to some extent this is just my sense of
> tidiness thinking that if we only have a handful of uses of
> something we should squash that down to zero :-)

I think we've got few enough uses of VLA that we would be justified in
removing them so that we can compile time prevent use of the potentially
dangerous feature, even if the current uses are safe.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|