diff mbox series

[v2] hw/virtio: Fix check available index on virtio loading

Message ID 20241211123349.904-1-Wafer@jaguarmicro.com (mailing list archive)
State New
Headers show
Series [v2] hw/virtio: Fix check available index on virtio loading | expand

Commit Message

Wafer Dec. 11, 2024, 12:33 p.m. UTC 
From: Wafer Xie <wafer@jaguarmicro.com>

The virtio-1.2 specification writes:

2.7.6 The Virtqueue Available Ring:
"idx field indicates where the driver would put the next descriptor entry
in the ring (modulo the queue size). This starts at 0, and increases"

The idx will increase from 0 to 0xFFFF and repeat,
So idx may be less than last_avail_idx.

Fixes: 258dc7c96b ("virtio: sanity-check available index")

Signed-off-by: Wafer Xie <wafer@jaguarmicro.com>

--
Changes in v2:
 -Modify the commit id of the fix.
---
 hw/virtio/virtio.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Eugenio Perez Martin Dec. 11, 2024, 12:45 p.m. UTC | #1 
On Wed, Dec 11, 2024 at 1:34 PM Wafer <Wafer@jaguarmicro.com> wrote:
>
> From: Wafer Xie <wafer@jaguarmicro.com>
>
> The virtio-1.2 specification writes:
>
> 2.7.6 The Virtqueue Available Ring:
> "idx field indicates where the driver would put the next descriptor entry
> in the ring (modulo the queue size). This starts at 0, and increases"
>
> The idx will increase from 0 to 0xFFFF and repeat,
> So idx may be less than last_avail_idx.
>

I don't get this change. If that happens the driver went buggy or
malicious and the next check nheads > vring.num should mark the vq as
buggy, isn't it?

