[V4,for,3.1,4/4] virtio-net-test: add large tx buffer test

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(+)

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