[PATCH] virtio_net: validate device stats reply records before use

[PATCH] virtio_net: validate device stats reply records before use

[Michael Bommarito]

__virtnet_get_hw_stats() walks the device statistics reply buffer with
"for (p = reply; p - reply < res_size; p += le16_to_cpu(hdr->size))",
using each record's device-supplied hdr->size as the stride without
checking that a full struct virtio_net_stats_reply_hdr remains, that
hdr->size is nonzero and matches the expected size for hdr->type, or that
the record fits within res_size. A backend that returns hdr->size == 0
spins the loop forever; a short or oversized size drives out-of-bounds
reads in virtnet_fill_stats().

Impact: a malicious or compromised virtio-net backend hangs the CPU
running the guest's device-statistics query in an infinite loop
(hdr->size == 0), or drives an out-of-bounds read of the reply buffer.
This matters most for a confidential guest, where the host is outside the
trust boundary.

Validate each record before use: require a full header in the remaining
bytes, a nonzero hdr->size that is at least the header size and matches the
size expected for hdr->type, and that the record fits within res_size; stop
the walk otherwise. Add virtnet_stats_reply_size() for the per-type size.

Fixes: 941168f8b40e ("virtio_net: support device stats")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
 drivers/net/virtio_net.c | 42 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 3e2a5876c6c8c..9cbe40d218cc4 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -3532,6 +3532,7 @@ static int virtnet_tx_resize(struct virtnet_info *vi, struct send_queue *sq,
 	return err;
 }
 
+
 /*
  * Send command via the control virtqueue and check status.  Commands
  * supported by the hypervisor, as indicated by feature bits, should
@@ -3546,6 +3547,7 @@ static bool virtnet_send_command_reply(struct virtnet_info *vi, u8 class, u8 cmd
 	bool ok;
 	int ret;
 
+
 	/* Caller should know better */
 	BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ));
 
@@ -4927,6 +4929,32 @@ static void virtnet_fill_stats(struct virtnet_info *vi, u32 qid,
 	}
 }
 
