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

[PATCH v3] 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 7.1-rc4 guest 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 ffff88800ae0ba20 by task hwrng/52
  Call Trace:
   __asan_memcpy+0x23/0x60
   virtio_read+0x394/0x5d0
   hwrng_fillfn+0xb2/0x470
   kthread+0x2cc/0x3a0
  Allocated by task 1:
   probe_common+0xa5/0x660
   virtio_dev_probe+0x549/0xbc0
  The buggy address belongs to the object at ffff88800ae0b800
   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 [ffff88800ae0b800, ffff88800ae0ba20)

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.

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-8
---
Changes in v3:
- No functional change from v2.  Reposting the v2 clamp after the v2
  thread went quiet on linux-crypto.  Michael S. Tsirkin reconfirmed
  off-list that clamping the device-reported used.len at
  sizeof(vi->data) addresses his earlier concern, so this resends that
  fix unchanged.
- Rebased onto v7.1-rc4.  copy_data() is unchanged since 2023, so the
  clamp applies as-is, and the KASAN reproduction above was re-run on
  v7.1-rc4 (stock splats, patched boots clean).

Changes in v2 (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 with the /dev/hwrng observation path and
  the hypervisor plus guest-root cooperation scenario.

v1: https://lore.kernel.org/all/20260418000020.1847122-1-michael.bommarito@gmail.com/
v2: https://lore.kernel.org/all/20260418150613.3522589-1-michael.bommarito@gmail.com/

 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 0ce02d7e5048e..5e83ffa105e41 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)

base-commit: a1f173eb51db0dc78536334729ef832c62d6c65a
-- 
2.53.0

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

[Herbert Xu]

On Sun, May 31, 2026 at 10:22:51AM -0400, Michael Bommarito wrote:
>
> +	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);

I don't see how nospec can help here.  Please enlighten me.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Michael S. Tsirkin]

On Thu, Jun 11, 2026 at 12:43:09PM +0800, Herbert Xu wrote:
> On Sun, May 31, 2026 at 10:22:51AM -0400, Michael Bommarito wrote:
> >
> > +	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);
> 
> I don't see how nospec can help here.  Please enlighten me.


All the "malicious device" things are confusing. Spectre things -
doubly so.

So if an access is speculated then CPU might speculate feeding a kernel
secret into RNG. And then the speculated RNG value maybe can be also
speculatively be used by some kernel code as an index
to trigger a cache access, finally leaking the secret?

Maybe?




> Thanks,
> -- 
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Herbert Xu]

On Thu, Jun 11, 2026 at 03:30:14AM -0400, Michael S. Tsirkin wrote:
> On Thu, Jun 11, 2026 at 12:43:09PM +0800, Herbert Xu wrote:
> > On Sun, May 31, 2026 at 10:22:51AM -0400, Michael Bommarito wrote:
> > >
> > > +	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);
> 
> All the "malicious device" things are confusing. Spectre things -
> doubly so.
> 
> So if an access is speculated then CPU might speculate feeding a kernel
> secret into RNG. And then the speculated RNG value maybe can be also
> speculatively be used by some kernel code as an index
> to trigger a cache access, finally leaking the secret?
> 
> Maybe?

The way Spectre works is if you have an actual instruction using
idx directly.  I don't see how that translates to memcpy.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Michael S. Tsirkin]

On Thu, Jun 11, 2026 at 03:46:58PM +0800, Herbert Xu wrote:
> On Thu, Jun 11, 2026 at 03:30:14AM -0400, Michael S. Tsirkin wrote:
> > On Thu, Jun 11, 2026 at 12:43:09PM +0800, Herbert Xu wrote:
> > > On Sun, May 31, 2026 at 10:22:51AM -0400, Michael Bommarito wrote:
> > > >
> > > > +	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);
> > 
> > All the "malicious device" things are confusing. Spectre things -
> > doubly so.
> > 
> > So if an access is speculated then CPU might speculate feeding a kernel
> > secret into RNG. And then the speculated RNG value maybe can be also
> > speculatively be used by some kernel code as an index
> > to trigger a cache access, finally leaking the secret?
> > 
> > Maybe?
> 
> The way Spectre works is if you have an actual instruction using
> idx directly.  I don't see how that translates to memcpy.

I am not sure it has to be direct:

