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

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

Changes from V1:
- slient compiling warnings
Changes from V2:
- don't use variable length argument

Jason Wang (4):
  net: drop too large packet early
  virtio-net-test: accept command line string instead of socket
  virtio-net-test: remove unused macro
  virtio-net-test: add large tx buffer test

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

-- 
2.17.1

[Qemu-devel] [PATCH V3 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 V3 for 3.1 2/4] virtio-net-test: accept command line string instead of socket

[Jason Wang]

This will allow passing different kinds of command line string.

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

diff --git a/tests/virtio-net-test.c b/tests/virtio-net-test.c
index dcb87a8b6e..233b9ab5f7 100644
--- a/tests/virtio-net-test.c
+++ b/tests/virtio-net-test.c
@@ -52,17 +52,15 @@ 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;
     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);
+        qs = qtest_pc_boot(cmd);
     } else if (strcmp(arch, "ppc64") == 0) {
-        qs = qtest_spapr_boot(cmd, socket);
+        qs = qtest_spapr_boot(cmd);
     } else {
         g_printerr("virtio-net tests are only available on x86 or ppc64\n");
         exit(EXIT_FAILURE);
@@ -219,11 +217,15 @@ static void pci_basic(gconstpointer data)
                   QVirtQueue *tvq,
                   int socket) = data;
     int sv[2], ret;
+    char cmd[256];
 
     ret = socketpair(PF_UNIX, SOCK_STREAM, 0, sv);
     g_assert_cmpint(ret, !=, -1);
 
-    qs = pci_test_start(sv[1]);
+    sprintf(cmd, "-netdev socket,fd=%d,id=hs0 "
+            "-device virtio-net-pci,netdev=hs0", sv[1]);
+
+    qs = pci_test_start(cmd);
     dev = virtio_net_pci_init(qs->pcibus, PCI_SLOT);
 
     rx = (QVirtQueuePCI *)qvirtqueue_setup(&dev->vdev, qs->alloc, 0);
-- 
2.17.1

[Qemu-devel] [PATCH V3 for 3.1 3/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 233b9ab5f7..f12f0afb51 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 V3 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.

Reviewed-by: Thomas Huth <thuth@redhat.com>
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 f12f0afb51..22df5b6f37 100644
--- a/tests/virtio-net-test.c
+++ b/tests/virtio-net-test.c
@@ -242,6 +242,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)
@@ -267,6 +311,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 V3 for 3.1 0/4] Fix possible OOB during queuing packets

[Jason Wang]

On 2018/12/3 下午5:35, 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.
>
> Thanks


Wrong version, please ignore this.

Thanks