* Re: [Qemu-devel] should we try to stop using variable length arrays?
2019-02-07 19:30 [Qemu-devel] should we try to stop using variable length arrays? Peter Maydell
@ 2019-02-07 19:39 ` Eric Blake
2019-02-07 20:28 ` Eric Blake
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Eric Blake @ 2019-02-07 19:39 UTC (permalink / raw)
To: Peter Maydell, QEMU Developers
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] should we try to stop using variable length arrays?
2019-02-07 19:30 [Qemu-devel] should we try to stop using variable length arrays? Peter Maydell
2019-02-07 19:39 ` Eric Blake
@ 2019-02-07 20:28 ` Eric Blake
2019-02-07 21:56 ` Philippe Mathieu-Daudé
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Eric Blake @ 2019-02-07 20:28 UTC (permalink / raw)
To: Peter Maydell, QEMU Developers
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] should we try to stop using variable length arrays?
2019-02-07 19:30 [Qemu-devel] should we try to stop using variable length arrays? Peter Maydell
2019-02-07 19:39 ` Eric Blake
2019-02-07 20:28 ` Eric Blake
@ 2019-02-07 21:56 ` Philippe Mathieu-Daudé
2019-02-11 3:26 ` Stefan Hajnoczi
2019-02-11 10:58 ` Daniel P. Berrangé
4 siblings, 0 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-02-07 21:56 UTC (permalink / raw)
To: Peter Maydell, QEMU Developers
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] should we try to stop using variable length arrays?
2019-02-07 19:30 [Qemu-devel] should we try to stop using variable length arrays? Peter Maydell
` (2 preceding siblings ...)
2019-02-07 21:56 ` Philippe Mathieu-Daudé
@ 2019-02-11 3:26 ` Stefan Hajnoczi
2019-02-11 10:58 ` Daniel P. Berrangé
4 siblings, 0 replies; 6+ messages in thread
From: Stefan Hajnoczi @ 2019-02-11 3:26 UTC (permalink / raw)
To: Peter Maydell; +Cc: QEMU Developers
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] should we try to stop using variable length arrays?
2019-02-07 19:30 [Qemu-devel] should we try to stop using variable length arrays? Peter Maydell
` (3 preceding siblings ...)
2019-02-11 3:26 ` Stefan Hajnoczi
@ 2019-02-11 10:58 ` Daniel P. Berrangé
4 siblings, 0 replies; 6+ messages in thread
From: Daniel P. Berrangé @ 2019-02-11 10:58 UTC (permalink / raw)
To: Peter Maydell; +Cc: QEMU Developers
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 :|
^ permalink raw reply [flat|nested] 6+ messages in thread