[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 06/23] hw/block/dataplane/virtio-blk: Avoid dynamic stack all
From: |
Stefan Hajnoczi |
Subject: |
Re: [PATCH 06/23] hw/block/dataplane/virtio-blk: Avoid dynamic stack allocation |
Date: |
Thu, 6 May 2021 15:47:19 +0100 |
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
signature.asc
Description: PGP signature
- Re: [PATCH 02/23] chardev/baum: Replace magic values by X_MAX / Y_MAX definitions, (continued)
[PATCH 07/23] hw/block/nvme: Use definition to avoid dynamic stack allocation, Philippe Mathieu-Daudé, 2021/05/05
- Re: [PATCH 07/23] hw/block/nvme: Use definition to avoid dynamic stack allocation, Keith Busch, 2021/05/05
- Re: [PATCH 07/23] hw/block/nvme: Use definition to avoid dynamic stack allocation, Philippe Mathieu-Daudé, 2021/05/05
- Re: [PATCH 07/23] hw/block/nvme: Use definition to avoid dynamic stack allocation, Eric Blake, 2021/05/05
- Re: [PATCH 07/23] hw/block/nvme: Use definition to avoid dynamic stack allocation, Warner Losh, 2021/05/05
- Re: [PATCH 07/23] hw/block/nvme: Use definition to avoid dynamic stack allocation, Keith Busch, 2021/05/05
- Re: [PATCH 07/23] hw/block/nvme: Use definition to avoid dynamic stack allocation, Philippe Mathieu-Daudé, 2021/05/06
- Re: [PATCH 07/23] hw/block/nvme: Use definition to avoid dynamic stack allocation, Richard Henderson, 2021/05/07