+static int virtnet_stats_reply_size(u8 type)
+{
+	switch (type) {
+	case VIRTIO_NET_STATS_TYPE_REPLY_CVQ:
+		return sizeof(struct virtio_net_stats_cvq);
+	case VIRTIO_NET_STATS_TYPE_REPLY_RX_BASIC:
+		return sizeof(struct virtio_net_stats_rx_basic);
+	case VIRTIO_NET_STATS_TYPE_REPLY_RX_CSUM:
+		return sizeof(struct virtio_net_stats_rx_csum);
+	case VIRTIO_NET_STATS_TYPE_REPLY_RX_GSO:
+		return sizeof(struct virtio_net_stats_rx_gso);
+	case VIRTIO_NET_STATS_TYPE_REPLY_RX_SPEED:
+		return sizeof(struct virtio_net_stats_rx_speed);
+	case VIRTIO_NET_STATS_TYPE_REPLY_TX_BASIC:
+		return sizeof(struct virtio_net_stats_tx_basic);
+	case VIRTIO_NET_STATS_TYPE_REPLY_TX_CSUM:
+		return sizeof(struct virtio_net_stats_tx_csum);
+	case VIRTIO_NET_STATS_TYPE_REPLY_TX_GSO:
+		return sizeof(struct virtio_net_stats_tx_gso);
+	case VIRTIO_NET_STATS_TYPE_REPLY_TX_SPEED:
+		return sizeof(struct virtio_net_stats_tx_speed);
+	default:
+		return sizeof(struct virtio_net_stats_reply_hdr);
+	}
+}
+
 static int __virtnet_get_hw_stats(struct virtnet_info *vi,
 				  struct virtnet_stats_ctx *ctx,
 				  struct virtio_net_ctrl_queue_stats *req,
@@ -4936,7 +4964,7 @@ static int __virtnet_get_hw_stats(struct virtnet_info *vi,
 	struct scatterlist sgs_in, sgs_out;
 	void *p;
 	u32 qid;
-	int ok;
+	int hdr_size, ok, remaining;
 
 	sg_init_one(&sgs_out, req, req_size);
 	sg_init_one(&sgs_in, reply, res_size);
@@ -4948,8 +4976,17 @@ static int __virtnet_get_hw_stats(struct virtnet_info *vi,
 	if (!ok)
 		return ok;
 
-	for (p = reply; p - reply < res_size; p += le16_to_cpu(hdr->size)) {
+	for (p = reply; p - reply < res_size; p += hdr_size) {
+		remaining = res_size - (p - reply);
+		if (remaining < sizeof(*hdr))
+			return -EINVAL;
+
 		hdr = p;
+		hdr_size = le16_to_cpu(hdr->size);
+		if (hdr_size < virtnet_stats_reply_size(hdr->type) ||
+		    hdr_size > remaining)
+			return -EINVAL;
+
 		qid = le16_to_cpu(hdr->vq_index);
 		virtnet_fill_stats(vi, qid, ctx, p, false, hdr->type);
 	}
@@ -7305,3 +7342,4 @@ module_exit(virtio_net_driver_exit);
 MODULE_DEVICE_TABLE(virtio, id_table);
 MODULE_DESCRIPTION("Virtio network driver");
 MODULE_LICENSE("GPL");
+
-- 
2.53.0

Re: [PATCH] virtio_net: validate device stats reply records before use

[Michael S. Tsirkin]

On Sat, Jul 11, 2026 at 11:07:54AM -0400, Michael Bommarito wrote:
> __virtnet_get_hw_stats() walks the device statistics reply buffer with
> "for (p = reply; p - reply < res_size; p += le16_to_cpu(hdr->size))",
> using each record's device-supplied hdr->size as the stride without
> checking that a full struct virtio_net_stats_reply_hdr remains, that
> hdr->size is nonzero and matches the expected size for hdr->type, or that
> the record fits within res_size. A backend that returns hdr->size == 0
> spins the loop forever; a short or oversized size drives out-of-bounds
> reads in virtnet_fill_stats().
> 
> Impact: a malicious or compromised virtio-net backend hangs the CPU
> running the guest's device-statistics query in an infinite loop
> (hdr->size == 0), or drives an out-of-bounds read of the reply buffer.
> This matters most for a confidential guest, where the host is outside the
> trust boundary.

Why does it "matter most", or at all, there?
Host can always deny guest service. In fact, this is how cloud vendors
charge their clients, by denying service to whoever did not pay them.


> Validate each record before use: require a full header in the remaining
> bytes, a nonzero hdr->size that is at least the header size and matches the
> size expected for hdr->type, and that the record fits within res_size; stop
> the walk otherwise. Add virtnet_stats_reply_size() for the per-type size.

I'm all for making things easier to debug even when the device is buggy.
But I'm not inclined to add tons of hard to maintain code to
that end, and I would be worried broken hosts will come to
rely on drivers working around them.


> 
> Fixes: 941168f8b40e ("virtio_net: support device stats")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
> ---
>  drivers/net/virtio_net.c | 42 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 3e2a5876c6c8c..9cbe40d218cc4 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -3532,6 +3532,7 @@ static int virtnet_tx_resize(struct virtnet_info *vi, struct send_queue *sq,
>  	return err;
>  }
>  
> +
>  /*
>   * Send command via the control virtqueue and check status.  Commands
>   * supported by the hypervisor, as indicated by feature bits, should
> @@ -3546,6 +3547,7 @@ static bool virtnet_send_command_reply(struct virtnet_info *vi, u8 class, u8 cmd
>  	bool ok;
>  	int ret;
>  
> +
>  	/* Caller should know better */
>  	BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ));
>


we don't need this.
  
> @@ -4927,6 +4929,32 @@ static void virtnet_fill_stats(struct virtnet_info *vi, u32 qid,
>  	}
>  }
>  
> +static int virtnet_stats_reply_size(u8 type)
> +{
> +	switch (type) {
> +	case VIRTIO_NET_STATS_TYPE_REPLY_CVQ:
> +		return sizeof(struct virtio_net_stats_cvq);
> +	case VIRTIO_NET_STATS_TYPE_REPLY_RX_BASIC:
> +		return sizeof(struct virtio_net_stats_rx_basic);
> +	case VIRTIO_NET_STATS_TYPE_REPLY_RX_CSUM:
> +		return sizeof(struct virtio_net_stats_rx_csum);
> +	case VIRTIO_NET_STATS_TYPE_REPLY_RX_GSO:
> +		return sizeof(struct virtio_net_stats_rx_gso);
> +	case VIRTIO_NET_STATS_TYPE_REPLY_RX_SPEED:
> +		return sizeof(struct virtio_net_stats_rx_speed);
> +	case VIRTIO_NET_STATS_TYPE_REPLY_TX_BASIC:
> +		return sizeof(struct virtio_net_stats_tx_basic);
> +	case VIRTIO_NET_STATS_TYPE_REPLY_TX_CSUM:
> +		return sizeof(struct virtio_net_stats_tx_csum);
> +	case VIRTIO_NET_STATS_TYPE_REPLY_TX_GSO:
> +		return sizeof(struct virtio_net_stats_tx_gso);
> +	case VIRTIO_NET_STATS_TYPE_REPLY_TX_SPEED:
> +		return sizeof(struct virtio_net_stats_tx_speed);
> +	default:
> +		return sizeof(struct virtio_net_stats_reply_hdr);
> +	}
> +}
> +
>  static int __virtnet_get_hw_stats(struct virtnet_info *vi,
>  				  struct virtnet_stats_ctx *ctx,
>  				  struct virtio_net_ctrl_queue_stats *req,
> @@ -4936,7 +4964,7 @@ static int __virtnet_get_hw_stats(struct virtnet_info *vi,
>  	struct scatterlist sgs_in, sgs_out;
>  	void *p;
>  	u32 qid;
> -	int ok;
> +	int hdr_size, ok, remaining;
>  
>  	sg_init_one(&sgs_out, req, req_size);
>  	sg_init_one(&sgs_in, reply, res_size);
> @@ -4948,8 +4976,17 @@ static int __virtnet_get_hw_stats(struct virtnet_info *vi,
>  	if (!ok)
>  		return ok;
>  
> -	for (p = reply; p - reply < res_size; p += le16_to_cpu(hdr->size)) {
> +	for (p = reply; p - reply < res_size; p += hdr_size) {
> +		remaining = res_size - (p - reply);
> +		if (remaining < sizeof(*hdr))
> +			return -EINVAL;
> +
>  		hdr = p;
> +		hdr_size = le16_to_cpu(hdr->size);
> +		if (hdr_size < virtnet_stats_reply_size(hdr->type) ||
> +		    hdr_size > remaining)
> +			return -EINVAL;
> +
>  		qid = le16_to_cpu(hdr->vq_index);
>  		virtnet_fill_stats(vi, qid, ctx, p, false, hdr->type);
>  	}

That's a lot of fragile code for unclear benefit.


> @@ -7305,3 +7342,4 @@ module_exit(virtio_net_driver_exit);
>  MODULE_DEVICE_TABLE(virtio, id_table);
>  MODULE_DESCRIPTION("Virtio network driver");
>  MODULE_LICENSE("GPL");
> +
> -- 
> 2.53.0

Re: [PATCH] virtio_net: validate device stats reply records before use

[Michael Bommarito]

On Sat, Jul 11, 2026 at 11:20 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> Why does it "matter most", or at all, there?
> Host can always deny guest service. In fact, this is how cloud vendors
> charge their clients, by denying service to whoever did not pay them.
...
> I'm all for making things easier to debug even when the device is buggy.
> But I'm not inclined to add tons of hard to maintain code to
> that end, and I would be worried broken hosts will come to
> rely on drivers working around them.

I am always confused by the CoCo threat model to be honest, since it
seems like some people care a lot about maximalist reliance on the
contract and other people are more practical about how many other
vectors exist anyway.  No hard feelings if you want to NACK, but at
least it's documented publicly now for people to consider.

Thanks,
Mike

Re: [PATCH] virtio_net: validate device stats reply records before use

[Michael S. Tsirkin]

On Sat, Jul 11, 2026 at 11:29:56AM -0400, Michael Bommarito wrote:
> On Sat, Jul 11, 2026 at 11:20 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > Why does it "matter most", or at all, there?
> > Host can always deny guest service. In fact, this is how cloud vendors
> > charge their clients, by denying service to whoever did not pay them.
> ...
> > I'm all for making things easier to debug even when the device is buggy.
> > But I'm not inclined to add tons of hard to maintain code to
> > that end, and I would be worried broken hosts will come to
> > rely on drivers working around them.
> 
> I am always confused by the CoCo threat model to be honest,

Confidential computing? It's vague at points, given the term covers a
lot of different hardware. But one thing is clear - it's about
confidentiality.  DoS by host is empathically outside the threat model.
On any virtualization platform I know without exception,
host can just exit the VM, done, service denied.

> since it
> seems like some people care a lot about maximalist reliance on the
> contract and other people are more practical about how many other
> vectors exist anyway.

I don't really know what "vectors" or "the contract" are here.

Making a guest recover from a misbehaving device has as much a chance
to reduce security as increase it. So the only benefit is
robustness for users/developers, not security. And that
has to be weighted against the maintainance cost of the change.
This one is too costly, I judge.

>  No hard feelings if you want to NACK, but at
> least it's documented publicly now for people to consider.
> 
> Thanks,
> Mike