Re: [PATCH] hwrng: virtio: reject invalid used.len from the device

[Michael Bommarito]

"Actionable" is probably the better word there, sorry.  If it were
otherwise, I wouldn't have filed publicly

If you end up ACKing the correctness change, I can send v2 with better log

Thanks,
Michael Bommarito

On Fri, Apr 17, 2026 at 8:13 PM Michael S. Tsirkin <mst@xxxxxxxxxx> wrote:
>
> On Fri, Apr 17, 2026 at 08:00:20PM -0400, Michael Bommarito wrote:
> > random_recv_done() stored the device-reported used.len directly into
> > vi->data_avail without validating it against the posted buffer size
> > sizeof(vi->data) (SMP_CACHE_BYTES bytes, typically 32 or 64).  A
> > malicious or buggy virtio-rng backend could set used.len beyond
> > vi->data so that the subsequent copy_data() in virtio_read() issues
> > memcpy() from vi->data + vi->data_idx past the end of the inline
> > array, reading adjacent kmalloc-1k slab bytes into the hwrng core's
> > buffer and from there into /dev/hwrng consumers and the kernel
> > entropy pool.
> >
> > Exploitable most clearly in threat models that do not trust the
> > hypervisor (confidential-compute guests on SEV-SNP or TDX; vhost-user
> > split backends).
>
> Exploitable? I don't get it. How is reading this data into hwrng
> a problem?
>
>
> > 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)
> >
> > hwrng_fillfn is a kernel thread that runs as soon as the device is
> > probed; no guest userspace interaction is needed.
> >
> > 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 added len-vs-sizeof(vi->data) clamp in place the same
> > harness boots cleanly: the driver logs "bogus used.len" once and
> > subsequent reads wait for a honest response.
> >
> > Fixes: f7f510ec1957 ("virtio: An entropy device, as suggested by hpa.")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Michael Bommarito <michael.bommarito@xxxxxxxxx>
> > Assisted-by: Claude:claude-opus-4-7
> > ---
> >  drivers/char/hw_random/virtio-rng.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
> > index 0ce02d7e5048..6cff480787ca 100644
> > --- a/drivers/char/hw_random/virtio-rng.c
> > +++ b/drivers/char/hw_random/virtio-rng.c
> > @@ -47,6 +47,18 @@ static void random_recv_done(struct virtqueue *vq)
> >       if (!virtqueue_get_buf(vi->vq, &len))
> >               return;
> >
> > +     /*
> > +      * The device sets used.len; a malicious or buggy backend can
> > +      * report more bytes than we posted.  Clamp before it reaches
> > +      * copy_data() which indexes vi->data[].
> > +      */
> > +     if (len > sizeof(vi->data)) {
> > +             dev_err(&vq->vdev->dev,
> > +                     "bogus used.len %u > buffer size %zu\n",
> > +                     len, sizeof(vi->data));
> > +             len = 0;
> > +     }
> > +
> >       smp_store_release(&vi->data_avail, len);
> >       complete(&vi->have_data);
> >  }
> > --
> > 2.53.0
>