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

["Michael S. Tsirkin" <mst@redhat.com>]

On Sat, Apr 18, 2026 at 01:25:35PM -0400, Michael Bommarito wrote:
> 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. 

Maybe we do I'm just not sure I understand how do
all these checks help, and for what threat.
It could be just me being dense.

The commit log merely describes use.len being OOB
and also mentions data_idx.
Requests are always for sizeof(vi->data)
and they reset data_idx to 0:

static void request_entropy(struct virtrng_info *vi)
{
        struct scatterlist sg;

        reinit_completion(&vi->have_data);
        vi->data_idx = 0;

        sg_init_one(&sg, vi->data, sizeof(vi->data));

        /* There should always be room for one buffer. */
        virtqueue_add_inbuf(vi->vq, &sg, 1, vi->data, GFP_KERNEL);

        virtqueue_kick(vi->vq);
}


so to me, it looks like
clamping that at sizeof(vi->data) addresses that.


is there another threat you are worried about then?




> 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@redhat.com> 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.


there should be --- here, btw.

> > > 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@vger.kernel.org
> > > Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> > > Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
> > > 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
> >