[PATCH] hw/block/dataplane/virtio-block: Avoid dynamic stack allocation

[PATCH] hw/block/dataplane/virtio-block: Avoid dynamic stack allocation

[Peter Maydell]

Instead of using a variable length array in notify_guest_bh(), always
use a fixed sized bitmap (this will be 128 bytes).  This means we
need to avoid assuming that bitmap and the s->batch_notify_vqs bitmap
are the same size; the neatest way to do this is to switch to using
bitmap.h APIs to declare, copy and clear, because then we can specify
the length in bits, exactly as we did when creating
s->batch_notify_vqs with bitmap_new().

The codebase has very few VLAs, and if we can get rid of them all 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).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
In discussion on Philippe's attempt at getting rid of this VLA:
https://patchew.org/QEMU/20210505211047.1496765-1-philmd@redhat.com/20210505211047.1496765-7-philmd@redhat.com/
Stefan suggested getting rid of the local bitmap array entirely.
But I don't know this code at all and have no idea of the
implications (presumably there is a reason we have the local
array rather than iterating directly on batch_notify_vqs),
so I have opted for the more minimal change.

Usual disclaimer: tested only with "make check" and
"make check-avocado".
---
 hw/block/dataplane/virtio-blk.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index da36fcfd0b5..f31ec79d0b2 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -59,11 +59,16 @@ static void notify_guest_bh(void *opaque)
 {
     VirtIOBlockDataPlane *s = opaque;
     unsigned nvqs = s->conf->num_queues;
-    unsigned long bitmap[BITS_TO_LONGS(nvqs)];
+    DECLARE_BITMAP(bitmap, VIRTIO_QUEUE_MAX);
     unsigned j;
 
-    memcpy(bitmap, s->batch_notify_vqs, sizeof(bitmap));
-    memset(s->batch_notify_vqs, 0, sizeof(bitmap));
+    /*
+     * Note that our local 'bitmap' is declared at a fixed
+     * worst case size, but s->batch_notify_vqs has only
+     * nvqs bits in it.
+     */
+    bitmap_copy(bitmap, s->batch_notify_vqs, nvqs);
+    bitmap_zero(s->batch_notify_vqs, nvqs);
 
     for (j = 0; j < nvqs; j += BITS_PER_LONG) {
         unsigned long bits = bitmap[j / BITS_PER_LONG];
-- 
2.34.1

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

[Stefan Hajnoczi]

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

On Thu, Aug 24, 2023 at 05:57:40PM +0100, Peter Maydell wrote:
> Instead of using a variable length array in notify_guest_bh(), always
> use a fixed sized bitmap (this will be 128 bytes).  This means we
> need to avoid assuming that bitmap and the s->batch_notify_vqs bitmap
> are the same size; the neatest way to do this is to switch to using
> bitmap.h APIs to declare, copy and clear, because then we can specify
> the length in bits, exactly as we did when creating
> s->batch_notify_vqs with bitmap_new().
> 
> The codebase has very few VLAs, and if we can get rid of them all 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).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> In discussion on Philippe's attempt at getting rid of this VLA:
> https://patchew.org/QEMU/20210505211047.1496765-1-philmd@redhat.com/20210505211047.1496765-7-philmd@redhat.com/
> Stefan suggested getting rid of the local bitmap array entirely.
> But I don't know this code at all and have no idea of the
> implications (presumably there is a reason we have the local
> array rather than iterating directly on batch_notify_vqs),
> so I have opted for the more minimal change.
> 
> Usual disclaimer: tested only with "make check" and
> "make check-avocado".
> ---
>  hw/block/dataplane/virtio-blk.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)

Hi Peter,
I recently sent a patch series that removes notify_guest_bh() completely:
https://lore.kernel.org/qemu-devel/20230817155847.3605115-5-stefanha@redhat.com/

If it's urgent we can merge your patch immediately, though I hope my
series will be merged soon anyway:
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index da36fcfd0b5..f31ec79d0b2 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -59,11 +59,16 @@ static void notify_guest_bh(void *opaque)
>  {
>      VirtIOBlockDataPlane *s = opaque;
>      unsigned nvqs = s->conf->num_queues;
> -    unsigned long bitmap[BITS_TO_LONGS(nvqs)];
> +    DECLARE_BITMAP(bitmap, VIRTIO_QUEUE_MAX);
>      unsigned j;
>  
> -    memcpy(bitmap, s->batch_notify_vqs, sizeof(bitmap));
> -    memset(s->batch_notify_vqs, 0, sizeof(bitmap));
> +    /*
> +     * Note that our local 'bitmap' is declared at a fixed
> +     * worst case size, but s->batch_notify_vqs has only
> +     * nvqs bits in it.
> +     */
> +    bitmap_copy(bitmap, s->batch_notify_vqs, nvqs);
> +    bitmap_zero(s->batch_notify_vqs, nvqs);
>  
>      for (j = 0; j < nvqs; j += BITS_PER_LONG) {
>          unsigned long bits = bitmap[j / BITS_PER_LONG];
> -- 
> 2.34.1
> 

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

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

[Peter Maydell]

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

On Thu, 24 Aug 2023 at 18:15, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Thu, Aug 24, 2023 at 05:57:40PM +0100, Peter Maydell wrote:
> > Instead of using a variable length array in notify_guest_bh(), always
> > use a fixed sized bitmap (this will be 128 bytes).  This means we
> > need to avoid assuming that bitmap and the s->batch_notify_vqs bitmap
> > are the same size; the neatest way to do this is to switch to using
> > bitmap.h APIs to declare, copy and clear, because then we can specify
> > the length in bits, exactly as we did when creating
> > s->batch_notify_vqs with bitmap_new().
> >
> > The codebase has very few VLAs, and if we can get rid of them all 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).
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> > In discussion on Philippe's attempt at getting rid of this VLA:
> >
https://patchew.org/QEMU/20210505211047.1496765-1-philmd@redhat.com/20210505211047.1496765-7-philmd@redhat.com/
> > Stefan suggested getting rid of the local bitmap array entirely.
> > But I don't know this code at all and have no idea of the
> > implications (presumably there is a reason we have the local
> > array rather than iterating directly on batch_notify_vqs),
> > so I have opted for the more minimal change.
> >
> > Usual disclaimer: tested only with "make check" and
> > "make check-avocado".
> > ---
> >  hw/block/dataplane/virtio-blk.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
>
> Hi Peter,
> I recently sent a patch series that removes notify_guest_bh() completely:
>
https://lore.kernel.org/qemu-devel/20230817155847.3605115-5-stefanha@redhat.com/
>
> If it's urgent we can merge your patch immediately, though I hope my
> series will be merged soon anyway:
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Great. There's no hurry here, so your change looks like the
best approach.

-- PMM

[-- Attachment #2: Type: text/html, Size: 3230 bytes --]