> Fixes: 258dc7c96b ("virtio: sanity-check available index")
>
> Signed-off-by: Wafer Xie <wafer@jaguarmicro.com>
>
> --
> Changes in v2:
>  -Modify the commit id of the fix.
> ---
>  hw/virtio/virtio.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index a26f18908e..ae7d407113 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -3362,7 +3362,13 @@ virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
>                  continue;
>              }
>
> -            nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
> +            if (vring_avail_idx(&vdev->vq[i]) >= vdev->vq[i].last_avail_idx) {
> +                nheads = vring_avail_idx(&vdev->vq[i]) -
> +                         vdev->vq[i].last_avail_idx;
> +            } else {
> +                nheads = UINT16_MAX - vdev->vq[i].last_avail_idx +
> +                         vring_avail_idx(&vdev->vq[i]) + 1;
> +            }
>              /* Check it isn't doing strange things with descriptor numbers. */
>              if (nheads > vdev->vq[i].vring.num) {
>                  virtio_error(vdev, "VQ %d size 0x%x Guest index 0x%x "
> --
> 2.27.0
>
Wafer Dec. 12, 2024, 2:30 a.m. UTC | #2 
> -----Original Message-----
> From: Eugenio Perez Martin <eperezma@redhat.com>
> Sent: 2024年12月11日 20:45
> To: Wafer <wafer@jaguarmicro.com>
> Cc: mst@redhat.com; jasowang@redhat.com; qemu-devel@nongnu.org;
> Angus Chen <angus.chen@jaguarmicro.com>
> Subject: Re: [PATCH v2] hw/virtio: Fix check available index on virtio loading
> 
> External Mail: This email originated from OUTSIDE of the organization!
> Do not click links, open attachments or provide ANY information unless you
> recognize the sender and know the content is safe.
> 
> 
> On Wed, Dec 11, 2024 at 1:34 PM Wafer <Wafer@jaguarmicro.com> wrote:
> >
> > From: Wafer Xie <wafer@jaguarmicro.com>
> >
> > The virtio-1.2 specification writes:
> >
> > 2.7.6 The Virtqueue Available Ring:
> > "idx field indicates where the driver would put the next descriptor
> > entry in the ring (modulo the queue size). This starts at 0, and increases"
> >
> > The idx will increase from 0 to 0xFFFF and repeat, So idx may be less
> > than last_avail_idx.
> >
> 
> I don't get this change. If that happens the driver went buggy or malicious
> and the next check nheads > vring.num should mark the vq as buggy, isn't it?
> 

During the migration process, let's assume a scenario where:
The depth of the avail ring is 0x10000, last_avail_index is 0xFFF0, and avail->idx is 0xFFFFF.
At this point, the guest VM driver sends a virtio data packet, and avail->idx is updated to 0x0.
The migration occurs, and last_avail_index is sent to the target QEMU. 
During the loading process of the target QEMU, it will check both last_avail_index and avail->idx. 
In this case, last_avail_index is greater than avail->idx.

> > Fixes: 258dc7c96b ("virtio: sanity-check available index")
> >
> > Signed-off-by: Wafer Xie <wafer@jaguarmicro.com>
> >
> > --
> > Changes in v2:
> >  -Modify the commit id of the fix.
> > ---
> >  hw/virtio/virtio.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index
> > a26f18908e..ae7d407113 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -3362,7 +3362,13 @@ virtio_load(VirtIODevice *vdev, QEMUFile *f, int
> version_id)
> >                  continue;
> >              }
> >
> > -            nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
> > +            if (vring_avail_idx(&vdev->vq[i]) >= vdev->vq[i].last_avail_idx) {
> > +                nheads = vring_avail_idx(&vdev->vq[i]) -
> > +                         vdev->vq[i].last_avail_idx;
> > +            } else {
> > +                nheads = UINT16_MAX - vdev->vq[i].last_avail_idx +
> > +                         vring_avail_idx(&vdev->vq[i]) + 1;
> > +            }
> >              /* Check it isn't doing strange things with descriptor numbers. */
> >              if (nheads > vdev->vq[i].vring.num) {
> >                  virtio_error(vdev, "VQ %d size 0x%x Guest index 0x%x "
> > --
> > 2.27.0
> >
Eugenio Perez Martin Dec. 12, 2024, 7:33 a.m. UTC | #3 
On Thu, Dec 12, 2024 at 3:30 AM Wafer <wafer@jaguarmicro.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Eugenio Perez Martin <eperezma@redhat.com>
> > Sent: 2024年12月11日 20:45
> > To: Wafer <wafer@jaguarmicro.com>
> > Cc: mst@redhat.com; jasowang@redhat.com; qemu-devel@nongnu.org;
> > Angus Chen <angus.chen@jaguarmicro.com>
> > Subject: Re: [PATCH v2] hw/virtio: Fix check available index on virtio loading
> >
> > External Mail: This email originated from OUTSIDE of the organization!
> > Do not click links, open attachments or provide ANY information unless you
> > recognize the sender and know the content is safe.
> >
> >
> > On Wed, Dec 11, 2024 at 1:34 PM Wafer <Wafer@jaguarmicro.com> wrote:
> > >
> > > From: Wafer Xie <wafer@jaguarmicro.com>
> > >
> > > The virtio-1.2 specification writes:
> > >
> > > 2.7.6 The Virtqueue Available Ring:
> > > "idx field indicates where the driver would put the next descriptor
> > > entry in the ring (modulo the queue size). This starts at 0, and increases"
> > >
> > > The idx will increase from 0 to 0xFFFF and repeat, So idx may be less
> > > than last_avail_idx.
> > >
> >
> > I don't get this change. If that happens the driver went buggy or malicious
> > and the next check nheads > vring.num should mark the vq as buggy, isn't it?
> >
>
> During the migration process, let's assume a scenario where:
> The depth of the avail ring is 0x10000, last_avail_index is 0xFFF0, and avail->idx is 0xFFFFF.
> At this point, the guest VM driver sends a virtio data packet, and avail->idx is updated to 0x0.
> The migration occurs, and last_avail_index is sent to the target QEMU.
> During the loading process of the target QEMU, it will check both last_avail_index and avail->idx.
> In this case, last_avail_index is greater than avail->idx.
>

But (uint16_t)0x0 - (uint16_t)0xFFF0 is well defined to 0x10. So
nheads value is correct, isn't it?

> > > Fixes: 258dc7c96b ("virtio: sanity-check available index")
> > >
> > > Signed-off-by: Wafer Xie <wafer@jaguarmicro.com>
> > >
> > > --
> > > Changes in v2:
> > >  -Modify the commit id of the fix.
> > > ---
> > >  hw/virtio/virtio.c | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index
> > > a26f18908e..ae7d407113 100644
> > > --- a/hw/virtio/virtio.c
> > > +++ b/hw/virtio/virtio.c
> > > @@ -3362,7 +3362,13 @@ virtio_load(VirtIODevice *vdev, QEMUFile *f, int
> > version_id)
> > >                  continue;
> > >              }
> > >
> > > -            nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
> > > +            if (vring_avail_idx(&vdev->vq[i]) >= vdev->vq[i].last_avail_idx) {
> > > +                nheads = vring_avail_idx(&vdev->vq[i]) -
> > > +                         vdev->vq[i].last_avail_idx;
> > > +            } else {
> > > +                nheads = UINT16_MAX - vdev->vq[i].last_avail_idx +
> > > +                         vring_avail_idx(&vdev->vq[i]) + 1;
> > > +            }
> > >              /* Check it isn't doing strange things with descriptor numbers. */
> > >              if (nheads > vdev->vq[i].vring.num) {
> > >                  virtio_error(vdev, "VQ %d size 0x%x Guest index 0x%x "
> > > --
> > > 2.27.0
> > >
>
Wafer Dec. 12, 2024, 1:09 p.m. UTC | #4 
> -----Original Message-----
> From: Eugenio Perez Martin <eperezma@redhat.com>
> Sent: 2024年12月12日 15:33
> To: Wafer <wafer@jaguarmicro.com>
> Cc: mst@redhat.com; jasowang@redhat.com; qemu-devel@nongnu.org;
> Angus Chen <angus.chen@jaguarmicro.com>
> Subject: Re: [PATCH v2] hw/virtio: Fix check available index on virtio loading
> 
> External Mail: This email originated from OUTSIDE of the organization!
> Do not click links, open attachments or provide ANY information unless you
> recognize the sender and know the content is safe.
> 
> 
> On Thu, Dec 12, 2024 at 3:30 AM Wafer <wafer@jaguarmicro.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Eugenio Perez Martin <eperezma@redhat.com>
> > > Sent: 2024年12月11日 20:45
> > > To: Wafer <wafer@jaguarmicro.com>
> > > Cc: mst@redhat.com; jasowang@redhat.com; qemu-devel@nongnu.org;
> > > Angus Chen <angus.chen@jaguarmicro.com>
> > > Subject: Re: [PATCH v2] hw/virtio: Fix check available index on
> > > virtio loading
> > >
> > > External Mail: This email originated from OUTSIDE of the organization!
> > > Do not click links, open attachments or provide ANY information
> > > unless you recognize the sender and know the content is safe.
> > >
> > >
> > > On Wed, Dec 11, 2024 at 1:34 PM Wafer <Wafer@jaguarmicro.com>
> wrote:
> > > >
> > > > From: Wafer Xie <wafer@jaguarmicro.com>
> > > >
> > > > The virtio-1.2 specification writes:
> > > >
> > > > 2.7.6 The Virtqueue Available Ring:
> > > > "idx field indicates where the driver would put the next
> > > > descriptor entry in the ring (modulo the queue size). This starts at 0,
> and increases"
> > > >
> > > > The idx will increase from 0 to 0xFFFF and repeat, So idx may be
> > > > less than last_avail_idx.
> > > >
> > >
> > > I don't get this change. If that happens the driver went buggy or
> > > malicious and the next check nheads > vring.num should mark the vq as
> buggy, isn't it?
> > >
> >
> > During the migration process, let's assume a scenario where:
> > The depth of the avail ring is 0x10000, last_avail_index is 0xFFF0, and avail-
> >idx is 0xFFFFF.
> > At this point, the guest VM driver sends a virtio data packet, and avail->idx
> is updated to 0x0.
> > The migration occurs, and last_avail_index is sent to the target QEMU.
> > During the loading process of the target QEMU, it will check both
> last_avail_index and avail->idx.
> > In this case, last_avail_index is greater than avail->idx.
> >
> 
> But (uint16_t)0x0 - (uint16_t)0xFFF0 is well defined to 0x10. So nheads value
> is correct, isn't it?
> 

Thanks, u are right.

> > > > Fixes: 258dc7c96b ("virtio: sanity-check available index")
> > > >
> > > > Signed-off-by: Wafer Xie <wafer@jaguarmicro.com>
> > > >
> > > > --
> > > > Changes in v2:
> > > >  -Modify the commit id of the fix.
> > > > ---
> > > >  hw/virtio/virtio.c | 8 +++++++-
> > > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index
> > > > a26f18908e..ae7d407113 100644
> > > > --- a/hw/virtio/virtio.c
> > > > +++ b/hw/virtio/virtio.c
> > > > @@ -3362,7 +3362,13 @@ virtio_load(VirtIODevice *vdev, QEMUFile
> > > > *f, int
> > > version_id)
> > > >                  continue;
> > > >              }
> > > >
> > > > -            nheads = vring_avail_idx(&vdev->vq[i]) - vdev-
> >vq[i].last_avail_idx;
> > > > +            if (vring_avail_idx(&vdev->vq[i]) >= vdev->vq[i].last_avail_idx) {
> > > > +                nheads = vring_avail_idx(&vdev->vq[i]) -
> > > > +                         vdev->vq[i].last_avail_idx;
> > > > +            } else {
> > > > +                nheads = UINT16_MAX - vdev->vq[i].last_avail_idx +
> > > > +                         vring_avail_idx(&vdev->vq[i]) + 1;
> > > > +            }
> > > >              /* Check it isn't doing strange things with descriptor numbers. */
> > > >              if (nheads > vdev->vq[i].vring.num) {
> > > >                  virtio_error(vdev, "VQ %d size 0x%x Guest index 0x%x "
> > > > --
> > > > 2.27.0
> > > >
> >
diff mbox series

Patch

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index a26f18908e..ae7d407113 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -3362,7 +3362,13 @@  virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
                 continue;
             }
 
-            nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx;
+            if (vring_avail_idx(&vdev->vq[i]) >= vdev->vq[i].last_avail_idx) {
+                nheads = vring_avail_idx(&vdev->vq[i]) -
+                         vdev->vq[i].last_avail_idx;
+            } else {
+                nheads = UINT16_MAX - vdev->vq[i].last_avail_idx +
+                         vring_avail_idx(&vdev->vq[i]) + 1;
+            }
             /* Check it isn't doing strange things with descriptor numbers. */
             if (nheads > vdev->vq[i].vring.num) {
                 virtio_error(vdev, "VQ %d size 0x%x Guest index 0x%x "