Re: [PATCH] virtio: fix off-by-one and invalid access in virtqueue_ordered_fill

[Si-Wei Liu <si-wei.liu@oracle.com>]

Looks good.

On 7/21/2025 8:02 AM, Jonah Palmer wrote:
> Commit b44135daa372 introduced virtqueue_ordered_fill for
> VIRTIO_F_IN_ORDER support but had a few issues:
>
> * Conditional while loop used 'steps <= max_steps' but should've been
>    'steps < max_steps' since reaching steps == max_steps would indicate
>    that we didn't find an element, which is an error. Without this
>    change, the code would attempt to read invalid data at an index
>    outside of our search range.
>
> * Incremented 'steps' using the next chain's ndescs instead of the
>    current one.
>
> This patch corrects the loop bounds and synchronizes 'steps' and index
> increments.
>
> We also add a defensive sanity check against malicious or invalid
> descriptor counts to avoid a potential infinite loop and DoS.
>
> Fixes: b44135daa372 ("virtio: virtqueue_ordered_fill - VIRTIO_F_IN_ORDER support")
> Reported-by: terrynini <terrynini38514@gmail.com>
> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
Reviewed-by: Si-Wei Liu <si-wei.liu@oracle.com>
> ---
>   hw/virtio/virtio.c | 22 ++++++++++++++++------
>   1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 82a285a31d..cc1c072cf7 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -938,18 +938,18 @@ static void virtqueue_packed_fill(VirtQueue *vq, const VirtQueueElement *elem,
>   static void virtqueue_ordered_fill(VirtQueue *vq, const VirtQueueElement *elem,
>                                      unsigned int len)
>   {
> -    unsigned int i, steps, max_steps;
> +    unsigned int i, steps, max_steps, ndescs;
>   
>       i = vq->used_idx % vq->vring.num;
>       steps = 0;
>       /*
> -     * We shouldn't need to increase 'i' by more than the distance
> -     * between used_idx and last_avail_idx.
> +     * We shouldn't need to increase 'i' by more than or equal to
> +     * the distance between used_idx and last_avail_idx (max_steps).
>        */
>       max_steps = (vq->last_avail_idx - vq->used_idx) % vq->vring.num;
>   
>       /* Search for element in vq->used_elems */
> -    while (steps <= max_steps) {
> +    while (steps < max_steps) {
>           /* Found element, set length and mark as filled */
>           if (vq->used_elems[i].index == elem->index) {
>               vq->used_elems[i].len = len;
> @@ -957,8 +957,18 @@ static void virtqueue_ordered_fill(VirtQueue *vq, const VirtQueueElement *elem,
>               break;
>           }
>   
> -        i += vq->used_elems[i].ndescs;
> -        steps += vq->used_elems[i].ndescs;
> +        ndescs = vq->used_elems[i].ndescs;
> +
> +        /* Defensive sanity check */
> +        if (unlikely(ndescs == 0 || ndescs > vq->vring.num)) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "%s: %s invalid ndescs %u at position %u\n",
> +                          __func__, vq->vdev->name, ndescs, i);
> +            return;
> +        }
> +
> +        i += ndescs;
> +        steps += ndescs;
>   
>           if (i >= vq->vring.num) {
>               i -= vq->vring.num;