| Message ID | 20230824165740.2653919-1-peter.maydell@linaro.org |
|---|---|
| State | Rejected |
| Headers | show |
| Series | hw/block/dataplane/virtio-block: Avoid dynamic stack allocation | expand |
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 >
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
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];
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(-)