[Qemu-devel] [PATCH v3 0/2] check VirtiQueue Vring objects

[Qemu-devel] [PATCH v3 0/2] check VirtiQueue Vring objects

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

Hello,

An user could attempt to use an uninitialised VirtQueue object
or set Vring object with undue values, raising an unexpected
exception in Qemu. This patch set fixes this issue and also adds
a unit test to the suite.

Thank you.
--
Prasad J Pandit (2):
  virtio: check VirtQueue Vring object is set
  tests: add test to check VirtQueue object

Update: Fixed style error, add space before parenthesis '('
  -> https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg04652.html

 hw/virtio/virtio.c      | 14 +++++++++++---
 tests/virtio-blk-test.c | 25 +++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 3 deletions(-)

-- 
2.13.6

[Qemu-devel] [PATCH v3 1/2] virtio: check VirtQueue Vring object is set

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

An user could attempt to use an uninitialised VirtQueue object
or unset Vring.align leading to a arithmetic exception. Add check
to avoid it.

Reported-by: Zhangboxian <zhangboxian@huawei.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/virtio/virtio.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 5884ce3480..c01eac87a5 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -182,7 +182,7 @@ void virtio_queue_update_rings(VirtIODevice *vdev, int n)
 {
     VRing *vring = &vdev->vq[n].vring;
 
-    if (!vring->desc) {
+    if (!vdev->vq[n].vring.num || !vring->desc || !vring->align) {
         /* not yet setup -> nothing to do */
         return;
     }
@@ -1414,6 +1414,9 @@ void virtio_config_modern_writel(VirtIODevice *vdev,
 
 void virtio_queue_set_addr(VirtIODevice *vdev, int n, hwaddr addr)
 {
+    if (!vdev->vq[n].vring.num) {
+        return;
+    }
     vdev->vq[n].vring.desc = addr;
     virtio_queue_update_rings(vdev, n);
 }
@@ -1426,6 +1429,9 @@ hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n)
 void virtio_queue_set_rings(VirtIODevice *vdev, int n, hwaddr desc,
                             hwaddr avail, hwaddr used)
 {
+    if (!vdev->vq[n].vring.num || !desc || !vdev->vq[n].vring.align) {
+        return;
+    }
     vdev->vq[n].vring.desc = desc;
     vdev->vq[n].vring.avail = avail;
     vdev->vq[n].vring.used = used;
@@ -1494,8 +1500,10 @@ void virtio_queue_set_align(VirtIODevice *vdev, int n, int align)
      */
     assert(k->has_variable_vring_alignment);
 
-    vdev->vq[n].vring.align = align;
-    virtio_queue_update_rings(vdev, n);
+    if (align) {
+        vdev->vq[n].vring.align = align;
+        virtio_queue_update_rings(vdev, n);
+    }
 }
 
 static bool virtio_queue_notify_aio_vq(VirtQueue *vq)
-- 
2.13.6

[Qemu-devel] [PATCH v3 2/2] tests: add test to check VirtQueue object

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

An uninitialised VirtQueue object or one with Vring.align field
set to zero(0) could lead to arithmetic exceptions. Add a unit
test to validate it.

Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 tests/virtio-blk-test.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index e6fb9bac87..45f368dcd9 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -674,6 +674,30 @@ static void pci_hotplug(void)
     qtest_shutdown(qs);
 }
 
+/*
+ * Check that setting the vring addr on a non-existent virtqueue does
+ * not crash.
+ */
+static void test_nonexistent_virtqueue(void)
+{
+    QPCIBar bar0;
+    QOSState *qs;
+    QPCIDevice *dev;
+
+    qs = pci_test_start();
+    dev = qpci_device_find(qs->pcibus, QPCI_DEVFN(4, 0));
+    g_assert(dev != NULL);
+
+    qpci_device_enable(dev);
+    bar0 = qpci_iomap(dev, 0, NULL);
+
+    qpci_io_writeb(dev, bar0, VIRTIO_PCI_QUEUE_SEL, 2);
+    qpci_io_writel(dev, bar0, VIRTIO_PCI_QUEUE_PFN, 1);
+
+    g_free(dev);
+    qtest_shutdown(qs);
+}
+
 static void mmio_basic(void)
 {
     QVirtioMMIODevice *dev;
@@ -724,6 +748,7 @@ int main(int argc, char **argv)
         qtest_add_func("/virtio/blk/pci/basic", pci_basic);
         qtest_add_func("/virtio/blk/pci/indirect", pci_indirect);
         qtest_add_func("/virtio/blk/pci/config", pci_config);
+        qtest_add_func("/virtio/blk/pci/nxvirtq", test_nonexistent_virtqueue);
         if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
             qtest_add_func("/virtio/blk/pci/msix", pci_msix);
             qtest_add_func("/virtio/blk/pci/idx", pci_idx);
-- 
2.13.6

Re: [Qemu-devel] [PATCH v3 1/2] virtio: check VirtQueue Vring object is set

[Cornelia Huck]

On Sat, 25 Nov 2017 00:04:45 +0530
P J P <ppandit@redhat.com> wrote:

> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> An user could attempt to use an uninitialised VirtQueue object

s/An user/A guest/ ?

> or unset Vring.align leading to a arithmetic exception. Add check
> to avoid it.
> 
> Reported-by: Zhangboxian <zhangboxian@huawei.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/virtio/virtio.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 5884ce3480..c01eac87a5 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -182,7 +182,7 @@ void virtio_queue_update_rings(VirtIODevice *vdev, int n)
>  {
>      VRing *vring = &vdev->vq[n].vring;
>  
> -    if (!vring->desc) {
> +    if (!vdev->vq[n].vring.num || !vring->desc || !vring->align) {
>          /* not yet setup -> nothing to do */
>          return;
>      }
> @@ -1414,6 +1414,9 @@ void virtio_config_modern_writel(VirtIODevice *vdev,
>  
>  void virtio_queue_set_addr(VirtIODevice *vdev, int n, hwaddr addr)
>  {
> +    if (!vdev->vq[n].vring.num) {
> +        return;
> +    }
>      vdev->vq[n].vring.desc = addr;
>      virtio_queue_update_rings(vdev, n);
>  }
> @@ -1426,6 +1429,9 @@ hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n)
>  void virtio_queue_set_rings(VirtIODevice *vdev, int n, hwaddr desc,
>                              hwaddr avail, hwaddr used)
>  {
> +    if (!vdev->vq[n].vring.num || !desc || !vdev->vq[n].vring.align) {


Checking for !desc is wrong (why shouldn't a driver be able to unset a
descriptor table?)

The check for align is not really needed, as virtio-1 disallows setting
align anyway.

> +        return;
> +    }
>      vdev->vq[n].vring.desc = desc;
>      vdev->vq[n].vring.avail = avail;
>      vdev->vq[n].vring.used = used;
> @@ -1494,8 +1500,10 @@ void virtio_queue_set_align(VirtIODevice *vdev, int n, int align)
>       */
>      assert(k->has_variable_vring_alignment);
>  
> -    vdev->vq[n].vring.align = align;
> -    virtio_queue_update_rings(vdev, n);
> +    if (align) {
> +        vdev->vq[n].vring.align = align;
> +        virtio_queue_update_rings(vdev, n);
> +    }
>  }
>  
>  static bool virtio_queue_notify_aio_vq(VirtQueue *vq)

Re: [Qemu-devel] [PATCH v3 1/2] virtio: check VirtQueue Vring object is set

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 355 bytes --]

On Sat, Nov 25, 2017 at 12:04:45AM +0530, P J P wrote:
> @@ -1426,6 +1429,9 @@ hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n)
>  void virtio_queue_set_rings(VirtIODevice *vdev, int n, hwaddr desc,
>                              hwaddr avail, hwaddr used)
>  {
> +    if (!vdev->vq[n].vring.num || !desc || !vdev->vq[n].vring.align) {

Why !desc?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v3 1/2] virtio: check VirtQueue Vring object is set

[P J P]

+-- On Mon, 27 Nov 2017, Cornelia Huck wrote --+
|The check for align is not really needed, as virtio-1 disallows setting align 
|anyway.

 disallows...?

| Checking for !desc is wrong (why shouldn't a driver be able to unset a
| descriptor table?)

+-- On Mon, 27 Nov 2017, Stefan Hajnoczi wrote --+
| > +    if (!vdev->vq[n].vring.num || !desc || !vdev->vq[n].vring.align) {
| ...
|   vdev->vq[n].vring.desc = desc;
|
| Why !desc?

virtio_queue_set_rings
 virtio_init_region_cache
   VirtQueue *vq = &vdev->vq[n];
   ...
   addr = vq->vring.desc;
   if (!addr) {
       return;
   }

These checks seem to be repeating all over. As mentioned earlier, could these 
be collated in one place, maybe virtio_queue_get_num()?

  int virtio_queue_get_num(VirtIODevice *vdev, int n)
  {
      VirtQueue *vq = &vdev->vq[n];

      if (!vq->.vring.num
           || !vq->vring.desc
           || !vq->vring.align) {
          return 0;  /* vq not set */
      }

      return vdev->vq[n].vring.num;
  }


Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH v3 1/2] virtio: check VirtQueue Vring object is set

[Cornelia Huck]

On Mon, 27 Nov 2017 23:25:28 +0530 (IST)
P J P <ppandit@redhat.com> wrote:

> +-- On Mon, 27 Nov 2017, Cornelia Huck wrote --+
> |The check for align is not really needed, as virtio-1 disallows setting align 
> |anyway.
> 
>  disallows...?

See the check in virtio_queue_set_align(). Moreover, the calculation
that breaks virtqueue setup for align == 0 is only called for the
legacy setup, IOW not for this virtio-1 only function.

> 
> | Checking for !desc is wrong (why shouldn't a driver be able to unset a
> | descriptor table?)
> 
> +-- On Mon, 27 Nov 2017, Stefan Hajnoczi wrote --+
> | > +    if (!vdev->vq[n].vring.num || !desc || !vdev->vq[n].vring.align) {
> | ...
> |   vdev->vq[n].vring.desc = desc;
> |
> | Why !desc?
> 
> virtio_queue_set_rings
>  virtio_init_region_cache
>    VirtQueue *vq = &vdev->vq[n];
>    ...
>    addr = vq->vring.desc;
>    if (!addr) {
>        return;
>    }
> 
> These checks seem to be repeating all over. As mentioned earlier, could these 
> be collated in one place, maybe virtio_queue_get_num()?
> 
>   int virtio_queue_get_num(VirtIODevice *vdev, int n)
>   {
>       VirtQueue *vq = &vdev->vq[n];
> 
>       if (!vq->.vring.num
>            || !vq->vring.desc
>            || !vq->vring.align) {
>           return 0;  /* vq not set */
>       }
> 
>       return vdev->vq[n].vring.num;
>   }

This is conflating different things:
- vq does not exist (num == 0)
- vq is not setup by the guest (desc == 0)
- vq has no valid alignment (which is only relevant for legacy)

Re: [Qemu-devel] [PATCH v3 1/2] virtio: check VirtQueue Vring object is set

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1257 bytes --]

On Tue, Nov 28, 2017 at 10:11:54AM +0100, Cornelia Huck wrote:
> On Mon, 27 Nov 2017 23:25:28 +0530 (IST)
> P J P <ppandit@redhat.com> wrote:
> > +-- On Mon, 27 Nov 2017, Stefan Hajnoczi wrote --+
> > | > +    if (!vdev->vq[n].vring.num || !desc || !vdev->vq[n].vring.align) {
> > | ...
> > |   vdev->vq[n].vring.desc = desc;
> > |
> > | Why !desc?
> > 
> > virtio_queue_set_rings
> >  virtio_init_region_cache
> >    VirtQueue *vq = &vdev->vq[n];
> >    ...
> >    addr = vq->vring.desc;
> >    if (!addr) {
> >        return;
> >    }
> > 
> > These checks seem to be repeating all over. As mentioned earlier, could these 
> > be collated in one place, maybe virtio_queue_get_num()?
> > 
> >   int virtio_queue_get_num(VirtIODevice *vdev, int n)
> >   {
> >       VirtQueue *vq = &vdev->vq[n];
> > 
> >       if (!vq->.vring.num
> >            || !vq->vring.desc
> >            || !vq->vring.align) {
> >           return 0;  /* vq not set */
> >       }
> > 
> >       return vdev->vq[n].vring.num;
> >   }
> 
> This is conflating different things:
> - vq does not exist (num == 0)
> - vq is not setup by the guest (desc == 0)
> - vq has no valid alignment (which is only relevant for legacy)

I agree.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v3 1/2] virtio: check VirtQueue Vring object is set

[P J P]

+-- On Tue, 28 Nov 2017, Stefan Hajnoczi wrote --+
| > This is conflating different things:
| > - vq does not exist (num == 0)
| > - vq is not setup by the guest (desc == 0)
| > - vq has no valid alignment (which is only relevant for legacy)
| 
| I agree.

Either case, vq would be unfit for use, no?

--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH v3 1/2] virtio: check VirtQueue Vring object is set

[Cornelia Huck]

On Tue, 28 Nov 2017 16:57:34 +0530 (IST)
P J P <ppandit@redhat.com> wrote:

> +-- On Tue, 28 Nov 2017, Stefan Hajnoczi wrote --+
> | > This is conflating different things:
> | > - vq does not exist (num == 0)
> | > - vq is not setup by the guest (desc == 0)
> | > - vq has no valid alignment (which is only relevant for legacy)
> | 
> | I agree.
> 
> Either case, vq would be unfit for use, no?

What is "unfit for use"?

I'm not quite sure what you want to achieve with this patch. I assume
you want to fix the issue that a guest may provide invalid values for
align etc. which can cause qemu to crash or behave badly.

If so, you need to do different things for the different points above.
- The guest should not muck around with a non-existing queue (num == 0)
  in any case, so this should be fenced for any manipulation triggered
  by the guest.
- Processing a non-setup queue (desc == 0; also applies to the other
  buffers for virtio-1) should be skipped. However, _setting_ desc etc.
  to 0 from the guest is fine (as long as it follows the other
  constraints of the spec).
- Setting alignment to 0 only applies to legacy + virtio-mmio. I would
  not overengineer fencing this. A simple check in update_rings should
  be enough.

Re: [Qemu-devel] [PATCH v3 1/2] virtio: check VirtQueue Vring object is set

[P J P]

  Hello Cornelia,

+-- On Tue, 28 Nov 2017, Cornelia Huck wrote --+
| What is "unfit for use"?

Unfit for use because we see checks like

  if (!virtio_queue_get_num(vdev, n)) {
            continue;
  ...
  if (!vdev->vq[n].vring.num) {
      return;

'virtio_queue_set_rings' sets 'vring.desc' as

  vdev->vq[n].vring.desc = desc;

and calls virtio_init_region_cache(vdev, n);
which returns if vq->vring.desc is zero(0).

  addr = vq->vring.desc;
  if (!addr) {
      return;
  }

Same in virtio_queue_set_addr() -> virtio_queue_update_rings().

It seems that for 'vq' instance to be useful, vring.num, vring.desc etc. 
fields need to be set properly. Unless an unused/free 'vq' is being accessed 
to set these fields.

| I'm not quite sure what you want to achieve with this patch. I assume
| you want to fix the issue that a guest may provide invalid values for
| align etc. which can cause qemu to crash or behave badly.

True. In the process I'm trying to figure out if a usable 'vq' instance could 
be decided in once place, than having repeating checks, if possible.

Ex. 'virtio_queue_update_rings' is called as

virtio_queue_set_addr
 -> virtio_queue_update_rings

virtio_queue_set_align
 -> virtio_queue_update_rings

virtio_load
 for (i = 0; i < num; i++) {
   if (vdev->vq[i].vring.desc) {
   ...
     virtio_queue_update_rings

Of these, virtio_load checks that 'vring.desc' is non-zero(0). Current 
patch adds couple checks to the other two callers above. And again,

virtio_queue_update_rings would check

    if (!vring->num || !vring->desc || !vring->align) {
       /* not yet setup -> nothing to do */
       return;
    }

| If so, you need to do different things for the different points above.
| - The guest should not muck around with a non-existing queue (num == 0)
|   in any case, so this should be fenced for any manipulation triggered
|   by the guest.

I guess done by !virtio_queue_get_num() check above?

| - Processing a non-setup queue (desc == 0; also applies to the other
|   buffers for virtio-1) should be skipped. However, _setting_ desc etc.
|   to 0 from the guest is fine (as long as it follows the other
|   constraints of the spec).

Okay. Though its non-zero(0) value is preferred?

| - Setting alignment to 0 only applies to legacy + virtio-mmio. I would
|   not overengineer fencing this. A simple check in update_rings should
|   be enough.

Okay.x


Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH v3 1/2] virtio: check VirtQueue Vring object is set

[Cornelia Huck]

On Wed, 29 Nov 2017 15:41:45 +0530 (IST)
P J P <ppandit@redhat.com> wrote:

>   Hello Cornelia,
> 
> +-- On Tue, 28 Nov 2017, Cornelia Huck wrote --+
> | What is "unfit for use"?
> 
> Unfit for use because we see checks like
> 
>   if (!virtio_queue_get_num(vdev, n)) {
>             continue;
>   ...
>   if (!vdev->vq[n].vring.num) {
>       return;
> 
> 'virtio_queue_set_rings' sets 'vring.desc' as
> 
>   vdev->vq[n].vring.desc = desc;
> 
> and calls virtio_init_region_cache(vdev, n);
> which returns if vq->vring.desc is zero(0).
> 
>   addr = vq->vring.desc;
>   if (!addr) {
>       return;
>   }
> 
> Same in virtio_queue_set_addr() -> virtio_queue_update_rings().
> 
> It seems that for 'vq' instance to be useful, vring.num, vring.desc etc. 
> fields need to be set properly. Unless an unused/free 'vq' is being accessed 
> to set these fields.

I think the basic problem is still that you conflate two things:
- vring.num, which cannot be flipped between 0 and !0 by the guest
- vring.{desc,avail,used}, which can

IOW, if vring.num == 0, the guest cannot manipulate the queue; if
vring.desc == 0, the guest can. 

> 
> | I'm not quite sure what you want to achieve with this patch. I assume
> | you want to fix the issue that a guest may provide invalid values for
> | align etc. which can cause qemu to crash or behave badly.
> 
> True. In the process I'm trying to figure out if a usable 'vq' instance could 
> be decided in once place, than having repeating checks, if possible.
> 
> Ex. 'virtio_queue_update_rings' is called as
> 
> virtio_queue_set_addr
>  -> virtio_queue_update_rings  
> 
> virtio_queue_set_align
>  -> virtio_queue_update_rings  
> 
> virtio_load
>  for (i = 0; i < num; i++) {
>    if (vdev->vq[i].vring.desc) {
>    ...
>      virtio_queue_update_rings
> 
> Of these, virtio_load checks that 'vring.desc' is non-zero(0). Current 
> patch adds couple checks to the other two callers above. And again,
> 
> virtio_queue_update_rings would check
> 
>     if (!vring->num || !vring->desc || !vring->align) {
>        /* not yet setup -> nothing to do */
>        return;
>     }

vring.num and vring.desc are really different things. You don't want
the guest to do anything with the queue if vring.num == 0, while you
just want to skip various processing if vring.desc == 0.

(virtio_load() does not need to care about vring.num, as it is not
triggered by the guest.)

> 
> | If so, you need to do different things for the different points above.
> | - The guest should not muck around with a non-existing queue (num == 0)
> |   in any case, so this should be fenced for any manipulation triggered
> |   by the guest.
> 
> I guess done by !virtio_queue_get_num() check above?

Yes.

> 
> | - Processing a non-setup queue (desc == 0; also applies to the other
> |   buffers for virtio-1) should be skipped. However, _setting_ desc etc.
> |   to 0 from the guest is fine (as long as it follows the other
> |   constraints of the spec).
> 
> Okay. Though its non-zero(0) value is preferred?

Many functions have a likely/unlikely check, setup routines excepted.

> 
> | - Setting alignment to 0 only applies to legacy + virtio-mmio. I would
> |   not overengineer fencing this. A simple check in update_rings should
> |   be enough.
> 
> Okay.x

Re: [Qemu-devel] [PATCH v3 1/2] virtio: check VirtQueue Vring object is set

[P J P]

+-- On Wed, 29 Nov 2017, Cornelia Huck wrote --+
| I think the basic problem is still that you conflate two things:
| - vring.num, which cannot be flipped between 0 and !0 by the guest
| - vring.{desc,avail,used}, which can
| 
| IOW, if vring.num == 0, the guest cannot manipulate the queue; if
| vring.desc == 0, the guest can. 
| 
| Many functions have a likely/unlikely check, setup routines excepted.

Have sent a revised patch v4. Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F