[PATCH net v2] virtio_net: reject stats replies for queues outside the request

[PATCH net v2] virtio_net: reject stats replies for queues outside the request

[Lianjie Wang]

__virtnet_get_hw_stats() passes the vq_index of each record in the
device's statistics reply to virtnet_fill_stats() without checking it,
so the device decides where the driver writes the counters.

For ethtool -S, virtnet_fill_stats() uses vq_index as an offset into the
output array, which the ethtool core sized for curr_queue_pairs. A
record with the vq_index of a higher queue can make the driver write
past the end of the array.

For the per-queue netlink stats, the queue type also comes from
vq_index. A receive record in reply to a request for a transmit queue
makes the driver fill the struct netdev_queue_stats_tx on the stack as
if it were a struct netdev_queue_stats_rx. If the device offers
VIRTIO_NET_STATS_TYPE_RX_SPEED, this writes one counter to the eight
bytes after the tx struct.

Fix this by rejecting the reply when the vq_index of a record lies
outside the range of queues the request asked about. The control vq is
checked apart from that range, since the vqs of the queue pairs beyond
curr_queue_pairs lie between that range and the control vq.

Claude Opus 5.5 was used to audit the driver and draft this patch. To
reproduce, the virtio-net device in QEMU, which has one queue pair, was
modified to answer an ethtool -S request with a record for a nonexistent
receive queue. On an unmodified kernel, KASAN reports a write past the
ethtool -S output array:

  BUG: KASAN: vmalloc-out-of-bounds in virtnet_fill_stats+0xab1/0xec0
  Write of size 8 at addr ffffc90000066140 by task ethtool_stats/159
  Call Trace:
   virtnet_fill_stats+0xab1/0xec0
   virtnet_get_hw_stats+0x5d6/0x7c0
   virtnet_get_ethtool_stats+0x11b/0x920
   ethtool_get_stats+0x150/0x1f0
   dev_ethtool+0xaaa/0x1be0
  The buggy address belongs to a 1-page vmalloc region starting at 0xffffc90000066000 allocated at ethtool_get_stats+0x120/0x1f0

With this patch, the driver rejects the same reply with "Failed to get
hw stats.". When ethtool -L leaves queue pairs unused, the driver also
rejects a record for the first unused receive queue, while a well-formed
reply fills the statistics as before, those of the control vq included.
The drivers/net/stats.py selftest passes on a device that reports
receive and transmit statistics.

Fixes: 941168f8b40e ("virtio_net: support device stats")
Fixes: d888f04c09bb ("virtio-net: support queue stat")
Cc: stable@vger.kernel.org
Signed-off-by: Lianjie Wang <lian@is.s.u-tokyo.ac.jp>
---
v2:
- Add a comment and a sentence on why the control vq is checked apart
  from the range of the other queues.
