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