Message ID | 20181203100608.28538-5-jasowang@redhat.com |
---|---|
State | New |
Headers | show |
Series | Fix possible OOB during queuing packets | expand |
On 12/3/18 4:06 AM, 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. Can you also add a packet just slightly larger than NET_BUFSIZE (68k) to show that we aren't having any further issues at even smaller (and more likely) values of oversized packets? > > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > tests/virtio-net-test.c | 44 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > +++ b/tests/virtio-net-test.c > @@ -245,6 +245,49 @@ 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; > + uint64_t req_addr; > + uint32_t free_head; > + size_t alloc_size = UINT_MAX / 64; > + int i; > + > + qs = pci_test_start("-netdev hubport,id=hp0,hubid=0 " > + "-device virtio-net-pci,netdev=hp0 "); Why the trailing space? > + 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); Any time I see both 'true' and 'false' in a ?: operator, I have to wonder why you didn't just write the simpler version directly on the condition. This is the same as 'i != 63'. > + } > + 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 +313,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); > I can reproduce Peter's complaints of this introducing noise to 'make check': qemu-system-x86_64: warning: hub 0 is not connected to host network With patches 2-4 applied but patch 1 omitted, 'make check' fails with: Broken pipe tests/libqtest.c:125: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped) GTester: last random seed: R02S9569aabcb2d834a9dadcce272c5588db so the test is definitely triggering the problem as patched by part 1. Although I'm not confident enough of what the test is doing for an R-b, and would like it to be less noisy, I can at least add: Tested-by: Eric Blake <eblake@redhat.com>
On 2018/12/4 上午12:46, Eric Blake wrote: > On 12/3/18 4:06 AM, 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. > > Can you also add a packet just slightly larger than NET_BUFSIZE (68k) > to show that we aren't having any further issues at even smaller (and > more likely) values of oversized packets? Ok. > >> >> Signed-off-by: Jason Wang <jasowang@redhat.com> >> --- >> tests/virtio-net-test.c | 44 +++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 44 insertions(+) >> > >> +++ b/tests/virtio-net-test.c >> @@ -245,6 +245,49 @@ 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; >> + uint64_t req_addr; >> + uint32_t free_head; >> + size_t alloc_size = UINT_MAX / 64; >> + int i; >> + >> + qs = pci_test_start("-netdev hubport,id=hp0,hubid=0 " >> + "-device virtio-net-pci,netdev=hp0 "); > > Why the trailing space? I guess this is a cut and paste error. > >> + 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); > > Any time I see both 'true' and 'false' in a ?: operator, I have to > wonder why you didn't just write the simpler version directly on the > condition. This is the same as 'i != 63'. > Ok. >> + } >> + 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 +313,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); > > I can reproduce Peter's complaints of this introducing noise to 'make > check': > qemu-system-x86_64: warning: hub 0 is not connected to host network Yes, this is intended since the test does not require any host network. I will add a patch to suppress this warning if qtest is enabled. > > With patches 2-4 applied but patch 1 omitted, 'make check' fails with: > > Broken pipe > tests/libqtest.c:125: kill_qemu() detected QEMU death from signal 11 > (Segmentation fault) (core dumped) > GTester: last random seed: R02S9569aabcb2d834a9dadcce272c5588db > > so the test is definitely triggering the problem as patched by part 1. > Although I'm not confident enough of what the test is doing for an > R-b, and would like it to be less noisy, I can at least add: > > Tested-by: Eric Blake <eblake@redhat.com> > Thanks
diff --git a/tests/virtio-net-test.c b/tests/virtio-net-test.c index bdd6af9999..566596a397 100644 --- a/tests/virtio-net-test.c +++ b/tests/virtio-net-test.c @@ -245,6 +245,49 @@ 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; + uint64_t req_addr; + uint32_t free_head; + size_t alloc_size = UINT_MAX / 64; + int i; + + qs = pci_test_start("-netdev hubport,id=hp0,hubid=0 " + "-device virtio-net-pci,netdev=hp0 "); + 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 +313,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);
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 | 44 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)