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

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- 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 --]