virtio: fix off-by-one and invalid access in virtqueue_ordered_fill

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

Posted by Jonah Palmer 3 months, 3 weeks ago

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>
---
 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;
-- 
2.47.1

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

Posted by Michael Tokarev 3 months, 2 weeks ago

On 21.07.2025 18:02, 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")

This looks like a good candidate for qemu-stable, isn't it?

Thanks,

/mjt

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

Posted by Michael S. Tsirkin 3 months, 2 weeks ago

On Fri, Aug 01, 2025 at 06:46:09PM +0300, Michael Tokarev wrote:
> On 21.07.2025 18:02, 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")
> 
> This looks like a good candidate for qemu-stable, isn't it?
> 
> Thanks,
> 
> /mjt

indeed.

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

Posted by Jason Wang 3 months, 3 weeks ago

On Mon, Jul 21, 2025 at 11:02 PM Jonah Palmer <jonah.palmer@oracle.com> 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>
> ---

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks

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

Posted by Si-Wei Liu 3 months, 3 weeks ago

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;