RE: [PATCH 1/4] Drivers: hv: vmbus: Bounds check the shared ring buffer indices

[Michael Kelley]

From: Kameron Carr <kameroncarr@xxxxxxxxxxxxxxxxxxx> Sent: Thursday, October 1, 2026 3:11 PM
> 
> In a CoCo VM the host is untrusted. Since the VMBus ring buffer read and
> write indices live in shared memory, the guest has to treat both as
> potentially malicious.
> 
> hv_ringbuffer_write() copies into the ring at write_index with no bounds
> checking, so the host can make the guest write packet data at any offset
> up to 4 GiB past the start of the ring buffer. read_index matters too:
> the available space derived from both indices is the only bound on how
> much is copied, and an out-of-range index can make it far larger than
> the ring.
> 
> The fix is to validate both indices before use and to use the validated
> snapshot instead of re-accessing. For invalid indices,
> hv_ringbuffer_write() logs and returns -EIO.

There's one place in hv_ringbuffer_write() where you aren't using the
validated snapshot. Using the validated snapshot is deferred to Patch
2 of this series. I presume that's because the guest does not use that
value to index into the ring buffer. The value is only written to the ring
buffer as a kind of post-header for the packet. The Hyper-V host must
already be protecting itself by validating the packet that it reads from
the ring buffer, so presumably it would catch the bogus value. Net, it's
OK to read write_index again and use it unvalidated for this purpose.
But perhaps this situation should be noted in the commit message or
a code comment so someone later doesn't think it has been overlooked.

> 
> Open-coding the bytes available arithmetic keeps the fix free of
> prerequisites; a later patch refactors it into a helper function.
> 
> The unchecked write goes back to the commit in the Fixes tag, but the
> host is only untrusted in CoCo VMs, which Linux has supported since
> v5.12, so the stable tag starts at 5.15.x. This applies as-is to v5.15
> and later.

Arguably, this paragraph goes below the "---" since it is commentary
on the backport process rather than part of the description of the
commit.

> 
> Fixes: 3e7ee4902fe6 ("Staging: hv: add the Hyper-V virtual bus")
> Cc: <stable@xxxxxxxxxxxxxxx> # 5.15.x
> Signed-off-by: Kameron Carr <kameroncarr@xxxxxxxxxxxxxxxxxxx>

My discussion of the commit text notwithstanding,

Reviewed-by: Michael Kelley <mhklinux@xxxxxxxxxxx>

> ---
>  drivers/hv/ring_buffer.c | 29 ++++++++++++++++-------------
>  1 file changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
> index 592a960..a18b309 100644
> --- a/drivers/hv/ring_buffer.c
> +++ b/drivers/hv/ring_buffer.c
> @@ -70,15 +70,6 @@ static void hv_signal_on_write(u32 old_write, struct
> vmbus_channel *channel)
>  	}
>  }
> 
> -/* Get the next write location for the specified ring buffer. */
> -static inline u32
> -hv_get_next_write_location(struct hv_ring_buffer_info *ring_info)
> -{
> -	u32 next = ring_info->ring_buffer->write_index;
> -
> -	return next;
> -}
> -
>  /* Set the next write location for the specified ring buffer. */
>  static inline void
>  hv_set_next_write_location(struct hv_ring_buffer_info *ring_info,
> @@ -281,6 +272,7 @@ int hv_ringbuffer_write(struct vmbus_channel *channel,
>  	u32 totalbytes_towrite = sizeof(u64);
>  	u32 next_write_location;
>  	u32 old_write;
> +	u32 read_index;
>  	u64 prev_indices;
>  	unsigned long flags;
>  	struct hv_ring_buffer_info *outring_info = &channel->outbound;
> @@ -295,7 +287,20 @@ int hv_ringbuffer_write(struct vmbus_channel *channel,
> 
>  	spin_lock_irqsave(&outring_info->ring_lock, flags);
> 
> -	bytes_avail_towrite = hv_get_bytes_to_write(outring_info);
> +	read_index = READ_ONCE(outring_info->ring_buffer->read_index);
> +	old_write = READ_ONCE(outring_info->ring_buffer->write_index);
> +	if (unlikely(read_index >= outring_info->ring_datasize ||
> +		     old_write >= outring_info->ring_datasize)) {
> +		spin_unlock_irqrestore(&outring_info->ring_lock, flags);
> +		pr_err_ratelimited("outbound ring indices out of range: relid %u read
> %u write %u size %u\n",
> +				   channel->offermsg.child_relid, read_index,
> +				   old_write, outring_info->ring_datasize);
> +		return -EIO;
> +	}
> +
> +	bytes_avail_towrite = old_write >= read_index ?
> +		outring_info->ring_datasize - (old_write - read_index) :
> +		read_index - old_write;
> 
>  	/*
>  	 * If there is only room for the packet, assume it is full.
> @@ -317,9 +322,7 @@ int hv_ringbuffer_write(struct vmbus_channel *channel,
>  	channel->out_full_flag = false;
> 
>  	/* Write to the ring buffer */
> -	next_write_location = hv_get_next_write_location(outring_info);
> -
> -	old_write = next_write_location;
> +	next_write_location = old_write;
> 
>  	for (i = 0; i < kv_count; i++) {
>  		next_write_location = hv_copyto_ringbuffer(outring_info,
> 
> --
> 2.45.4