Re: [PATCH 2/3] vhost: fix descriptor chain bounds check in control queue

[David Marchand <david.marchand@redhat.com>]

On Thu, 8 Jan 2026 at 14:50, Maxime Coquelin <maxime.coquelin@redhat.com> wrote:
>
> The virtio_net_ctrl_pop() function traverses descriptor chains from
> guest-controlled memory without validating that the descriptor index
> stays within bounds and without a counter to prevent infinite loops
> from circular chains.
>
> A malicious guest could craft descriptors with a next field pointing
> out of bounds causing memory corruption, or create circular descriptor
> chains causing an infinite loop and denial of service.
>
> Add bounds checking and a loop counter to both descriptor chain
> traversal loops, similar to the existing protection in virtio_net.c
> fill_vec_buf_split().

Too bad we have two separate implementations..

>
> Fixes: 474f4d7840ad ("vhost: add control virtqueue")
> Cc: stable@dpdk.org
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  lib/vhost/virtio_net_ctrl.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/lib/vhost/virtio_net_ctrl.c b/lib/vhost/virtio_net_ctrl.c
> index 603a8db728..8149885384 100644
> --- a/lib/vhost/virtio_net_ctrl.c
> +++ b/lib/vhost/virtio_net_ctrl.c
> @@ -28,7 +28,7 @@ virtio_net_ctrl_pop(struct virtio_net *dev, struct vhost_virtqueue *cvq,
>                 struct virtio_net_ctrl_elem *ctrl_elem)
>         __rte_requires_shared_capability(&cvq->iotlb_lock)
>  {
> -       uint16_t avail_idx, desc_idx, n_descs = 0;
> +       uint16_t avail_idx, desc_idx, n_descs = 0, nr_descs, cnt = 0;
>         uint64_t desc_len, desc_addr, desc_iova, data_len = 0;
>         uint8_t *ctrl_req;
>         struct vring_desc *descs;
> @@ -59,12 +59,19 @@ virtio_net_ctrl_pop(struct virtio_net *dev, struct vhost_virtqueue *cvq,
>                         goto err;
>                 }
>
> +               nr_descs = desc_len / sizeof(struct vring_desc);
>                 desc_idx = 0;
>         } else {
>                 descs = cvq->desc;
> +               nr_descs = cvq->size;
>         }
>
>         while (1) {
> +               if (unlikely(desc_idx >= nr_descs || ++cnt > nr_descs)) {

I don't like pre increment.
Can we use post increment and stay aligned with known to work
fill_vec_buf_split?


> +                       VHOST_CONFIG_LOG(dev->ifname, ERR, "Invalid ctrl descriptor chain");
> +                       goto err;
> +               }
> +
>                 desc_len = descs[desc_idx].len;
>                 desc_iova = descs[desc_idx].addr;
>
> @@ -142,12 +149,23 @@ virtio_net_ctrl_pop(struct virtio_net *dev, struct vhost_virtqueue *cvq,
>                         goto free_err;
>                 }
>
> +               nr_descs = desc_len / sizeof(struct vring_desc);
>                 desc_idx = 0;
>         } else {
>                 descs = cvq->desc;
> +               nr_descs = cvq->size;
>         }
>
> -       while (!(descs[desc_idx].flags & VRING_DESC_F_WRITE)) {
> +       cnt = 0;
> +       while (1) {
> +               if (unlikely(desc_idx >= nr_descs || ++cnt > nr_descs)) {

Idem.


> +                       VHOST_CONFIG_LOG(dev->ifname, ERR, "Invalid ctrl descriptor chain");
> +                       goto free_err;
> +               }
> +
> +               if (descs[desc_idx].flags & VRING_DESC_F_WRITE)
> +                       break;
> +
>                 desc_len = descs[desc_idx].len;
>                 desc_iova = descs[desc_idx].addr;
>

I wonder if we are missing some call to vhost_alloc_copy_ind_table() as well.


-- 
David Marchand