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

[Jakub Kicinski <kuba@kernel.org>]

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