Re: [PATCH v2] hwrng: virtio: clamp device-reported used.len at copy_data()

[Michael Bommarito]

I think the difference comes back to how much you care about the
threat model and something like Spectre on the memcpy later in
copy_data.  The more verbose patch would keep the barrier at the cost
of the code complexity and a few extra cycles, but then we're back to
same tradeoffs that have haunted just about everyone.

Will obviously defer to you on which path is really preferred, so let
me know if you want v3 with the simple nospec clamp.

Thanks,
Michael Bommarito

On Sat, Apr 18, 2026 at 1:18 PM Michael S. Tsirkin <mst@xxxxxxxxxx> wrote:
>
> On Sat, Apr 18, 2026 at 11:06:13AM -0400, Michael Bommarito wrote:
> > random_recv_done() stores the device-reported used.len directly into
> > vi->data_avail.  copy_data() then indexes vi->data[] using
> > vi->data_idx (advanced by previous copy_data() calls) and issues a
> > memcpy() without re-validating either value against the posted
> > buffer size sizeof(vi->data) (SMP_CACHE_BYTES bytes, typically 32
> > or 64).
> >
> > A malicious or buggy virtio-rng backend can set used.len beyond
> > sizeof(vi->data), steering the memcpy() past the end of the inline
> > array into adjacent kmalloc-1k slab bytes.  hwrng_fillfn() mixes
> > those bytes into the guest RNG, and guest root can also observe
> > them directly via /dev/hwrng.
> >
> > Concrete impact is inside the guest:
> >
> >  - Memory-safety / hardening: any virtio-rng backend that
> >    over-reports used.len causes the driver to read past vi->data
> >    into unrelated slab contents.  hwrng_fillfn() is a kernel thread
> >    that runs as soon as the device is probed; no guest userspace
> >    interaction is required to first-trigger the OOB.
> >
> >  - Cross-boundary leak (confidential-compute threat model): a
> >    malicious hypervisor cooperating with a malicious or compromised
> >    guest root userspace can use /dev/hwrng as a leak channel for
> >    guest-kernel heap data.  The host sets a large used.len, guest
> >    root reads /dev/hwrng, and the returned bytes contain guest
> >    kernel slab contents that were adjacent to vi->data.  In
> >    practice, confidential-compute guests (SEV-SNP, TDX) usually
> >    disable virtio-rng entirely, so this path is narrow, but the
> >    fix is still worth carrying because the underlying
> >    memory-safety bug contaminates the guest RNG on any host.
> >
> > KASAN confirms the OOB on a guest booted under a QEMU 9.0 whose
> > virtio-rng backend has been patched to report used.len = 0x10000:
> >
> >   BUG: KASAN: slab-out-of-bounds in virtio_read+0x394/0x5d0
> >   Read of size 64 at addr ffff8880089c2220 by task hwrng/52
> >   Call Trace:
> >    __asan_memcpy
> >    virtio_read+0x394/0x5d0
> >    hwrng_fillfn+0xb2/0x470
> >    kthread
> >   Allocated by task 1:
> >    probe_common+0xa5/0x660
> >    virtio_dev_probe+0x549/0xbc0
> >   The buggy address belongs to the object at ffff8880089c2000
> >    which belongs to the cache kmalloc-1k of size 1024
> >   The buggy address is located 0 bytes to the right of
> >    allocated 544-byte region [ffff8880089c2000, ffff8880089c2220)
> >
> > Same class of bug as commit c04db81cd028 ("net/9p: Fix buffer
> > overflow in USB transport layer"), which hardened
> > usb9pfs_rx_complete() against unchecked device-reported length in
> > the USB 9p transport.
> >
> > With the clamp at point of use and array_index_nospec() in place,
> > the same harness boots cleanly: copy_data() returns zero for the
> > bogus report, the device-supplied bytes after data_idx are
> > discarded, and the driver issues a fresh request.
> >
> > Changes in v2 (per Michael S. Tsirkin review):
> > - move the bound check from random_recv_done() into copy_data(),
> >   so the clamp sits immediately next to the memcpy it protects
> > - clamp to sizeof(vi->data) rather than substituting len = 0, so a
> >   previously-working but buggy device that occasionally over-reports
> >   used.len does not start returning zero-length reads
> > - add array_index_nospec() on vi->data_idx to defeat a speculative
> >   out-of-bounds read given the malicious-backend threat model
> > - expand the commit message to describe the /dev/hwrng observation
> >   path and the hypervisor + guest-root cooperation scenario
> >
> > Fixes: f7f510ec1957 ("virtio: An entropy device, as suggested by hpa.")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Suggested-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
> > Signed-off-by: Michael Bommarito <michael.bommarito@xxxxxxxxx>
> > Assisted-by: Claude:claude-opus-4-7
> > ---
> >  drivers/char/hw_random/virtio-rng.c | 23 +++++++++++++++++++++--
> >  1 file changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
> > index 0ce02d7e5048..5e83ffa105e4 100644
> > --- a/drivers/char/hw_random/virtio-rng.c
> > +++ b/drivers/char/hw_random/virtio-rng.c
> > @@ -7,6 +7,7 @@
> >  #include <asm/barrier.h>
> >  #include <linux/err.h>
> >  #include <linux/hw_random.h>
> > +#include <linux/nospec.h>
> >  #include <linux/scatterlist.h>
> >  #include <linux/spinlock.h>
> >  #include <linux/virtio.h>
> > @@ -69,8 +70,26 @@ static void request_entropy(struct virtrng_info *vi)
> >  static unsigned int copy_data(struct virtrng_info *vi, void *buf,
> >                             unsigned int size)
> >  {
> > -     size = min_t(unsigned int, size, vi->data_avail);
> > -     memcpy(buf, vi->data + vi->data_idx, size);
> > +     unsigned int idx, avail;
> > +
> > +     /*
> > +      * vi->data_avail was set from the device-reported used.len and
> > +      * vi->data_idx was advanced by previous copy_data() calls.  A
> > +      * malicious or buggy virtio-rng backend can drive either past
> > +      * sizeof(vi->data).  Clamp at point of use and harden the index
> > +      * with array_index_nospec() so the memcpy() below cannot be
> > +      * steered into adjacent slab memory, including under
> > +      * speculation.
> > +      */
> > +     avail = min_t(unsigned int, vi->data_avail, sizeof(vi->data));
> > +     if (vi->data_idx >= avail) {
> > +             vi->data_avail = 0;
> > +             request_entropy(vi);
> > +             return 0;
> > +     }
> > +     size = min_t(unsigned int, size, avail - vi->data_idx);
> > +     idx = array_index_nospec(vi->data_idx, sizeof(vi->data));
> > +     memcpy(buf, vi->data + idx, size);
> >       vi->data_idx += size;
> >       vi->data_avail -= size;
> >       if (vi->data_avail == 0)
> > --
>
>
> This came out quite complex.
> Tell me, will the following do the trick?
>
>
> diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
> index 0ce02d7e5048..e887a68cc151 100644
> --- a/drivers/char/hw_random/virtio-rng.c
> +++ b/drivers/char/hw_random/virtio-rng.c
> @@ -47,6 +47,8 @@ static void random_recv_done(struct virtqueue *vq)
>         if (!virtqueue_get_buf(vi->vq, &len))
>                 return;
>
> +       len = array_index_nospec(len, sizeof(vi->data));
> +
>         smp_store_release(&vi->data_avail, len);
>         complete(&vi->have_data);
>  }
>
>
>
> > 2.53.0
>