Re: [PATCH 06/23] hw/block/dataplane/virtio-blk: Avoid dynamic stack allocation

From: Stefan Hajnoczi <stefanha@redhat.com>
To: "Philippe Mathieu-Daudé" <philmd@redhat.com>
Cc: "Kevin Wolf" <kwolf@redhat.com>,
	"Daniel P. Berrangé" <berrange@redhat.com>,
	qemu-block@nongnu.org,
	"Richard Henderson" <richard.henderson@linaro.org>,
	qemu-devel@nongnu.org, "Max Reitz" <mreitz@redhat.com>,
	qemu-ppc@nongnu.org, "Gerd Hoffmann" <kraxel@redhat.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Marc-André Lureau" <marcandre.lureau@redhat.com>
Subject: Re: [PATCH 06/23] hw/block/dataplane/virtio-blk: Avoid dynamic stack allocation
Date: Thu, 6 May 2021 15:47:19 +0100	[thread overview]
Message-ID: <YJQBd+lnvQnbK0XH@stefanha-x1.localdomain> (raw)
In-Reply-To: <124ddeb2-ca4a-c551-19ad-d1125451226f@redhat.com>

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

On Thu, May 06, 2021 at 11:01:47AM +0200, Philippe Mathieu-Daudé wrote:
> On 5/6/21 10:53 AM, Stefan Hajnoczi wrote:
> > On Wed, May 05, 2021 at 11:10:30PM +0200, Philippe Mathieu-Daudé wrote:
> >> Use autofree heap allocation instead of variable-length
> >> array on the stack.
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> ---
> >>  hw/block/dataplane/virtio-blk.c | 7 ++++---
> >>  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > Why?
> 
> The motivation behind removing all variable-length allocations
> (and adding CPPFLAG+=-Wvla at the end) is to avoid security
> vulnerabilities such CVE-2021-3527.

I see. Please mention it in the commit description. There could be other
reasons for this change, like minimizing stack usage, so I wasn't sure
why.

> > This is a performance-critical code path and BITS_TO_LONGS(nvqs) is
> > small.
> 
> OK, having looked better at nvqs, I suppose this is preferred:
> 
> -- >8 --
> @@ -60,7 +60,7 @@ static void notify_guest_bh(void *opaque)
>  {
>      VirtIOBlockDataPlane *s = opaque;
>      unsigned nvqs = s->conf->num_queues;
> -    unsigned long bitmap[BITS_TO_LONGS(nvqs)];
> +    unsigned long bitmap[BITS_TO_LONGS(VIRTIO_QUEUE_MAX)];
>      unsigned j;
> 
>      memcpy(bitmap, s->batch_notify_vqs, sizeof(bitmap));
> ---
> 
> Would that work for you?

It's a little risky since s->batch_notify_vqs does not have
sizeof(bitmap). That makes uninitialized data and buffer overflows more
likely. Your example has the bug:

  memcpy(bitmap, s->batch_notify_vqs, sizeof(bitmap));
                                      ^^^^^^^^^^^^^^
  Accesses beyond the end of s->batch_notify_vqs[].

Can we eliminate bitmap[] entirely by using bitops.h APIs on
s->batch_notify_vqs instead?

Stefan

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