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

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

On Fri, Apr 17, 2026 at 08:18:06PM -0400, Michael Bommarito wrote:
> "Actionable" is probably the better word there, sorry.

Actionable meaning what?

>  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@redhat.com> 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@vger.kernel.org
> > > Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
> > > 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;
> > > +     }


Maybe clamp at sizeof(vi->data) then? 0 might break buggy devices that
were working earlier.  
Or just clamp where it's used, for clarity.
And maybe we need the array_index dance, given
you are worried about malicious.


> > > +
> > >       smp_store_release(&vi->data_avail, len);
> > >       complete(&vi->have_data);
> > >  }
> > > --
> > > 2.53.0
> >