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
