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

[Qemu-devel] [PATCH v1 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

 hw/virtio/virtio.c      | 11 ++++++++---
 tests/virtio-blk-test.c | 21 +++++++++++++++++++++
 2 files changed, 29 insertions(+), 3 deletions(-)

-- 
2.13.6

[Qemu-devel] [PATCH v1 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 | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Updated: add vring.num check to virtio_queue_update_rings
  -> https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg04499.html

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 5884ce3480..0940f1daf3 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);
 }
@@ -1494,8 +1497,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

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

[Cornelia Huck]

On Fri, 24 Nov 2017 13:25:41 +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
> 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 | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> Updated: add vring.num check to virtio_queue_update_rings
>   -> https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg04499.html  
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 5884ce3480..0940f1daf3 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 */

The check now implies "not yet setup or non-existing queue", no?

(Not that the check is wrong.)

>          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);
>  }
> @@ -1494,8 +1497,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)

I think you also need to check for num in virtio_queue_set_rings().

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

[Stefan Hajnoczi]

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

On Fri, Nov 24, 2017 at 01:25:41PM +0530, P J P wrote:
> 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 | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> Updated: add vring.num check to virtio_queue_update_rings
>   -> https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg04499.html

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

[Qemu-devel] [PATCH v1 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 | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index e6fb9bac87..d00ceb0501 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -674,6 +674,26 @@ static void pci_hotplug(void)
     qtest_shutdown(qs);
 }
 
+static void test_vring_align(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 +744,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/vring", test_vring_align);
         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 v1 2/2] tests: add test to check VirtQueue object

[Stefan Hajnoczi]

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

On Fri, Nov 24, 2017 at 01:25:42PM +0530, P J P wrote:
> 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 | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
> index e6fb9bac87..d00ceb0501 100644
> --- a/tests/virtio-blk-test.c
> +++ b/tests/virtio-blk-test.c
> @@ -674,6 +674,26 @@ static void pci_hotplug(void)
>      qtest_shutdown(qs);
>  }
>  
> +static void test_vring_align(void)

It's important to include a doc comment so the purpose of the test is
clear:

/*
 * Check that setting the vring addr on a non-existent virtqueue does
 * not crash.
 */

I think the _align name is confusing since the vring alignment is not
directly affected by this test case.  Please call it
test_nonexistent_virtqueue() instead.

> +{
> +    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);
> +}


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

Re: [Qemu-devel] [PATCH v1 0/2] check VirtiQueue Vring objects

[Paolo Bonzini]

On 24/11/2017 08:55, P J P wrote:
> 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
> 
>  hw/virtio/virtio.c      | 11 ++++++++---
>  tests/virtio-blk-test.c | 21 +++++++++++++++++++++
>  2 files changed, 29 insertions(+), 3 deletions(-)
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>