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

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

[Michael Bommarito]

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).

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;
+	}
+
 	smp_store_release(&vi->data_avail, len);
 	complete(&vi->have_data);
 }
-- 
2.53.0

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

[Michael S. Tsirkin]

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;
> +	}
> +
>  	smp_store_release(&vi->data_avail, len);
>  	complete(&vi->have_data);
>  }
> -- 
> 2.53.0

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@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;
> > +     }
> > +
> >       smp_store_release(&vi->data_avail, len);
> >       complete(&vi->have_data);
> >  }
> > --
> > 2.53.0
>

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

[Michael S. Tsirkin]

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
> >

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

[Michael Bommarito]

On Fri, Apr 17, 2026 at 8:31 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> Actionable meaning what?

Well, between the BLAKE2 pass and the fact that 99% of guests already
shouldn't trust what's above, I agree that actionable doesn't mean
much to most people, not even for breaking KASLR.

But after doing some research, I realized that SEV-SNP/TDX guests that
expect lockdown=confidentiality might actually expect otherwise under
that security model.  Still not a lot to work with, but more than just
correctness in those cases, and those might be the environments that
care the most.

> 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.

Happy to send a v2 with those changes but I can only test on a 1-2 TDX
variants at home and don't have access to an EPYC bare metal box, so
not very confident about your buggy device point

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

[Michael S. Tsirkin]

On Fri, Apr 17, 2026 at 08:47:09PM -0400, Michael Bommarito wrote:
> On Fri, Apr 17, 2026 at 8:31 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > Actionable meaning what?
> 
> Well, between the BLAKE2 pass and the fact that 99% of guests already
> shouldn't trust what's above, I agree that actionable doesn't mean
> much to most people, not even for breaking KASLR.
> 
> But after doing some research, I realized that SEV-SNP/TDX guests that
> expect lockdown=confidentiality might actually expect otherwise under
> that security model.  Still not a lot to work with, but more than just
> correctness in those cases, and those might be the environments that
> care the most.

Sorry this went over my head. We are talking about a device where guest
trusts host to feed it randomness, enabling it is already a questionable
enterprise for SEV-SNP/TDX. So what does it matter whether guest gets by
data from host directly or by tricking it into feeding its own data to
it?  It's all supposed to be securely mixed with the cpu rng, right?

I am not arguing we should not fix it, I am trying to figure out
the actual security impact.


> > 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.
> 
> Happy to send a v2 with those changes but I can only test on a 1-2 TDX
> variants at home and don't have access to an EPYC bare metal box, so
> not very confident about your buggy device point


I am not sure why this matters.

-- 
MST

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

[Michael Bommarito]

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@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)
-- 
2.53.0

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

[Michael S. Tsirkin]

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@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

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

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

[Michael S. Tsirkin]

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
> >

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

[Michael Bommarito]

On Sat, Apr 18, 2026 at 1:39 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> 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.

I also don't feel confident about how much the differences matter.
For background, I think I lifted the pattern from similar issues in
kvm and io_uring.  Your point about request_entropy is right either
way.

Maybe we'll see if anyone else weighs in over the next few days and if
not, I'll go with your shorter fix for v3.