[PATCH 0/4] Drivers: hv: vmbus: Harden the ring buffer against a malicious host

[PATCH 0/4] Drivers: hv: vmbus: Harden the ring buffer against a malicious host

[Kameron Carr]

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 and as changing at any time. This patch series
adds bounds checking and reuses the validated indices instead of
re-accessing them.

Patch 1 contains the minimum security fix, and is the only patch in the
series intended to be backported. hv_ringbuffer_write() copies into the
ring at the write index, so a malicious host can make the guest write to
memory outside the ring buffer. This is reachable on any channel a CoCo
VM accepts.

Patches 2 and 3 add READ/WRITE_ONCE annotations and refactor the helper
functions to allow the caller to work with a consistent snapshot of the
ring buffer indices.

Patch 4 adds the rest of the bounds checking. The memcopy() in
hv_pkt_iter_avail() can only result in an out-of-bounds read if
rbi->pkt_buffer_size exceeds the ring's data size. KVP is the only
in-tree channel whose max_pkt_size exceeds its ring's data size (16K vs
12K on a 4K page guest). CoCo VMs reject the KVP channel, so this bug is
currently unreachable on CoCo VMs. The other paths patch 4 checks can't
cause a bad access, only a nonsense byte count or a wrong signaling
decision.

Patch 1 applies cleanly to v5.15 and later. The unchecked write goes
back to the original driver, but the host is only untrusted in CoCo VMs,
which Linux has supported since v5.12, so patch 1's Fixes tag points at
the original driver while its stable tag starts at 5.15.x.

---
Kameron Carr (4):
      Drivers: hv: vmbus: Bounds check the shared ring buffer indices
      Drivers: hv: vmbus: Annotate accesses to the shared ring buffer indices
      Drivers: hv: vmbus: Compute ring byte counts from a caller-held snapshot
      Drivers: hv: vmbus: Keep the ring byte counts sane for a bad index

 drivers/hv/ring_buffer.c | 112 +++++++++++++++++++++--------------------------
 include/linux/hyperv.h   |  58 ++++++++++++++++++------
 2 files changed, 94 insertions(+), 76 deletions(-)
---
base-commit: 72d3fcf802c45d00b300f25b848a93c3a2bd7c7e

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

[Kameron Carr]

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.

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.

Fixes: 3e7ee4902fe6 ("Staging: hv: add the Hyper-V virtual bus")
Cc: <stable@vger.kernel.org> # 5.15.x
Signed-off-by: Kameron Carr <kameroncarr@linux.microsoft.com>
---
 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

[PATCH 2/4] Drivers: hv: vmbus: Annotate accesses to the shared ring buffer indices

[Kameron Carr]

The ring buffer read/write indices live in a page shared with the host,
so the compiler must not split, merge or refetch accesses to them. Add
READ_ONCE()/WRITE_ONCE() in hv_set_next_write_location(),
hv_pkt_iter_close(), hv_get_bytes_to_read() and hv_get_bytes_to_write().
The accesses in hv_ringbuffer_get_debuginfo() are left to the next
patch.

Drop hv_get_ring_bufferindices(). It has one caller and is a one-line
expression on the write index. Inlining it moves the access to the call
site, so hv_ringbuffer_write() can reuse its validated snapshot of that
index, old_write, rather than reading the shared memory a second time.

No functional change intended for a well-behaved host.

Signed-off-by: Kameron Carr <kameroncarr@linux.microsoft.com>
---
 drivers/hv/ring_buffer.c | 15 ++++-----------
 include/linux/hyperv.h   |  4 ++--
 2 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index a18b309..7f466c5 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -75,7 +75,7 @@ static inline void
 hv_set_next_write_location(struct hv_ring_buffer_info *ring_info,
 		     u32 next_write_location)
 {
-	ring_info->ring_buffer->write_index = next_write_location;
+	WRITE_ONCE(ring_info->ring_buffer->write_index, next_write_location);
 }
 
 /* Get the size of the ring buffer. */
@@ -85,13 +85,6 @@ hv_get_ring_buffersize(const struct hv_ring_buffer_info *ring_info)
 	return ring_info->ring_datasize;
 }
 
-/* Get the read and write indices as u64 of the specified ring buffer. */
-static inline u64
-hv_get_ring_bufferindices(struct hv_ring_buffer_info *ring_info)
-{
-	return (u64)ring_info->ring_buffer->write_index << 32;
-}
-
 /*
  * Helper routine to copy from source to ring buffer.
  * Assume there is enough room. Handles wrap-around in dest case only!!
@@ -358,7 +351,7 @@ int hv_ringbuffer_write(struct vmbus_channel *channel,
 		*trans_id = __trans_id;
 
 	/* Set previous packet start */
