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é <phi...@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