- Add the KASAN report and a test with unused queue pairs.
v1: https://lore.kernel.org/all/20261022182710.2055771-1-karin0.zst@gmail.com/

 drivers/net/virtio_net.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index dd2c41ef6fbe7..db56f984bb86a 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -4973,7 +4973,8 @@ static void virtnet_fill_stats(struct virtnet_info *vi, u32 qid,
 static int __virtnet_get_hw_stats(struct virtnet_info *vi,
 				  struct virtnet_stats_ctx *ctx,
 				  struct virtio_net_ctrl_queue_stats *req,
-				  int req_size, void *reply, int res_size)
+				  int req_size, void *reply, int res_size,
+				  int first_vq, int last_vq, bool enable_cvq)
 {
 	struct virtio_net_stats_reply_hdr *hdr;
 	struct scatterlist sgs_in, sgs_out;
@@ -4994,6 +4995,13 @@ static int __virtnet_get_hw_stats(struct virtnet_info *vi,
 	for (p = reply; p - reply < res_size; p += le16_to_cpu(hdr->size)) {
 		hdr = p;
 		qid = le16_to_cpu(hdr->vq_index);
+		/* The vqs of the queue pairs beyond curr_queue_pairs lie
+		 * between last_vq and the control vq.
+		 */
+		if ((qid < first_vq || qid > last_vq) &&
+		    !(enable_cvq && qid == vi->max_queue_pairs * 2))
+			return -EINVAL;
+
 		virtnet_fill_stats(vi, qid, ctx, p, false, hdr->type);
 	}
 
@@ -5073,7 +5081,8 @@ static int virtnet_get_hw_stats(struct virtnet_info *vi,
 	if (enable_cvq)
 		virtnet_make_stat_req(vi, ctx, req, vi->max_queue_pairs * 2, &j);
 
-	ok = __virtnet_get_hw_stats(vi, ctx, req, sizeof(*req) * j, reply, res_size);
+	ok = __virtnet_get_hw_stats(vi, ctx, req, sizeof(*req) * j, reply, res_size,
+				    first_vq, last_vq, enable_cvq);
 
 	kfree(req);
 	kfree(reply);

base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
-- 
2.56.0

Re: [PATCH net v2] virtio_net: reject stats replies for queues outside the request

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
virtio_net: reject stats replies for queues outside the request

With this patch, __virtnet_get_hw_stats() rejects a device statistics
reply if any record has a vq_index outside the range of queues in the
request. The control vq is the one exception. This stops the device from
steering writes in virtnet_fill_stats() past the end of the ethtool -S
output array or the on-stack struct netdev_queue_stats_tx.

> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index dd2c41ef6fbe7..db56f984bb86a 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c

[ ... ]

> @@ -4994,6 +4995,13 @@ static int __virtnet_get_hw_stats(struct virtnet_info *vi,
>  	for (p = reply; p - reply < res_size; p += le16_to_cpu(hdr->size)) {
>  		hdr = p;
>  		qid = le16_to_cpu(hdr->vq_index);

[Severity: High]
This isn't a bug introduced by this patch, but the new vq_index check only
limits where the driver writes. Can the device still steer the parse
through hdr->size?

The loop in __virtnet_get_hw_stats() advances by hdr->size, which comes
from the device. It only checks that each record starts below res_size.
It does not check that hdr->size is non-zero, or that it covers the header
plus the payload for hdr->type, or that the record ends inside the reply
buffer.

If hdr->size is 0 and vq_index is in range, p never advances. Will this
loop then spin forever? The loop has no cond_resched(), and on the
ethtool -S path it runs with rtnl held:

  virtnet_get_ethtool_stats()
    virtnet_get_hw_stats(vi, &ctx, -1)
      __virtnet_get_hw_stats()

If a short hdr->size puts a record near the end of the buffer, the found:
loop in virtnet_fill_stats() still reads the whole payload for the reply
type:

	v = (const __le64 *)(base + desc[i].offset);
	ctx->data[offset + i] = le64_to_cpu(*v);

Can that read past the end of the kmalloc'd reply and copy slab bytes into
the ethtool -S output? virtnet_fill_stats_qstat() does the same read for
the per-queue netlink stats.

The parser also has no way to know how much of the reply the device wrote.
virtnet_send_command_reply() throws away the used length:

	while (!virtqueue_get_buf(vi->cvq, &tmp) &&
	       !virtqueue_is_broken(vi->cvq)) {

and virtnet_get_hw_stats() allocates the reply with plain
kmalloc(res_size, GFP_KERNEL). If the reply is short, the leftover heap
bytes get parsed as records. Would they be reported as counters whenever a
stale vq_index happens to fall in [first_vq, last_vq]?

> +		/* The vqs of the queue pairs beyond curr_queue_pairs lie
> +		 * between last_vq and the control vq.
> +		 */
> +		if ((qid < first_vq || qid > last_vq) &&
> +		    !(enable_cvq && qid == vi->max_queue_pairs * 2))
> +			return -EINVAL;

[Severity: Medium]
This is a pre-existing issue, but after this change a malformed reply
returns -EINVAL while a failed command still returns 0. Just above the
loop in __virtnet_get_hw_stats():

	if (!ok)
		return ok;

virtnet_send_command_reply() returns a bool. So when the device answers
with a status other than VIRTIO_NET_OK, or the kick fails, or the cvq is
broken, __virtnet_get_hw_stats() returns 0.

virtnet_get_ethtool_stats() only warns when the return value is non-zero:

	if (virtnet_get_hw_stats(vi, &ctx, -1))
		dev_warn(&vi->dev->dev, "Failed to get hw stats.\n");

As it stands, a rejected command is not reported, and ethtool -S shows zero
for every hardware counter. Should this path also return an errno?

> +
>  		virtnet_fill_stats(vi, qid, ctx, p, false, hdr->type);
>  	}

[ ... ]
-- 
pw-bot: cr