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

["Michael S. Tsirkin" <mst@redhat.com>]

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