On Tue, Jun 10, 2025 at 12:41:00PM -0400, Michael S. Tsirkin wrote:
> On Wed, Jun 04, 2025 at 03:18:43PM -0400, Stefan Hajnoczi wrote:
> > Since commit 7ff9ff039380 ("meson: mitigate against use of uninitialize
> > stack for exploits") the -ftrivial-auto-var-init=zero compiler option is
> > used to zero local variables. While this reduces security risks
> > associated with uninitialized stack data, it introduced a measurable
> > bottleneck in the virtqueue_split_pop() and virtqueue_packed_pop()
> > functions.
> >
> > These virtqueue functions are in the hot path. They are called for each
> > element (request) that is popped from a VIRTIO device's virtqueue. Using
> > __attribute__((uninitialized)) on large stack variables in these
> > functions improves fio randread bs=4k iodepth=64 performance from 304k
> > to 332k IOPS (+9%).
>
> I ask however whether it is always not worth it for all users.
> It does reduce chances of leaking stack info, does it not?
>
> Maybe we can start with a tri-state Kconfig knob to select between
> performance/balanced/paranoid for this and similar variables?
I'd prefer this not to be configurable. With a fixed setup, when we
analyse reported bugs, we can make a clear determination of whether
it is actually an expoitable security bug or not, without having to
concern ourselves with build time settings. Essentially the
-ftrivial-auto-var-init=zero turned almost all 'uninitialized
stack variable' bugs into plain bugs, or even non-bugs in most
cases, instead of likely security issues. The QEMU_UNINITIALIZED
annotations are targetted fixed to avoid the worst case perf
hits, without compromising our security posture.
>
>
> > This issue was found using perf-top(1). virtqueue_split_pop() was one of
> > the top CPU consumers and the "annotate" feature showed that the memory
> > zeroing instructions at the beginning of the functions were hot.
> >
> > Fixes: 7ff9ff039380 ("meson: mitigate against use of uninitialize stack for
> > exploits")
> > Cc: Daniel P. Berrangé <berra...@redhat.com>
> > Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com>
> > ---
> > include/qemu/compiler.h | 12 ++++++++++++
> > hw/virtio/virtio.c | 8 ++++----
> > 2 files changed, 16 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
> > index 496dac5ac1..fabd540b02 100644
> > --- a/include/qemu/compiler.h
> > +++ b/include/qemu/compiler.h
> > @@ -207,6 +207,18 @@
> > # define QEMU_USED
> > #endif
> >
> > +/*
> > + * Disable -ftrivial-auto-var-init on a local variable. Use this in rare
> > cases
> > + * when the compiler zeroes a large on-stack variable and this causes a
> > + * performance bottleneck. Only use it when performance data indicates
> > this is
> > + * necessary since security risks increase with uninitialized stack
> > variables.
> > + */
> > +#if __has_attribute(uninitialized)
> > +# define QEMU_UNINITIALIZED __attribute__((uninitialized))
> > +#else
> > +# define QEMU_UNINITIALIZED
> > +#endif
> > +
> > /*
> > * http://clang.llvm.org/docs/ThreadSafetyAnalysis.html
> > *
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 5534251e01..82a285a31d 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -1689,8 +1689,8 @@ static void *virtqueue_split_pop(VirtQueue *vq,
> > size_t sz)
> > VirtIODevice *vdev = vq->vdev;
> > VirtQueueElement *elem = NULL;
> > unsigned out_num, in_num, elem_entries;
> > - hwaddr addr[VIRTQUEUE_MAX_SIZE];
> > - struct iovec iov[VIRTQUEUE_MAX_SIZE];
> > + hwaddr QEMU_UNINITIALIZED addr[VIRTQUEUE_MAX_SIZE];
> > + struct iovec QEMU_UNINITIALIZED iov[VIRTQUEUE_MAX_SIZE];
> > VRingDesc desc;
> > int rc;
> >
> > @@ -1836,8 +1836,8 @@ static void *virtqueue_packed_pop(VirtQueue *vq,
> > size_t sz)
> > VirtIODevice *vdev = vq->vdev;
> > VirtQueueElement *elem = NULL;
> > unsigned out_num, in_num, elem_entries;
> > - hwaddr addr[VIRTQUEUE_MAX_SIZE];
> > - struct iovec iov[VIRTQUEUE_MAX_SIZE];
> > + hwaddr QEMU_UNINITIALIZED addr[VIRTQUEUE_MAX_SIZE];
> > + struct iovec QEMU_UNINITIALIZED iov[VIRTQUEUE_MAX_SIZE];
> > VRingPackedDesc desc;
> > uint16_t id;
> > int rc;
> > --
> > 2.49.0
>
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
