Skip to contentSkip to conversation

Fix possible OOB during queuing packets

PATCH Superseded Jason Wang 5 participants 10 messages Nov 29, 2018 – Nov 30, 2018 Networking

v2 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Jason Wang

author
#0 lore raw

v2 cover Fix possible OOB during queuing packets

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(-)
Signature · 1 line
2.17.1
author
#1 lore raw

PATCH v2 1/4 net: drop too large packet early

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.

---
 net/net.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)
net/net.c +7 -6
index 07c194a8f6..affe1877cf 100644
@@ -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 */
Signature · 1 line
2.17.1
author
#2 lore raw

PATCH v2 2/4 virtio-net-test: remove unused macro

In reply to Jason Wang
---
 tests/virtio-net-test.c | 1 -
 1 file changed, 1 deletion(-)
tests/virtio-net-test.c +0 -1
index dcb87a8b6e..231e7c767e 100644
@@ -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)
Signature · 1 line
2.17.1
author
#3 lore raw

PATCH v2 3/4 virtio-net-test: accept variable length argument in pci_test_start()

In reply to Jason Wang

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

---
 tests/virtio-net-test.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)
tests/virtio-net-test.c +11 -6
index 231e7c767e..33d26ab079 100644
@@ -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);
Signature · 1 line
2.17.1
author
#4 lore raw

PATCH v2 4/4 virtio-net-test: add large tx buffer test

In reply to 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.

---
 tests/virtio-net-test.c | 45 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)
tests/virtio-net-test.c +45 -0
index 33d26ab079..1b39a73b6a 100644
@@ -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);
 
Signature · 1 line
2.17.1
Reviewed-by on v2 4/4
#5 lore raw

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

Quoted 8 lines · 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.

---
 tests/virtio-net-test.c | 45 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)
#6 lore raw
In reply to Jason Wang
Quoted 11 lines · 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.

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?

Signature · 3 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
#7 lore raw
+-- 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

author
#8 lore raw
Quoted 4 lines · 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)?

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

Quoted 13 lines

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
Quoted 5 lines

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

#9 lore raw
In reply to P J P
Quoted 7 lines · 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?
Quoted 8 lines

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

Signature · 3 lines
|: 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 :|