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

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

[P J P]

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

Hello,

A guest 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      | 14 +++++++++++---
 tests/virtio-blk-test.c | 25 +++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 3 deletions(-)

--
2.13.6

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

[P J P]

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

A guest 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(-)

Update: removed !desc and !vring.align check from virtio_queue_set_rings
  -> https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg04809.html

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 5884ce3480..a0d2c887cc 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 (!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) {
+        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 v4 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 v4 1/2] virtio: check VirtQueue Vring object is set

[Cornelia Huck]

On Wed, 29 Nov 2017 23:14:27 +0530
P J P <ppandit@redhat.com> wrote:

> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> A guest 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(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

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

[Stefan Hajnoczi]

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

On Wed, Nov 29, 2017 at 11:14:26PM +0530, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> Hello,
> 
> A guest 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      | 14 +++++++++++---
>  tests/virtio-blk-test.c | 25 +++++++++++++++++++++++++
>  2 files changed, 36 insertions(+), 3 deletions(-)
> 
> --
> 2.13.6

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

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

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

[Stefan Hajnoczi]

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

On Wed, Nov 29, 2017 at 11:14:26PM +0530, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>

Michael is the virtio maintainer.  I have added him to this email
thread so the patch series can be merged.

> 
> Hello,
> 
> A guest 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      | 14 +++++++++++---
>  tests/virtio-blk-test.c | 25 +++++++++++++++++++++++++
>  2 files changed, 36 insertions(+), 3 deletions(-)
> 
> --
> 2.13.6

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

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

[P J P]

+-- On Thu, 30 Nov 2017, Stefan Hajnoczi wrote --+
| Michael is the virtio maintainer.  I have added him to this email
| thread so the patch series can be merged.

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

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

[P J P]

+-- On Thu, 30 Nov 2017, P J P wrote --+
| +-- On Thu, 30 Nov 2017, Stefan Hajnoczi wrote --+
| | Michael is the virtio maintainer.  I have added him to this email
| | thread so the patch series can be merged.

  -> https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg05473.html

@mst: this qtest is not pulled in it seems.

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

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

[Michael S. Tsirkin]

On Thu, Dec 07, 2017 at 12:37:42PM +0530, P J P wrote:
> +-- On Thu, 30 Nov 2017, P J P wrote --+
> | +-- On Thu, 30 Nov 2017, Stefan Hajnoczi wrote --+
> | | Michael is the virtio maintainer.  I have added him to this email
> | | thread so the patch series can be merged.
> 
>   -> https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg05473.html
> 
> @mst: this qtest is not pulled in it seems.
> 
> Thank you.

That can wait until after the release, pls ping me then.

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