| Message ID | 20210505211047.1496765-7-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | misc: Remove variable-length arrays on the stack | expand |
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? This is a performance-critical code path and BITS_TO_LONGS(nvqs) is small. Stefan
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. > 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? > > Stefan >
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
On 5/6/21 4:47 PM, Stefan Hajnoczi wrote: > 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. Fair enough. >>> 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[]. Doh I missed that :/ > Can we eliminate bitmap[] entirely by using bitops.h APIs on > s->batch_notify_vqs instead? You meant the bitmap.h API I assume, OK, better idea! Thanks, Phil.
On Thu, May 06, 2021 at 05:19:31PM +0200, Philippe Mathieu-Daudé wrote: > > Can we eliminate bitmap[] entirely by using bitops.h APIs on > > s->batch_notify_vqs instead? > > You meant the bitmap.h API I assume, OK, better idea! I did mean include/qemu/bitops.h. Not sure I see a suitable bitmap.h API but in any case bitmap.h includes bitops.h :-). Stefan
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c index e9050c8987e..53f5e4d8aa6 100644 --- a/hw/block/dataplane/virtio-blk.c +++ b/hw/block/dataplane/virtio-blk.c @@ -60,11 +60,12 @@ static void notify_guest_bh(void *opaque) { VirtIOBlockDataPlane *s = opaque; unsigned nvqs = s->conf->num_queues; - unsigned long bitmap[BITS_TO_LONGS(nvqs)]; + long bitmap_nr = BITS_TO_LONGS(nvqs); + g_autofree unsigned long *bitmap = g_new(unsigned long, bitmap_nr); unsigned j; - memcpy(bitmap, s->batch_notify_vqs, sizeof(bitmap)); - memset(s->batch_notify_vqs, 0, sizeof(bitmap)); + memcpy(bitmap, s->batch_notify_vqs, bitmap_nr * sizeof(*bitmap)); + memset(s->batch_notify_vqs, 0, bitmap_nr * sizeof(*bitmap)); for (j = 0; j < nvqs; j += BITS_PER_LONG) { unsigned long bits = bitmap[j / BITS_PER_LONG];
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(-)