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

[Maxime Coquelin]

On Thu, Jan 8, 2026 at 3:53 PM David Marchand <david.marchand at redhat.com> wrote:
>
> On Thu, 8 Jan 2026 at 14:50, Maxime Coquelin <maxime.coquelin at 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..

It may be possible to refactor into a single implementation,
but I was afraid of the potential performance impact on the datapath.

>
> >
> > Fixes: 474f4d7840ad ("vhost: add control virtqueue")
> > Cc: stable at dpdk.org
> >
> > Signed-off-by: Maxime Coquelin <maxime.coquelin at 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?

Indeed, I will change that for consistency.

>
>
> > +                       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.

We could do it, but I would not consider not doing it a security vulnerability.
No out of bounds memory accesses can be caused by a malicious guest AFAICT.
Non-contiguous buffer is detected by length checking.

>
>
> --
> David Marchand
>