-	prev_indices = hv_get_ring_bufferindices(outring_info);
+	prev_indices = (u64)old_write << 32;
 
 	next_write_location = hv_copyto_ringbuffer(outring_info,
 					     next_write_location,
@@ -582,8 +575,8 @@ void hv_pkt_iter_close(struct vmbus_channel *channel)
 	 * is updated.
 	 */
 	virt_rmb();
-	start_read_index = rbi->ring_buffer->read_index;
-	rbi->ring_buffer->read_index = rbi->priv_read_index;
+	start_read_index = READ_ONCE(rbi->ring_buffer->read_index);
+	WRITE_ONCE(rbi->ring_buffer->read_index, rbi->priv_read_index);
 
 	/*
 	 * Older versions of Hyper-V (before WS2102 and Win8) do not
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 9e109d9..5c65820 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -214,7 +214,7 @@ static inline u32 hv_get_bytes_to_read(const struct hv_ring_buffer_info *rbi)
 	u32 read_loc, write_loc, dsize, read;
 
 	dsize = rbi->ring_datasize;
-	read_loc = rbi->ring_buffer->read_index;
+	read_loc = READ_ONCE(rbi->ring_buffer->read_index);
 	write_loc = READ_ONCE(rbi->ring_buffer->write_index);
 
 	read = write_loc >= read_loc ? (write_loc - read_loc) :
@@ -229,7 +229,7 @@ static inline u32 hv_get_bytes_to_write(const struct hv_ring_buffer_info *rbi)
 
 	dsize = rbi->ring_datasize;
 	read_loc = READ_ONCE(rbi->ring_buffer->read_index);
-	write_loc = rbi->ring_buffer->write_index;
+	write_loc = READ_ONCE(rbi->ring_buffer->write_index);
 
 	write = write_loc >= read_loc ? dsize - (write_loc - read_loc) :
 		read_loc - write_loc;

-- 
2.45.4

[PATCH 3/4] Drivers: hv: vmbus: Compute ring byte counts from a caller-held snapshot

[Kameron Carr]

hv_get_ringbuffer_availbytes() reads the indices directly, so a caller
that also wants the index values has to read them a second time. The
host can change them in between, resulting in the byte counts and the
indices reflecting two different states of the ring.

Replace it with hv_ringbuffer_avail_write() and
hv_ringbuffer_avail_read(), which take the indices as arguments and
return a single count. They are separate because most callers want only
one of the two values. Use them in hv_ringbuffer_write() in place of the
open-coded equivalent. hv_get_bytes_to_read() and
hv_get_bytes_to_write() duplicated the same arithmetic, so put the
helpers in include/linux/hyperv.h and use them there too.

Add a hv_ringbuffer_index_valid() helper for bounds checking and use it
for the checks in hv_ringbuffer_write(). Convert the remaining callers
to use a single snapshot:

  - hv_ringbuffer_get_debuginfo() now reports the indices and the
    computed byte counts from the same snapshot.

  - hv_pkt_iter_close() computes the free space from priv_read_index
    instead of re-reading the shared read index. The two agree unless a
    misbehaving host has rewritten the value.

No functional change intended for a well-behaved host.

Signed-off-by: Kameron Carr <kameroncarr@linux.microsoft.com>
---
 drivers/hv/ring_buffer.c | 63 ++++++++++++++++--------------------------------
 include/linux/hyperv.h   | 48 +++++++++++++++++++++++++++---------
 2 files changed, 58 insertions(+), 53 deletions(-)

diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index 7f466c5..29edab9 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -107,35 +107,12 @@ static u32 hv_copyto_ringbuffer(
 	return start_write_offset;
 }
 
-/*
- *
- * hv_get_ringbuffer_availbytes()
- *
- * Get number of bytes available to read and to write to
- * for the specified ring buffer
- */
-static void
-hv_get_ringbuffer_availbytes(const struct hv_ring_buffer_info *rbi,
-			     u32 *read, u32 *write)
-{
-	u32 read_loc, write_loc, dsize;
-
-	/* Capture the read/write indices before they changed */
-	read_loc = READ_ONCE(rbi->ring_buffer->read_index);
-	write_loc = READ_ONCE(rbi->ring_buffer->write_index);
-	dsize = rbi->ring_datasize;
-
-	*write = write_loc >= read_loc ? dsize - (write_loc - read_loc) :
-		read_loc - write_loc;
-	*read = dsize - *write;
-}
-
 /* Get various debug metrics for the specified ring buffer. */
 int hv_ringbuffer_get_debuginfo(struct hv_ring_buffer_info *ring_info,
 				struct hv_ring_buffer_debug_info *debug_info)
 {
-	u32 bytes_avail_towrite;
-	u32 bytes_avail_toread;
+	u32 read_index;
+	u32 write_index;
 
 	mutex_lock(&ring_info->ring_buffer_mutex);
 
@@ -144,13 +121,14 @@ int hv_ringbuffer_get_debuginfo(struct hv_ring_buffer_info *ring_info,
 		return -EINVAL;
 	}
 
-	hv_get_ringbuffer_availbytes(ring_info,
-				     &bytes_avail_toread,
-				     &bytes_avail_towrite);
-	debug_info->bytes_avail_toread = bytes_avail_toread;
-	debug_info->bytes_avail_towrite = bytes_avail_towrite;
-	debug_info->current_read_index = ring_info->ring_buffer->read_index;
-	debug_info->current_write_index = ring_info->ring_buffer->write_index;
+	read_index = READ_ONCE(ring_info->ring_buffer->read_index);
+	write_index = READ_ONCE(ring_info->ring_buffer->write_index);
+	debug_info->bytes_avail_toread =
+		hv_ringbuffer_avail_read(ring_info, read_index, write_index);
+	debug_info->bytes_avail_towrite =
+		hv_ringbuffer_avail_write(ring_info, read_index, write_index);
+	debug_info->current_read_index = read_index;
+	debug_info->current_write_index = write_index;
 	debug_info->current_interrupt_mask
 		= ring_info->ring_buffer->interrupt_mask;
 	mutex_unlock(&ring_info->ring_buffer_mutex);
@@ -282,8 +260,8 @@ int hv_ringbuffer_write(struct vmbus_channel *channel,
 
 	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)) {
+	if (unlikely(!hv_ringbuffer_index_valid(outring_info, read_index) ||
+		     !hv_ringbuffer_index_valid(outring_info, old_write))) {
 		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,
@@ -291,9 +269,8 @@ int hv_ringbuffer_write(struct vmbus_channel *channel,
 		return -EIO;
 	}
 
-	bytes_avail_towrite = old_write >= read_index ?
-		outring_info->ring_datasize - (old_write - read_index) :
-		read_index - old_write;
+	bytes_avail_towrite = hv_ringbuffer_avail_write(outring_info, read_index,
+							old_write);
 
 	/*
 	 * If there is only room for the packet, assume it is full.
@@ -568,6 +545,7 @@ void hv_pkt_iter_close(struct vmbus_channel *channel)
 {
 	struct hv_ring_buffer_info *rbi = &channel->inbound;
 	u32 curr_write_sz, pending_sz, bytes_read, start_read_index;
+	u32 write_index;
 
 	/*
 	 * Make sure all reads are done before we update the read index since
@@ -607,11 +585,13 @@ void hv_pkt_iter_close(struct vmbus_channel *channel)
 		return;
 
 	/*
-	 * Ensure the read of write_index in hv_get_bytes_to_write()
-	 * happens after the read of pending_send_sz.
+	 * Ensure the read of write_index happens after the read of
+	 * pending_send_sz.
 	 */
 	virt_rmb();
-	curr_write_sz = hv_get_bytes_to_write(rbi);
+	write_index = READ_ONCE(rbi->ring_buffer->write_index);
+	curr_write_sz = hv_ringbuffer_avail_write(rbi, rbi->priv_read_index,
+						  write_index);
 	bytes_read = hv_pkt_iter_bytes_read(rbi, start_read_index);
 
 	/*
@@ -627,8 +607,7 @@ void hv_pkt_iter_close(struct vmbus_channel *channel)
 	 * Exactly filling the ring buffer is treated as "not enough
 	 * space". The ring buffer always must have at least one byte
 	 * empty so the empty and full conditions are distinguishable.
-	 * hv_get_bytes_to_write() doesn't fully tell the truth in
-	 * this regard.
+	 * curr_write_sz doesn't fully tell the truth in this regard.
 	 *
 	 * So first check if we were in the "enough free space" state
 	 * before we began the iteration. If so, the host was not
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 5c65820..9d7d09c 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -209,31 +209,57 @@ struct hv_ring_buffer_info {
 };
 
 
+/*
+ * The indices live in memory shared with the untrusted host, so check one
+ * before using it as an offset or to compute a byte count.
+ */
+static inline bool
+hv_ringbuffer_index_valid(const struct hv_ring_buffer_info *rbi, u32 index)
+{
+	return index < rbi->ring_datasize;
+}
+
+/*
+ * Byte counts for a caller-supplied snapshot of the indices, so that the
+ * counts and the indices the caller goes on to use describe one state of the
+ * ring.
+ */
+static inline u32
+hv_ringbuffer_avail_write(const struct hv_ring_buffer_info *rbi,
+			  u32 read_loc, u32 write_loc)
+{
+	u32 dsize = rbi->ring_datasize;
+
+	return write_loc >= read_loc ? dsize - (write_loc - read_loc) :
+		read_loc - write_loc;
+}
+
+static inline u32
+hv_ringbuffer_avail_read(const struct hv_ring_buffer_info *rbi,
+			 u32 read_loc, u32 write_loc)
+{
+	return rbi->ring_datasize -
+		hv_ringbuffer_avail_write(rbi, read_loc, write_loc);
+}
+
 static inline u32 hv_get_bytes_to_read(const struct hv_ring_buffer_info *rbi)
 {
-	u32 read_loc, write_loc, dsize, read;
+	u32 read_loc, write_loc;
 
-	dsize = rbi->ring_datasize;
 	read_loc = READ_ONCE(rbi->ring_buffer->read_index);
 	write_loc = READ_ONCE(rbi->ring_buffer->write_index);
 
-	read = write_loc >= read_loc ? (write_loc - read_loc) :
-		(dsize - read_loc) + write_loc;
-
-	return read;
+	return hv_ringbuffer_avail_read(rbi, read_loc, write_loc);
 }
 
 static inline u32 hv_get_bytes_to_write(const struct hv_ring_buffer_info *rbi)
 {
-	u32 read_loc, write_loc, dsize, write;
+	u32 read_loc, write_loc;
 
-	dsize = rbi->ring_datasize;
 	read_loc = READ_ONCE(rbi->ring_buffer->read_index);
 	write_loc = READ_ONCE(rbi->ring_buffer->write_index);
 
-	write = write_loc >= read_loc ? dsize - (write_loc - read_loc) :
-		read_loc - write_loc;
-	return write;
+	return hv_ringbuffer_avail_write(rbi, read_loc, write_loc);
 }
 
 static inline u32 hv_get_avail_to_write_percent(

-- 
2.45.4

[PATCH 4/4] Drivers: hv: vmbus: Keep the ring byte counts sane for a bad index

[Kameron Carr]

The remaining users of the shared indices can't cause a bad memory
access with the channels a CoCo VM accepts today, but an out-of-range
index gives them a nonsense byte count. Give them defined behavior
instead.

hv_pkt_iter_first() bounds its memcpy() by hv_pkt_iter_avail(), which
is derived from write_index, and by pkt_buffer_size, which comes from
max_pkt_size and can exceed ring_datasize. A bad write index can then
make it read past the end of the ring. Only KVP has such a max_pkt_size
(16K on a 12K ring with 4K pages), and vmbus_is_valid_offer() rejects
it in isolated VMs, so this is latent. hv_pkt_iter_avail() now reports
an empty ring for a bad write index and logs it, rate-limited like
hv_ringbuffer_write(). It takes the channel instead of the ring so the
message can include the relid.

hv_get_bytes_to_read() and hv_get_bytes_to_write() now return 0 for a
bad index, so callers see nothing to read and no room to write. This
also stops hv_end_read() from reporting data that hv_pkt_iter_first()
won't return, which would keep a channel rescheduling its callback.

hv_pkt_iter_close() only uses the indices to decide whether to signal
the host, so skip the signal if either is out of range.

Signed-off-by: Kameron Carr <kameroncarr@linux.microsoft.com>
---
 drivers/hv/ring_buffer.c | 15 +++++++++++++--
 include/linux/hyperv.h   |  6 ++++++
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index 29edab9..b54a7d3 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -408,8 +408,9 @@ int hv_ringbuffer_read(struct vmbus_channel *channel,
  * This is similar to hv_get_bytes_to_read but with private
  * read index instead.
  */
-static u32 hv_pkt_iter_avail(const struct hv_ring_buffer_info *rbi)
+static u32 hv_pkt_iter_avail(const struct vmbus_channel *channel)
 {
+	const struct hv_ring_buffer_info *rbi = &channel->inbound;
 	u32 priv_read_loc = rbi->priv_read_index;
 	u32 write_loc;
 
@@ -421,6 +422,12 @@ static u32 hv_pkt_iter_avail(const struct hv_ring_buffer_info *rbi)
 	 * stale data.
 	 */
 	write_loc = virt_load_acquire(&rbi->ring_buffer->write_index);
+	if (unlikely(!hv_ringbuffer_index_valid(rbi, write_loc))) {
+		pr_err_ratelimited("inbound write index out of range: relid %u write %u size %u\n",
+				   channel->offermsg.child_relid, write_loc,
+				   rbi->ring_datasize);
+		return 0;
+	}
 
 	if (write_loc >= priv_read_loc)
 		return write_loc - priv_read_loc;
@@ -441,7 +448,7 @@ struct vmpacket_descriptor *hv_pkt_iter_first(struct vmbus_channel *channel)
 
 	hv_debug_delay_test(channel, MESSAGE_DELAY);
 
-	bytes_avail = hv_pkt_iter_avail(rbi);
+	bytes_avail = hv_pkt_iter_avail(channel);
 	if (bytes_avail < sizeof(struct vmpacket_descriptor))
 		return NULL;
 	bytes_avail = min(rbi->pkt_buffer_size, bytes_avail);
@@ -590,6 +597,10 @@ void hv_pkt_iter_close(struct vmbus_channel *channel)
 	 */
 	virt_rmb();
 	write_index = READ_ONCE(rbi->ring_buffer->write_index);
+	if (unlikely(!hv_ringbuffer_index_valid(rbi, write_index) ||
+		     !hv_ringbuffer_index_valid(rbi, start_read_index)))
+		return;
+
 	curr_write_sz = hv_ringbuffer_avail_write(rbi, rbi->priv_read_index,
 						  write_index);
 	bytes_read = hv_pkt_iter_bytes_read(rbi, start_read_index);
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 9d7d09c..fd61382 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -248,6 +248,9 @@ static inline u32 hv_get_bytes_to_read(const struct hv_ring_buffer_info *rbi)
 
 	read_loc = READ_ONCE(rbi->ring_buffer->read_index);
 	write_loc = READ_ONCE(rbi->ring_buffer->write_index);
+	if (unlikely(!hv_ringbuffer_index_valid(rbi, read_loc) ||
+		     !hv_ringbuffer_index_valid(rbi, write_loc)))
+		return 0;
 
 	return hv_ringbuffer_avail_read(rbi, read_loc, write_loc);
 }
@@ -258,6 +261,9 @@ static inline u32 hv_get_bytes_to_write(const struct hv_ring_buffer_info *rbi)
 
 	read_loc = READ_ONCE(rbi->ring_buffer->read_index);
 	write_loc = READ_ONCE(rbi->ring_buffer->write_index);
+	if (unlikely(!hv_ringbuffer_index_valid(rbi, read_loc) ||
+		     !hv_ringbuffer_index_valid(rbi, write_loc)))
+		return 0;
 
 	return hv_ringbuffer_avail_write(rbi, read_loc, write_loc);
 }

-- 
2.45.4

RE: [PATCH 0/4] Drivers: hv: vmbus: Harden the ring buffer against a malicious host

[Michael Kelley]

From: Kameron Carr <kameroncarr@linux.microsoft.com> 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 and as changing at any time. This patch series
> adds bounds checking and reuses the validated indices instead of
> re-accessing them.

I worked quite a bit on VMBus hardening for CoCo VMs back in 2021 and
2022. When I first saw the Subject of your patch set, I was a bit incredulous.
Surely we had not missed this rather obvious vulnerability! But we certainly
did, and I can't help but be a little embarrassed. :-( Thanks for putting things right.

I think this topic deserves a mention in Documentation/virt/hyperv/coco.rst.
There's a section entitled "Guest communication with Hyper-V" with
this paragraph:

   Similarly, when the guest reads memory that is shared with the host, it must
   validate the data before acting on it so that a malicious host cannot induce
   the guest to expose unintended data. Doing such validation can be tricky
   because the host can modify the shared memory areas even while or after
   validation is performed. For messages passed from the host to the guest in a
   VMBus ring buffer, the length of the message is validated, and the message is
   copied into a temporary (encrypted) buffer for further validation and
   processing. The copying adds a small amount of overhead, but is the only way
   to protect against a malicious host. See hv_pkt_iter_first().

This documentation is only a high-level overview, but maybe add a sentence
such as:

  Also, because the ring buffer header is shared with the host, the guest
  must validate the read and indices before indexing into the ring buffer to
  send or receive messages, or when using them to compute available space.

Overall this series is well done. I have only a couple of minor quibbles
or suggestions on individual patches, and I've given my "Reviewed-by" on
all patches.

Michael

> 
> Patch 1 contains the minimum security fix, and is the only patch in the
> series intended to be backported. hv_ringbuffer_write() copies into the
> ring at the write index, so a malicious host can make the guest write to
> memory outside the ring buffer. This is reachable on any channel a CoCo
> VM accepts.
> 
> Patches 2 and 3 add READ/WRITE_ONCE annotations and refactor the helper
> functions to allow the caller to work with a consistent snapshot of the
> ring buffer indices.
> 
> Patch 4 adds the rest of the bounds checking. The memcopy() in
> hv_pkt_iter_avail() can only result in an out-of-bounds read if
> rbi->pkt_buffer_size exceeds the ring's data size. KVP is the only
> in-tree channel whose max_pkt_size exceeds its ring's data size (16K vs
> 12K on a 4K page guest). CoCo VMs reject the KVP channel, so this bug is
> currently unreachable on CoCo VMs. The other paths patch 4 checks can't
> cause a bad access, only a nonsense byte count or a wrong signaling
> decision.
> 
> Patch 1 applies cleanly to v5.15 and later. The unchecked write goes
> back to the original driver, but the host is only untrusted in CoCo VMs,
> which Linux has supported since v5.12, so patch 1's Fixes tag points at
> the original driver while its stable tag starts at 5.15.x.
> 
> ---
> Kameron Carr (4):
>       Drivers: hv: vmbus: Bounds check the shared ring buffer indices
>       Drivers: hv: vmbus: Annotate accesses to the shared ring buffer indices
>       Drivers: hv: vmbus: Compute ring byte counts from a caller-held snapshot
>       Drivers: hv: vmbus: Keep the ring byte counts sane for a bad index
> 
>  drivers/hv/ring_buffer.c | 112 +++++++++++++++++++++--------------------------
>  include/linux/hyperv.h   |  58 ++++++++++++++++++------
>  2 files changed, 94 insertions(+), 76 deletions(-)
> ---
> base-commit: 72d3fcf802c45d00b300f25b848a93c3a2bd7c7e

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

[Michael Kelley]

From: Kameron Carr <kameroncarr@linux.microsoft.com> 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@vger.kernel.org> # 5.15.x
> Signed-off-by: Kameron Carr <kameroncarr@linux.microsoft.com>

My discussion of the commit text notwithstanding,

Reviewed-by: Michael Kelley <mhklinux@outlook.com>

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

RE: [PATCH 2/4] Drivers: hv: vmbus: Annotate accesses to the shared ring buffer indices

[Michael Kelley]

From: Kameron Carr <kameroncarr@linux.microsoft.com> Sent: Thursday, October 1, 2026 3:11 PM
> 

I have minor quibble with the patch "Subject". To me, "Annotate"
refers to adding metadata to the code, usually for use by some analysis
tool. READ/WRITE_ONCE() aren't annotations in that sense in that they
may affect the generated code and may be needed for correctness of
the code. As an alternative Subject, how about:

    Drivers: hv: vmbus: Use *_ONCE() to access shared ring buffer indices

But maybe my interpretation of "Annotate" is skewed ....

> The ring buffer read/write indices live in a page shared with the host,
> so the compiler must not split, merge or refetch accesses to them. Add
> READ_ONCE()/WRITE_ONCE() in hv_set_next_write_location(),
> hv_pkt_iter_close(), hv_get_bytes_to_read() and hv_get_bytes_to_write().
> The accesses in hv_ringbuffer_get_debuginfo() are left to the next
> patch.
> 
> Drop hv_get_ring_bufferindices(). It has one caller and is a one-line
> expression on the write index. Inlining it moves the access to the call
> site, so hv_ringbuffer_write() can reuse its validated snapshot of that
> index, old_write, rather than reading the shared memory a second time.
> 
> No functional change intended for a well-behaved host.
> 
> Signed-off-by: Kameron Carr <kameroncarr@linux.microsoft.com>

My quibble with the Subject notwithstanding,

Reviewed-by: Michael Kelley <mhklinux@outlook.com>

> ---
>  drivers/hv/ring_buffer.c | 15 ++++-----------
>  include/linux/hyperv.h   |  4 ++--
>  2 files changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
> index a18b309..7f466c5 100644
> --- a/drivers/hv/ring_buffer.c
> +++ b/drivers/hv/ring_buffer.c
> @@ -75,7 +75,7 @@ static inline void
>  hv_set_next_write_location(struct hv_ring_buffer_info *ring_info,
>  		     u32 next_write_location)
>  {
> -	ring_info->ring_buffer->write_index = next_write_location;
> +	WRITE_ONCE(ring_info->ring_buffer->write_index, next_write_location);
>  }
> 
>  /* Get the size of the ring buffer. */
> @@ -85,13 +85,6 @@ hv_get_ring_buffersize(const struct hv_ring_buffer_info *ring_info)
>  	return ring_info->ring_datasize;
>  }
> 
> -/* Get the read and write indices as u64 of the specified ring buffer. */
> -static inline u64
> -hv_get_ring_bufferindices(struct hv_ring_buffer_info *ring_info)
> -{
> -	return (u64)ring_info->ring_buffer->write_index << 32;
> -}
> -
>  /*
>   * Helper routine to copy from source to ring buffer.
>   * Assume there is enough room. Handles wrap-around in dest case only!!
> @@ -358,7 +351,7 @@ int hv_ringbuffer_write(struct vmbus_channel *channel,
>  		*trans_id = __trans_id;
> 
>  	/* Set previous packet start */
> -	prev_indices = hv_get_ring_bufferindices(outring_info);
> +	prev_indices = (u64)old_write << 32;
> 
>  	next_write_location = hv_copyto_ringbuffer(outring_info,
>  					     next_write_location,
> @@ -582,8 +575,8 @@ void hv_pkt_iter_close(struct vmbus_channel *channel)
>  	 * is updated.
>  	 */
>  	virt_rmb();
> -	start_read_index = rbi->ring_buffer->read_index;
> -	rbi->ring_buffer->read_index = rbi->priv_read_index;
> +	start_read_index = READ_ONCE(rbi->ring_buffer->read_index);
> +	WRITE_ONCE(rbi->ring_buffer->read_index, rbi->priv_read_index);
> 
>  	/*
>  	 * Older versions of Hyper-V (before WS2102 and Win8) do not
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index 9e109d9..5c65820 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -214,7 +214,7 @@ static inline u32 hv_get_bytes_to_read(const struct
> hv_ring_buffer_info *rbi)
>  	u32 read_loc, write_loc, dsize, read;
> 
>  	dsize = rbi->ring_datasize;
> -	read_loc = rbi->ring_buffer->read_index;
> +	read_loc = READ_ONCE(rbi->ring_buffer->read_index);
>  	write_loc = READ_ONCE(rbi->ring_buffer->write_index);
> 
>  	read = write_loc >= read_loc ? (write_loc - read_loc) :
> @@ -229,7 +229,7 @@ static inline u32 hv_get_bytes_to_write(const struct
> hv_ring_buffer_info *rbi)
> 
>  	dsize = rbi->ring_datasize;
>  	read_loc = READ_ONCE(rbi->ring_buffer->read_index);
> -	write_loc = rbi->ring_buffer->write_index;
> +	write_loc = READ_ONCE(rbi->ring_buffer->write_index);
> 
>  	write = write_loc >= read_loc ? dsize - (write_loc - read_loc) :
>  		read_loc - write_loc;
> 
> --
> 2.45.4

RE: [PATCH 3/4] Drivers: hv: vmbus: Compute ring byte counts from a caller-held snapshot

[Michael Kelley]

From: Kameron Carr <kameroncarr@linux.microsoft.com> Sent: Thursday, October 1, 2026 3:11 PM
> To: Haiyang Zhang <haiyangz@microsoft.com>; Wei Liu <wei.liu@kernel.org>; Dexuan Cui
> <decui@microsoft.com>; Long Li <longli@microsoft.com>; Michael Kelley
> <mikelley@microsoft.com>
> Cc: linux-hyperv@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [PATCH 3/4] Drivers: hv: vmbus: Compute ring byte counts from a caller-held
> snapshot
> 
> hv_get_ringbuffer_availbytes() reads the indices directly, so a caller
> that also wants the index values has to read them a second time. The
> host can change them in between, resulting in the byte counts and the
> indices reflecting two different states of the ring.
> 
> Replace it with hv_ringbuffer_avail_write() and
> hv_ringbuffer_avail_read(), which take the indices as arguments and
> return a single count. They are separate because most callers want only
> one of the two values. Use them in hv_ringbuffer_write() in place of the
> open-coded equivalent. hv_get_bytes_to_read() and
> hv_get_bytes_to_write() duplicated the same arithmetic, so put the
> helpers in include/linux/hyperv.h and use them there too.
> 
> Add a hv_ringbuffer_index_valid() helper for bounds checking and use it
> for the checks in hv_ringbuffer_write(). 

With all patches in this series applied, there are 5 places that
hv_ringbuffer_index_valid() is used. Four of those places check two indices --
a read index and a write index. One place checks just a single index. I'd
suggest having hv_ringbuffer_index_valid() check two indices instead of
just one. For the one place that checks a single index, just pass zero for the
other index. And flip the polarity -- make it hv_ringbuffer_indices_invalid()
so that it doesn't need a "!" in front of every usage. (For me anyway,
that's slightly less cognitive load when reading the code.)

This is just a suggestion. If you prefer to keep the code like it is,
I won't object.

> Convert the remaining callers
> to use a single snapshot:
> 
>   - hv_ringbuffer_get_debuginfo() now reports the indices and the
>     computed byte counts from the same snapshot.
> 
>   - hv_pkt_iter_close() computes the free space from priv_read_index
>     instead of re-reading the shared read index. The two agree unless a
>     misbehaving host has rewritten the value.
> 
> No functional change intended for a well-behaved host.
> 
> Signed-off-by: Kameron Carr <kameroncarr@linux.microsoft.com>

Regardless of whether you want to take my suggestion,

Reviewed-by: Michael Kelley <mhklinux@outlook.com>

> ---
>  drivers/hv/ring_buffer.c | 63 ++++++++++++++++--------------------------------
>  include/linux/hyperv.h   | 48 +++++++++++++++++++++++++++---------
>  2 files changed, 58 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
> index 7f466c5..29edab9 100644
> --- a/drivers/hv/ring_buffer.c
> +++ b/drivers/hv/ring_buffer.c
> @@ -107,35 +107,12 @@ static u32 hv_copyto_ringbuffer(
>  	return start_write_offset;
>  }
> 
> -/*
> - *
> - * hv_get_ringbuffer_availbytes()
> - *
> - * Get number of bytes available to read and to write to
> - * for the specified ring buffer
> - */
> -static void
> -hv_get_ringbuffer_availbytes(const struct hv_ring_buffer_info *rbi,
> -			     u32 *read, u32 *write)
> -{
> -	u32 read_loc, write_loc, dsize;
> -
> -	/* Capture the read/write indices before they changed */
> -	read_loc = READ_ONCE(rbi->ring_buffer->read_index);
> -	write_loc = READ_ONCE(rbi->ring_buffer->write_index);
> -	dsize = rbi->ring_datasize;
> -
> -	*write = write_loc >= read_loc ? dsize - (write_loc - read_loc) :
> -		read_loc - write_loc;
> -	*read = dsize - *write;
> -}
> -
>  /* Get various debug metrics for the specified ring buffer. */
>  int hv_ringbuffer_get_debuginfo(struct hv_ring_buffer_info *ring_info,
>  				struct hv_ring_buffer_debug_info *debug_info)
>  {
> -	u32 bytes_avail_towrite;
> -	u32 bytes_avail_toread;
> +	u32 read_index;
> +	u32 write_index;
> 
>  	mutex_lock(&ring_info->ring_buffer_mutex);
> 
> @@ -144,13 +121,14 @@ int hv_ringbuffer_get_debuginfo(struct hv_ring_buffer_info
> *ring_info,
>  		return -EINVAL;
>  	}
> 
> -	hv_get_ringbuffer_availbytes(ring_info,
> -				     &bytes_avail_toread,
> -				     &bytes_avail_towrite);
> -	debug_info->bytes_avail_toread = bytes_avail_toread;
> -	debug_info->bytes_avail_towrite = bytes_avail_towrite;
> -	debug_info->current_read_index = ring_info->ring_buffer->read_index;
> -	debug_info->current_write_index = ring_info->ring_buffer->write_index;
> +	read_index = READ_ONCE(ring_info->ring_buffer->read_index);
> +	write_index = READ_ONCE(ring_info->ring_buffer->write_index);
> +	debug_info->bytes_avail_toread =
> +		hv_ringbuffer_avail_read(ring_info, read_index, write_index);
> +	debug_info->bytes_avail_towrite =
> +		hv_ringbuffer_avail_write(ring_info, read_index, write_index);
> +	debug_info->current_read_index = read_index;
> +	debug_info->current_write_index = write_index;
>  	debug_info->current_interrupt_mask
>  		= ring_info->ring_buffer->interrupt_mask;
>  	mutex_unlock(&ring_info->ring_buffer_mutex);
> @@ -282,8 +260,8 @@ int hv_ringbuffer_write(struct vmbus_channel *channel,
> 
>  	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)) {
> +	if (unlikely(!hv_ringbuffer_index_valid(outring_info, read_index) ||
> +		     !hv_ringbuffer_index_valid(outring_info, old_write))) {
>  		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,
> @@ -291,9 +269,8 @@ int hv_ringbuffer_write(struct vmbus_channel *channel,
>  		return -EIO;
>  	}
> 
> -	bytes_avail_towrite = old_write >= read_index ?
> -		outring_info->ring_datasize - (old_write - read_index) :
> -		read_index - old_write;
> +	bytes_avail_towrite = hv_ringbuffer_avail_write(outring_info, read_index,
> +							old_write);
> 
>  	/*
>  	 * If there is only room for the packet, assume it is full.
> @@ -568,6 +545,7 @@ void hv_pkt_iter_close(struct vmbus_channel *channel)
>  {
>  	struct hv_ring_buffer_info *rbi = &channel->inbound;
>  	u32 curr_write_sz, pending_sz, bytes_read, start_read_index;
> +	u32 write_index;
> 
>  	/*
>  	 * Make sure all reads are done before we update the read index since
> @@ -607,11 +585,13 @@ void hv_pkt_iter_close(struct vmbus_channel *channel)
>  		return;
> 
>  	/*
> -	 * Ensure the read of write_index in hv_get_bytes_to_write()
> -	 * happens after the read of pending_send_sz.
> +	 * Ensure the read of write_index happens after the read of
> +	 * pending_send_sz.
>  	 */
>  	virt_rmb();
> -	curr_write_sz = hv_get_bytes_to_write(rbi);
> +	write_index = READ_ONCE(rbi->ring_buffer->write_index);
> +	curr_write_sz = hv_ringbuffer_avail_write(rbi, rbi->priv_read_index,
> +						  write_index);
>  	bytes_read = hv_pkt_iter_bytes_read(rbi, start_read_index);
> 
>  	/*
> @@ -627,8 +607,7 @@ void hv_pkt_iter_close(struct vmbus_channel *channel)
>  	 * Exactly filling the ring buffer is treated as "not enough
>  	 * space". The ring buffer always must have at least one byte
>  	 * empty so the empty and full conditions are distinguishable.
> -	 * hv_get_bytes_to_write() doesn't fully tell the truth in
> -	 * this regard.
> +	 * curr_write_sz doesn't fully tell the truth in this regard.
>  	 *
>  	 * So first check if we were in the "enough free space" state
>  	 * before we began the iteration. If so, the host was not
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index 5c65820..9d7d09c 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -209,31 +209,57 @@ struct hv_ring_buffer_info {
>  };
> 
> 
> +/*
> + * The indices live in memory shared with the untrusted host, so check one
> + * before using it as an offset or to compute a byte count.
> + */
> +static inline bool
> +hv_ringbuffer_index_valid(const struct hv_ring_buffer_info *rbi, u32 index)
> +{
> +	return index < rbi->ring_datasize;
> +}
> +
> +/*
> + * Byte counts for a caller-supplied snapshot of the indices, so that the
> + * counts and the indices the caller goes on to use describe one state of the
> + * ring.
> + */
> +static inline u32
> +hv_ringbuffer_avail_write(const struct hv_ring_buffer_info *rbi,
> +			  u32 read_loc, u32 write_loc)
> +{
> +	u32 dsize = rbi->ring_datasize;
> +
> +	return write_loc >= read_loc ? dsize - (write_loc - read_loc) :
> +		read_loc - write_loc;
> +}
> +
> +static inline u32
> +hv_ringbuffer_avail_read(const struct hv_ring_buffer_info *rbi,
> +			 u32 read_loc, u32 write_loc)
> +{
> +	return rbi->ring_datasize -
> +		hv_ringbuffer_avail_write(rbi, read_loc, write_loc);
> +}
> +
>  static inline u32 hv_get_bytes_to_read(const struct hv_ring_buffer_info *rbi)
>  {
> -	u32 read_loc, write_loc, dsize, read;
> +	u32 read_loc, write_loc;
> 
> -	dsize = rbi->ring_datasize;
>  	read_loc = READ_ONCE(rbi->ring_buffer->read_index);
>  	write_loc = READ_ONCE(rbi->ring_buffer->write_index);
> 
> -	read = write_loc >= read_loc ? (write_loc - read_loc) :
> -		(dsize - read_loc) + write_loc;
> -
> -	return read;
> +	return hv_ringbuffer_avail_read(rbi, read_loc, write_loc);
>  }
> 
>  static inline u32 hv_get_bytes_to_write(const struct hv_ring_buffer_info *rbi)
>  {
> -	u32 read_loc, write_loc, dsize, write;
> +	u32 read_loc, write_loc;
> 
> -	dsize = rbi->ring_datasize;
>  	read_loc = READ_ONCE(rbi->ring_buffer->read_index);
>  	write_loc = READ_ONCE(rbi->ring_buffer->write_index);
> 
> -	write = write_loc >= read_loc ? dsize - (write_loc - read_loc) :
> -		read_loc - write_loc;
> -	return write;
> +	return hv_ringbuffer_avail_write(rbi, read_loc, write_loc);
>  }
> 
>  static inline u32 hv_get_avail_to_write_percent(
> 
> --
> 2.45.4

RE: [PATCH 4/4] Drivers: hv: vmbus: Keep the ring byte counts sane for a bad index

[Michael Kelley]

From: Kameron Carr <kameroncarr@linux.microsoft.com> Sent: Thursday, October 1, 2026 3:11 PM
> 
> The remaining users of the shared indices can't cause a bad memory
> access with the channels a CoCo VM accepts today, but an out-of-range
> index gives them a nonsense byte count. Give them defined behavior
> instead.
> 
> hv_pkt_iter_first() bounds its memcpy() by hv_pkt_iter_avail(), which
> is derived from write_index, and by pkt_buffer_size, which comes from
> max_pkt_size and can exceed ring_datasize. A bad write index can then
> make it read past the end of the ring. Only KVP has such a max_pkt_size
> (16K on a 12K ring with 4K pages), and vmbus_is_valid_offer() rejects
> it in isolated VMs, so this is latent. hv_pkt_iter_avail() now reports
> an empty ring for a bad write index and logs it, rate-limited like
> hv_ringbuffer_write(). It takes the channel instead of the ring so the
> message can include the relid.
> 
> hv_get_bytes_to_read() and hv_get_bytes_to_write() now return 0 for a
> bad index, so callers see nothing to read and no room to write. This
> also stops hv_end_read() from reporting data that hv_pkt_iter_first()
> won't return, which would keep a channel rescheduling its callback.
> 
> hv_pkt_iter_close() only uses the indices to decide whether to signal
> the host, so skip the signal if either is out of range.
> 
> Signed-off-by: Kameron Carr <kameroncarr@linux.microsoft.com>

Reviewed-by: Michael Kelley <mhklinux@outlook.com>

> ---
>  drivers/hv/ring_buffer.c | 15 +++++++++++++--
>  include/linux/hyperv.h   |  6 ++++++
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
> index 29edab9..b54a7d3 100644
> --- a/drivers/hv/ring_buffer.c
> +++ b/drivers/hv/ring_buffer.c
> @@ -408,8 +408,9 @@ int hv_ringbuffer_read(struct vmbus_channel *channel,
>   * This is similar to hv_get_bytes_to_read but with private
>   * read index instead.
>   */
> -static u32 hv_pkt_iter_avail(const struct hv_ring_buffer_info *rbi)
> +static u32 hv_pkt_iter_avail(const struct vmbus_channel *channel)
>  {
> +	const struct hv_ring_buffer_info *rbi = &channel->inbound;
>  	u32 priv_read_loc = rbi->priv_read_index;
>  	u32 write_loc;
> 
> @@ -421,6 +422,12 @@ static u32 hv_pkt_iter_avail(const struct hv_ring_buffer_info *rbi)
>  	 * stale data.
>  	 */
>  	write_loc = virt_load_acquire(&rbi->ring_buffer->write_index);
> +	if (unlikely(!hv_ringbuffer_index_valid(rbi, write_loc))) {
> +		pr_err_ratelimited("inbound write index out of range: relid %u write %u size %u\n",
> +				   channel->offermsg.child_relid, write_loc,
> +				   rbi->ring_datasize);
> +		return 0;
> +	}
> 
>  	if (write_loc >= priv_read_loc)
>  		return write_loc - priv_read_loc;
> @@ -441,7 +448,7 @@ struct vmpacket_descriptor *hv_pkt_iter_first(struct vmbus_channel *channel)
> 
>  	hv_debug_delay_test(channel, MESSAGE_DELAY);
> 
> -	bytes_avail = hv_pkt_iter_avail(rbi);
> +	bytes_avail = hv_pkt_iter_avail(channel);
>  	if (bytes_avail < sizeof(struct vmpacket_descriptor))
>  		return NULL;
>  	bytes_avail = min(rbi->pkt_buffer_size, bytes_avail);
> @@ -590,6 +597,10 @@ void hv_pkt_iter_close(struct vmbus_channel *channel)
>  	 */
>  	virt_rmb();
>  	write_index = READ_ONCE(rbi->ring_buffer->write_index);
> +	if (unlikely(!hv_ringbuffer_index_valid(rbi, write_index) ||
> +		     !hv_ringbuffer_index_valid(rbi, start_read_index)))
> +		return;
> +
>  	curr_write_sz = hv_ringbuffer_avail_write(rbi, rbi->priv_read_index,
>  						  write_index);
>  	bytes_read = hv_pkt_iter_bytes_read(rbi, start_read_index);
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index 9d7d09c..fd61382 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -248,6 +248,9 @@ static inline u32 hv_get_bytes_to_read(const struct hv_ring_buffer_info *rbi)
> 
>  	read_loc = READ_ONCE(rbi->ring_buffer->read_index);
>  	write_loc = READ_ONCE(rbi->ring_buffer->write_index);
> +	if (unlikely(!hv_ringbuffer_index_valid(rbi, read_loc) ||
> +		     !hv_ringbuffer_index_valid(rbi, write_loc)))
> +		return 0;
> 
>  	return hv_ringbuffer_avail_read(rbi, read_loc, write_loc);
>  }
> @@ -258,6 +261,9 @@ static inline u32 hv_get_bytes_to_write(const struct hv_ring_buffer_info *rbi)
> 
>  	read_loc = READ_ONCE(rbi->ring_buffer->read_index);
>  	write_loc = READ_ONCE(rbi->ring_buffer->write_index);
> +	if (unlikely(!hv_ringbuffer_index_valid(rbi, read_loc) ||
> +		     !hv_ringbuffer_index_valid(rbi, write_loc)))
> +		return 0;
> 
>  	return hv_ringbuffer_avail_write(rbi, read_loc, write_loc);
>  }
> 
> --
> 2.45.4