if (malicious_idx > SIZE)
	return;
src += malicious_idx;
memcpy(&value, src, ...)
....
hash = complex_hash_of(value)
....
return p[hash * 512];

is IIUC still a valid spectre v1 gadget leaking a value beyong SIZE, or
did I miss something?


And rng is a kind of a complex hash, but I also think in that "...."
in the kernel is probably large enough to close any transient execution
window.


So sure, we can drop this.




> Cheers,
> -- 
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Herbert Xu]

On Thu, Jun 11, 2026 at 03:58:17AM -0400, Michael S. Tsirkin wrote:
> On Thu, Jun 11, 2026 at 03:46:58PM +0800, Herbert Xu wrote:
> > On Thu, Jun 11, 2026 at 03:30:14AM -0400, Michael S. Tsirkin wrote:
> > > On Thu, Jun 11, 2026 at 12:43:09PM +0800, Herbert Xu wrote:
> > > > On Sun, May 31, 2026 at 10:22:51AM -0400, Michael Bommarito wrote:
> > > > >
> > > > > +	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);
> > > 
> > > All the "malicious device" things are confusing. Spectre things -
> > > doubly so.
> > > 
> > > So if an access is speculated then CPU might speculate feeding a kernel
> > > secret into RNG. And then the speculated RNG value maybe can be also
> > > speculatively be used by some kernel code as an index
> > > to trigger a cache access, finally leaking the secret?
> > > 
> > > Maybe?
> > 
> > The way Spectre works is if you have an actual instruction using
> > idx directly.  I don't see how that translates to memcpy.
> 
> I am not sure it has to be direct:
> 
> if (malicious_idx > SIZE)
> 	return;
> src += malicious_idx;

Wait but vi->data_idx isn't even under the hypervisor's control.

It's an index maintained by our own driver.  So how can it be
malicious?

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Michael S. Tsirkin]

On Thu, Jun 11, 2026 at 04:18:46PM +0800, Herbert Xu wrote:
> On Thu, Jun 11, 2026 at 03:58:17AM -0400, Michael S. Tsirkin wrote:
> > On Thu, Jun 11, 2026 at 03:46:58PM +0800, Herbert Xu wrote:
> > > On Thu, Jun 11, 2026 at 03:30:14AM -0400, Michael S. Tsirkin wrote:
> > > > On Thu, Jun 11, 2026 at 12:43:09PM +0800, Herbert Xu wrote:
> > > > > On Sun, May 31, 2026 at 10:22:51AM -0400, Michael Bommarito wrote:
> > > > > >
> > > > > > +	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);
> > > > 
> > > > All the "malicious device" things are confusing. Spectre things -
> > > > doubly so.
> > > > 
> > > > So if an access is speculated then CPU might speculate feeding a kernel
> > > > secret into RNG. And then the speculated RNG value maybe can be also
> > > > speculatively be used by some kernel code as an index
> > > > to trigger a cache access, finally leaking the secret?
> > > > 
> > > > Maybe?
> > > 
> > > The way Spectre works is if you have an actual instruction using
> > > idx directly.  I don't see how that translates to memcpy.
> > 
> > I am not sure it has to be direct:
> > 
> > if (malicious_idx > SIZE)
> > 	return;
> > src += malicious_idx;
> 
> Wait but vi->data_idx isn't even under the hypervisor's control.
> 
> It's an index maintained by our own driver.  So how can it be
> malicious?
> 
> Cheers,
> -- 
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt


data_avail is under hypervisor control

        avail = min_t(unsigned int, vi->data_avail, sizeof(vi->data));
        if (vi->data_idx >= avail) {
        	vi->data_idx = 0;

and maybe this can speculate past the if?

I agree, this is all speculation )


-- 
MST

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

[Herbert Xu]

On Thu, Jun 11, 2026 at 05:10:32AM -0400, Michael S. Tsirkin wrote:
>
> data_avail is under hypervisor control
> 
>         avail = min_t(unsigned int, vi->data_avail, sizeof(vi->data));
>         if (vi->data_idx >= avail) {
>         	vi->data_idx = 0;
> 
> and maybe this can speculate past the if?
> 
> I agree, this is all speculation )

Either it is vulnerable to Spectre, or it isn't.  Adding nospec
markers when you're not sure is cargo cult programming.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt