[Qemu-devel] [PATCH V2 for 3.1 0/4] Fix possible OOB during queuing packets

[Qemu-devel] [PATCH V2 for 3.1 0/4] Fix possible OOB during queuing packets

[Jason Wang]

Hi:

This series tries to fix a possible OOB during queueing packets
through qemu_net_queue_append_iov(). This could happen when it tries
to queue a packet whose size is larger than INT_MAX which may lead
integer overflow. We've fixed similar issue in the past during
qemu_net_queue_deliver_iov() by ignoring large packets there. Let's
just move the check earlier to qemu_sendv_packet_async() and reduce
the limitation to NET_BUFSIZE. A simple qtest were also added this.

Please review.

Thanks

Jason Wang (4):
  net: drop too large packet early
  virtio-net-test: remove unused macro
  virtio-net-test: accept variable length argument in pci_test_start()
  virtio-net-test: add large tx buffer test

 net/net.c               | 13 +++++----
 tests/virtio-net-test.c | 63 ++++++++++++++++++++++++++++++++++++-----
 2 files changed, 63 insertions(+), 13 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH V2 for 3.1 1/4] net: drop too large packet early

[Jason Wang]

We try to detect and drop too large packet (>INT_MAX) in 1592a9947036
("net: ignore packet size greater than INT_MAX") during packet
delivering. Unfortunately, this is not sufficient as we may hit
another integer overflow when trying to queue such large packet in
qemu_net_queue_append_iov():

- size of the allocation may overflow on 32bit
- packet->size is integer which may overflow even on 64bit

Fixing this by move the check to qemu_sendv_packet_async() which is
the entrance of all networking codes and reduce the limit to
NET_BUFSIZE to be more conservative.

Cc: qemu-stable@nongnu.org
Cc: Li Qiang <liq3ea@163.com>
Reported-by: Li Qiang <liq3ea@gmail.com>
Reviewed-by: Li Qiang <liq3ea@gmail.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 net/net.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/net/net.c b/net/net.c
index 07c194a8f6..affe1877cf 100644
--- a/net/net.c
+++ b/net/net.c
@@ -712,15 +712,11 @@ ssize_t qemu_deliver_packet_iov(NetClientState *sender,
                                 void *opaque)
 {
     NetClientState *nc = opaque;
-    size_t size = iov_size(iov, iovcnt);
     int ret;
 
-    if (size > INT_MAX) {
-        return size;
-    }
 
     if (nc->link_down) {
-        return size;
+        return iov_size(iov, iovcnt);
     }
 
     if (nc->receive_disabled) {
@@ -745,10 +741,15 @@ ssize_t qemu_sendv_packet_async(NetClientState *sender,
                                 NetPacketSent *sent_cb)
 {
     NetQueue *queue;
+    size_t size = iov_size(iov, iovcnt);
     int ret;
 
+    if (size > NET_BUFSIZE) {
+        return size;
+    }
+
     if (sender->link_down || !sender->peer) {
-        return iov_size(iov, iovcnt);
+        return size;
     }
 
     /* Let filters handle the packet first */
-- 
2.17.1

[Qemu-devel] [PATCH V2 for 3.1 2/4] virtio-net-test: remove unused macro

[Jason Wang]

Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 tests/virtio-net-test.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tests/virtio-net-test.c b/tests/virtio-net-test.c
index dcb87a8b6e..231e7c767e 100644
--- a/tests/virtio-net-test.c
+++ b/tests/virtio-net-test.c
@@ -24,7 +24,6 @@
 
 #define PCI_SLOT_HP             0x06
 #define PCI_SLOT                0x04
-#define PCI_FN                  0x00
 
 #define QVIRTIO_NET_TIMEOUT_US (30 * 1000 * 1000)
 #define VNET_HDR_SIZE sizeof(struct virtio_net_hdr_mrg_rxbuf)
-- 
2.17.1

[Qemu-devel] [PATCH V2 for 3.1 3/4] virtio-net-test: accept variable length argument in pci_test_start()

[Jason Wang]

This allows flexibility to be reused for all kinds of command line
used by other tests.

Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 tests/virtio-net-test.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/tests/virtio-net-test.c b/tests/virtio-net-test.c
index 231e7c767e..33d26ab079 100644
--- a/tests/virtio-net-test.c
+++ b/tests/virtio-net-test.c
@@ -51,17 +51,20 @@ static QVirtioPCIDevice *virtio_net_pci_init(QPCIBus *bus, int slot)
     return dev;
 }
 
-static QOSState *pci_test_start(int socket)
+static QOSState *pci_test_start(const char *cmd, ...)
 {
     QOSState *qs;
+    va_list ap;
     const char *arch = qtest_get_arch();
-    const char *cmd = "-netdev socket,fd=%d,id=hs0 -device "
-                      "virtio-net-pci,netdev=hs0";
 
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
-        qs = qtest_pc_boot(cmd, socket);
+        va_start(ap, cmd);
+        qs = qtest_pc_vboot(cmd, ap);
+        va_end(ap);
     } else if (strcmp(arch, "ppc64") == 0) {
-        qs = qtest_spapr_boot(cmd, socket);
+        va_start(ap, cmd);
+        qs = qtest_spapr_vboot(cmd, ap);
+        va_end(ap);
     } else {
         g_printerr("virtio-net tests are only available on x86 or ppc64\n");
         exit(EXIT_FAILURE);
@@ -218,11 +221,13 @@ static void pci_basic(gconstpointer data)
                   QVirtQueue *tvq,
                   int socket) = data;
     int sv[2], ret;
+    const char *cmd = "-netdev socket,fd=%d,id=hs0 -device "
+                      "virtio-net-pci,netdev=hs0";
 
     ret = socketpair(PF_UNIX, SOCK_STREAM, 0, sv);
     g_assert_cmpint(ret, !=, -1);
 
-    qs = pci_test_start(sv[1]);
+    qs = pci_test_start(cmd, sv[1]);
     dev = virtio_net_pci_init(qs->pcibus, PCI_SLOT);
 
     rx = (QVirtQueuePCI *)qvirtqueue_setup(&dev->vdev, qs->alloc, 0);
-- 
2.17.1

[Qemu-devel] [PATCH V2 for 3.1 4/4] virtio-net-test: add large tx buffer test

[Jason Wang]

This test tries to build a packet whose size is greater than INT_MAX
which tries to trigger integer overflow in qemu_net_queue_append_iov()
which may result OOB.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 tests/virtio-net-test.c | 45 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/tests/virtio-net-test.c b/tests/virtio-net-test.c
index 33d26ab079..1b39a73b6a 100644
--- a/tests/virtio-net-test.c
+++ b/tests/virtio-net-test.c
@@ -245,6 +245,50 @@ static void pci_basic(gconstpointer data)
     g_free(dev);
     qtest_shutdown(qs);
 }
+
+static void large_tx(gconstpointer data)
+{
+    QVirtioPCIDevice *dev;
+    QOSState *qs;
+    QVirtQueuePCI *tx, *rx;
+    QVirtQueue *vq;
+    const char *cmd = "-netdev hubport,id=hp0,hubid=0 "
+                      "-device virtio-net-pci,netdev=hp0 ";
+    uint64_t req_addr;
+    uint32_t free_head;
+    size_t alloc_size = UINT_MAX / 64;
+    int i;
+
+    qs = pci_test_start(cmd);
+    dev = virtio_net_pci_init(qs->pcibus, PCI_SLOT);
+
+    rx = (QVirtQueuePCI *)qvirtqueue_setup(&dev->vdev, qs->alloc, 0);
+    tx = (QVirtQueuePCI *)qvirtqueue_setup(&dev->vdev, qs->alloc, 1);
+
+    driver_init(&dev->vdev);
+    vq = &tx->vq;
+
+    /* Bypass the limitation by pointing several descriptors to a single
+     * smaller area */
+    req_addr = guest_alloc(qs->alloc, alloc_size);
+    free_head = qvirtqueue_add(vq, req_addr, alloc_size, false, true);
+
+    for (i = 0; i < 64; i++) {
+        qvirtqueue_add(vq, req_addr, alloc_size, false, i == 63 ?
+                       false : true);
+    }
+    qvirtqueue_kick(&dev->vdev, vq, free_head);
+
+    qvirtio_wait_used_elem(&dev->vdev, vq, free_head, NULL,
+                           QVIRTIO_NET_TIMEOUT_US);
+
+    qvirtqueue_cleanup(dev->vdev.bus, &tx->vq, qs->alloc);
+    qvirtqueue_cleanup(dev->vdev.bus, &rx->vq, qs->alloc);
+    qvirtio_pci_device_disable(dev);
+    g_free(dev->pdev);
+    g_free(dev);
+    qtest_shutdown(qs);
+}
 #endif
 
 static void hotplug(void)
@@ -270,6 +314,7 @@ int main(int argc, char **argv)
     qtest_add_data_func("/virtio/net/pci/basic", send_recv_test, pci_basic);
     qtest_add_data_func("/virtio/net/pci/rx_stop_cont",
                         stop_cont_test, pci_basic);
+    qtest_add_data_func("/virtio/net/pci/large_tx", NULL, large_tx);
 #endif
     qtest_add_func("/virtio/net/pci/hotplug", hotplug);
 
-- 
2.17.1

Re: [Qemu-devel] [PATCH V2 for 3.1 4/4] virtio-net-test: add large tx buffer test

[Thomas Huth]

On 2018-11-29 13:14, Jason Wang wrote:
> This test tries to build a packet whose size is greater than INT_MAX
> which tries to trigger integer overflow in qemu_net_queue_append_iov()
> which may result OOB.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  tests/virtio-net-test.c | 45 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH V2 for 3.1 0/4] Fix possible OOB during queuing packets

[Eric Blake]

On 11/29/18 6:14 AM, Jason Wang wrote:
> Hi:
> 
> This series tries to fix a possible OOB during queueing packets
> through qemu_net_queue_append_iov(). This could happen when it tries
> to queue a packet whose size is larger than INT_MAX which may lead
> integer overflow. We've fixed similar issue in the past during
> qemu_net_queue_deliver_iov() by ignoring large packets there. Let's
> just move the check earlier to qemu_sendv_packet_async() and reduce
> the limitation to NET_BUFSIZE. A simple qtest were also added this.
> 
> Please review.

How important is this for 3.1?  We've missed -rc3.  Is this CVE quality 
because of a guest being able to cause mayhem by intentionally getting 
into this condition (in which case, we need it, as well as a CVE 
assigned)? Is it pre-existing in 3.0 at which point waiting for 4.0 is 
no worse off than what we already are?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH V2 for 3.1 0/4] Fix possible OOB during queuing packets

[P J P]

+-- On Thu, 29 Nov 2018, Eric Blake wrote --+
| How important is this for 3.1?  We've missed -rc3.  Is this CVE quality 
| because of a guest being able to cause mayhem by intentionally getting into 
| this condition (in which case, we need it, as well as a CVE assigned)? Is it 
| pre-existing in 3.0 at which point waiting for 4.0 is no worse off than what 
| we already are?

It is a revised patch to fix 'CVE-2018-17963' issue. Earlier patch was 
included in -rc0.

$ git tag --contains 1592a9947036d60dde5404204a5d45975133caf5
v3.1.0-rc0
v3.1.0-rc1
v3.1.0-rc2
v3.1.0-rc3

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

Re: [Qemu-devel] [PATCH V2 for 3.1 0/4] Fix possible OOB during queuing packets

[Jason Wang]

On 2018/11/30 下午5:18, P J P wrote:
> +-- On Thu, 29 Nov 2018, Eric Blake wrote --+
> | How important is this for 3.1?  We've missed -rc3.  Is this CVE quality
> | because of a guest being able to cause mayhem by intentionally getting into
> | this condition (in which case, we need it, as well as a CVE assigned)?


Yes, malicious guest can do this, but only with some specific setup e.g 
with hubports.


>   Is it
> | pre-existing in 3.0 at which point waiting for 4.0 is no worse off than what
> | we already are?
>
> It is a revised patch to fix 'CVE-2018-17963' issue. Earlier patch was
> included in -rc0.
>
> $ git tag --contains 1592a9947036d60dde5404204a5d45975133caf5
> v3.1.0-rc0
> v3.1.0-rc1
> v3.1.0-rc2
> v3.1.0-rc3
>
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
>

Yes, it could be treated as a follow up fixes for CVE-2018-17963. I 
think we need this.

Thanks

Re: [Qemu-devel] [PATCH V2 for 3.1 0/4] Fix possible OOB during queuing packets

[Daniel P. Berrangé]

On Fri, Nov 30, 2018 at 02:48:19PM +0530, P J P wrote:
> +-- On Thu, 29 Nov 2018, Eric Blake wrote --+
> | How important is this for 3.1?  We've missed -rc3.  Is this CVE quality 
> | because of a guest being able to cause mayhem by intentionally getting into 
> | this condition (in which case, we need it, as well as a CVE assigned)? Is it 
> | pre-existing in 3.0 at which point waiting for 4.0 is no worse off than what 
> | we already are?
> 
> It is a revised patch to fix 'CVE-2018-17963' issue. Earlier patch was 
> included in -rc0.
> 
> $ git tag --contains 1592a9947036d60dde5404204a5d45975133caf5
> v3.1.0-rc0
> v3.1.0-rc1
> v3.1.0-rc2
> v3.1.0-rc3

If we've already tried to fix CVE-2018-17963 in 3.1.0 and failed to address
some edge cases, then it would be really desirable to get this into 3.1.0
too.  If we waited until 4.0.0, then we'd need to consider it to be a new
CVE on the grounds that 3.1.0 released a flawed fix.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|