Fix possible OOB during queuing packets
v4 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
Changes from V1:
- slient compiling warnings
Changes from V2:
- annotate pci_test_start() with GCC_FMT_ATTR()
- drop intermediate cmd string variable
Jason Wang (4):
net: drop too large packet early
virtio-net-test: accept variable length argument in pci_test_start()
virtio-net-test: remove unused macro
virtio-net-test: add large tx buffer test
net/net.c | 13 +++++---- tests/virtio-net-test.c | 62 ++++++++++++++++++++++++++++++++++++----- 2 files changed, 62 insertions(+), 13 deletions(-)
Signature · 1 line
2.17.1
PATCH v4 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
@@ -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
Re: [Qemu-devel] [PATCH V4 for 3.1 1/4] net: drop too large packet early
Quoted 10 lines · Jason Wang
We try to detect and drop too large packet (>INT_MAX) in
1592a9947036("net: ignore packet size greater thanINT_MAX") during packet delivering. Unfortunately, this is not sufficient as we may hit another integer overflow when trying to queue such large packet inqemu_net_queue_append_iov():- size of the allocation may overflow on 32bit
-packet->sizeis integer which may overflow even on 64bitFixing this by move the check to
qemu_sendv_packet_async()which is
s/move/moving/
Quoted 2 lines
the entrance of all networking codes and reduce the limit to
NET_BUFSIZEto be more conservative.
Please mention commit 1592a994 in the commit message (since you are effectively reverting that with this as its replacement), and if this is (as I suspect) an additional patch required for the complete fix to CVE-2018-10839, also mention that.
Quoted 18 lines
Cc Li Qiang <[email protected]>Reported-by Li Qiang <[email protected]>Reviewed-by Li Qiang <[email protected]>Signed-off-by Jason Wang <[email protected]>--- net/net.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)net/net.c +0
-0@@ -712,15 +712,11 @@ ssize_t qemu_deliver_packet_iov(NetClientState *sender, void *opaque) {NetClientState *nc = opaque;
net/net.c ssize_t qemu_deliver_packet_iov(NetClientState *sender, · line 719
Quoted 10 lines
net/net.c +1
-5-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);
Reverts 1592a994, and...
net/net.c ssize_t qemu_sendv_packet_async(NetClientState *sender, · line 749
Quoted 13 lines
net/net.c +4
-0}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; + }
...returns early for packets larger than 68k (a much smaller limit than INT_MAX, which makes analysis for int overflow a lot easier) at a saner point in the code. Returning a large value is weird, but auditing all callers:
hw/net/virtio-net.c: ret = qemu_sendv_packet_async(qemu_get_subqueue(n->nic, queue_index),
- only checks if ret is 0 (returns -EBUSY) or not (assumes packet was
sent)net/netmap.c: iovsize = qemu_sendv_packet_async(&s->nc, s->iov, iovcnt,
- only checks if ret is 0 (starts polling) or not (assumes packet was
sent)net/net.c: return qemu_sendv_packet_async(nc, iov, iovcnt, NULL);
- implementation of qemu_sendv_packet() - so we have to audit those
callers as well:
hw/net/net_tx_pkt.c: qemu_sendv_packet(nc, iov, iov_cnt); hw/net/rocker/rocker_fp.c: qemu_sendv_packet(nc, iov, iovcnt); hw/net/rtl8139.c: qemu_sendv_packet(qemu_get_queue(s->nic), iov, 3); net/hub.c: qemu_sendv_packet(&port->nc, iov, iovcnt);
- all four of these do not check the return status
So, it looks like none of the callers cares if the return value is overlarge (no further math on the values), just that it is non-zero (where the callers then presumably assume the packet was sent). However, I am not familiar enough with the code to know if skipping the packet by returning a non-zero value is going to have knock-on effects - that is, my audit shows what the callers do, but not whether it was sane.
net/net.c ssize_t qemu_sendv_packet_async(NetClientState *sender, · line 753
Quoted 5 lines
net/net.c +2
-1+if (sender->link_down || !sender->peer) { -return iov_size (iov, iovcnt); +return size; }
If this is indeed CVE fixing, then we want it in -rc4, but I don't know if the patch is correctly secure yet without answers to my questions. Especially on a CVE fix for -rc4, you want to make the reviewer's life as easy as possible by providing a commit message that includes enough details to make analysis easy.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH V4 for 3.1 1/4] net: drop too large packet early
Quoted 7 lines · Eric Blake
Quoted 6 lines · Jason Wang
We try to detect and drop too large packet (>INT_MAX) in
1592a9947036("net: ignore packet size greater thanINT_MAX") during packet delivering. Unfortunately, this is not sufficient as we may hit another integer overflow when trying to queue such large packet inqemu_net_queue_append_iov():
Quoted 6 lines
Quoted 4 lines
- size of the allocation may overflow on 32bit
-packet->sizeis integer which may overflow even on 64bitFixing this by move the check to
qemu_sendv_packet_async()which iss/move/moving/
Ok.
Quoted 5 lines
Quoted 3 lines
the entrance of all networking codes and reduce the limit to
NET_BUFSIZEto be more conservative.Please mention commit
1592a994in the commit message (since you are effectively reverting that with this as its replacement),
I think I did it?
Quoted 1 line
and if this is (as I suspect) an additional patch required for the complete fix to CVE-2018-10839, also mention that.
Ok.
Quoted 31 lines
Quoted 18 lines
Cc Li Qiang <[email protected]>Reported-by Li Qiang <[email protected]>Reviewed-by Li Qiang <[email protected]>Signed-off-by Jason Wang <[email protected]>---
net/net.c| 13 +++++++------1 file changed, 7 insertions(+), 6 deletions(-)net/net.c +0
-0@@ -712,15 +712,11 @@ ssize_t qemu_deliver_packet_iov(NetClientState *sender, void *opaque)net/net.c ssize_t qemu_deliver_packet_iov(NetClientState · line 719
Quoted 10 lines
net/net.c +1
-4{ 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);Reverts
1592a994, and...
Quoted 14 lines
net/net.c ssize_t qemu_sendv_packet_async(NetClientState · line 749
Quoted 12 lines
net/net.c +3
-0} 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; + }...returns early for packets larger than 68k (a much smaller limit than
INT_MAX, which makes analysis for int overflow a lot easier) at a saner point in the code. Returning a large value is weird,
Might be, but we did this for years, see the following return value when link is down.
Quoted 9 lines
but auditing all callers:
hw/net/virtio-net.c: ret = qemu_sendv_packet_async(qemu_get_subqueue(n->nic,
queue_index),
- only checks if ret is 0 (returns-EBUSY) or not (assumes packet was sent) net/netmap.c: iovsize = qemu_sendv_packet_async(&s->nc,s->iov, iovcnt,
- only checks if ret is 0 (starts polling) or not (assumes packet was sent) net/net.c: returnqemu_sendv_packet_async(nc, iov, iovcnt, NULL);
- implementation ofqemu_sendv_packet()- so we have to audit those callers as well:
Quoted 7 lines
hw/net/net_tx_pkt.c:
qemu_sendv_packet(nc, iov, iov_cnt); hw/net/rocker/rocker_fp.c:qemu_sendv_packet(nc, iov, iovcnt);hw/net/rtl8139.c:qemu_sendv_packet(qemu_get_queue(s->nic), iov, 3); net/hub.c:qemu_sendv_packet(&port->nc, iov, iovcnt);
- all four of these do not check the return statusSo, it looks like none of the callers cares if the return value is overlarge (no further math on the values), just that it is non-zero (where the callers then presumably assume the packet was sent).
Yes, the caller may only care if it returns zero.
Quoted 2 lines
However, I am not familiar enough with the code to know if skipping the packet by returning a non-zero value is going to have knock-on effects - that is, my audit shows what the callers do, but not whether it was sane.
The difference between qemu_sendv_packet() and qemu_sendv_packet_async() is that the latter can trigger a callback (sent_cb) when peer can accept more packets. This could be used by some high speed networking implementation to prevent the source from producing more packets and wasting cpu cycles in dropping packets. After peer can accept more, sent_cb usually enable the source to produce packets. Those who use qemu_sendv_packet() will just waste some cpu in dropping the packets.
Consider we are emulating ethernet and packet will be copied if queued, it's safe to assume that the packet was sent.
Quoted 7 lines
net/net.c ssize_t qemu_sendv_packet_async(NetClientState *sender, · line 753
Quoted 5 lines
net/net.c +2
-1+ if (sender->link_down || !sender->peer) { - return iov_size(iov, iovcnt); + return size; }If this is indeed CVE fixing, then we want it in -rc4, but I don't know if the patch is correctly secure yet without answers to my questions. Especially on a CVE fix for -rc4, you want to make the reviewer's life as easy as possible by providing a commit message that includes enough details to make analysis easy.
Hope my answer help. If it is, I will add them to the commit log.
Thanks for the reviewing.
Re: [Qemu-devel] [PATCH V4 for 3.1 1/4] net: drop too large packet early
In reply to Jason WangQuoted 13 lines · Jason Wang
We try to detect and drop too large packet (>INT_MAX) in
1592a9947036("net: ignore packet size greater thanINT_MAX") during packet delivering. Unfortunately, this is not sufficient as we may hit another integer overflow when trying to queue such large packet inqemu_net_queue_append_iov():- size of the allocation may overflow on 32bit
-packet->sizeis integer which may overflow even on 64bitFixing this by move the check to
qemu_sendv_packet_async()which is the entrance of all networking codes and reduce the limit toNET_BUFSIZEto be more conservative.
Quoted 8 lines
Cc Li Qiang <[email protected]>Reported-by Li Qiang <[email protected]>Reviewed-by Li Qiang <[email protected]>Signed-off-by Jason Wang <[email protected]>--- net/net.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
Since this is a critical patch for rc4, here's a verbose review...
Quoted 9 lines
net/net.c +0
-1@@ -712,15 +712,11 @@ ssize_t qemu_deliver_packet_iov(NetClientState *sender, void *opaque) {NetClientState *nc = opaque; -size_t size =iov_size (iov, iovcnt);
net/net.c ssize_t qemu_deliver_packet_iov(NetClientState *sender, · line 720
Quoted 10 lines
net/net.c +1
-4int ret; -if (size >INT_MAX ) { -return size; - }if (nc->link_down) { -return size; +return iov_size (iov, iovcnt); }
In case you respin this patch again, please make qemu_deliver_packet_iov() "static", so that it is clear that it can not be called directly from the outside anymore. And in case there is no need to respin, please consider to send a separate patch for 4.0 instead.
Ok, thinking now load about the call chain:
Anyway, qemu_deliver_packet_iov is not directly called from any other file currently, so this is ok ...
So let's see how it is used in net.c ... it's only used as paramter
here: qemu_new_net_queue(qemu_deliver_packet_iov, nc) ... qemu_new_net_queue() assigns it to NetQueue->deliver which is only used within queue.c.
Functions using that ->deliver function pointer are the static functions qemu_net_queue_deliver() and qemu_net_queue_deliver_iov(). First one only uses one iov, so I don't think we can overflow the size here. Second one is used in turn are used by the public function qemu_net_queue_send_iov(). This has two callers, one in net.c in qemu_sendv_packet_async() which you guard below ==> OK.
The other caller is in qemu_netfilter_pass_to_next() in filter.c - and this function is called from many more other places ... but as far as I can see, these don't call it in a way where the size could overflow.
==> Removing the check in qemu_deliver_packet_iov() sounds ok to me, if it is checked in qemu_sendv_packet_async() instead.
net/net.c ssize_t qemu_sendv_packet_async(NetClientState *sender, · line 749
Quoted 11 lines
net/net.c +4
-0if (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; + }
It's a little bit unfortunate that the unsigned size will be cast to ssize_t, so a very large size could suddenly change sign. But as Eric already wrote in his mail, it seems like the callers are either ignoring the return value, or just checking for != 0, so it should be ok for now.
To be more consistent, maybe it would be better to always return an negative error code here instead?
net/net.c ssize_t qemu_sendv_packet_async(NetClientState *sender, · line 755
Quoted 7 lines
net/net.c +1
-1if (sender->link_down || !sender->peer) { -return iov_size (iov, iovcnt); +return size; }/* Let filters handle the packet first */
I think I'm fine if this patch is applied in its current shape for rc4, so:
... but please consider some follow-up patches to make qemu_deliver_packet_iov() static, and maybe to return an error code in qemu_sendv_packet_async() instead.
Re: [Qemu-devel] [PATCH V4 for 3.1 1/4] net: drop too large packet early
Quoted 39 lines · Thomas Huth
Quoted 13 lines · Jason Wang
We try to detect and drop too large packet (>INT_MAX) in
1592a9947036("net: ignore packet size greater thanINT_MAX") during packet delivering. Unfortunately, this is not sufficient as we may hit another integer overflow when trying to queue such large packet inqemu_net_queue_append_iov():- size of the allocation may overflow on 32bit
-packet->sizeis integer which may overflow even on 64bitFixing this by move the check to
qemu_sendv_packet_async()which is the entrance of all networking codes and reduce the limit toNET_BUFSIZEto be more conservative.Quoted 8 lines
Cc Li Qiang <[email protected]>Reported-by Li Qiang <[email protected]>Reviewed-by Li Qiang <[email protected]>Signed-off-by Jason Wang <[email protected]>--- net/net.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)Since this is a critical patch for rc4, here's a verbose review...
Quoted 15 lines
net/net.c +0
-4@@ -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; - }
Quoted 8 lines
net/net.c ssize_t qemu_deliver_packet_iov(NetClientState *sender, · line 720
Quoted 4 lines
net/net.c +1
-1if (nc->link_down) { -return size; +return iov_size (iov, iovcnt); }In case you respin this patch again, please make
qemu_deliver_packet_iov()"static", so that it is clear that it can not be called directly from the outside anymore. And in case there is no need to respin, please consider to send a separate patch for 4.0 instead.
Yes, will try to make it for the next version.
Quoted 21 lines
Ok, thinking now load about the call chain:
Anyway,
qemu_deliver_packet_iovis not directly called from any other file currently, so this is ok ...So let's see how it is used in
net.c... it's only used as paramter
here:qemu_new_net_queue(qemu_deliver_packet_iov, nc)...qemu_new_net_queue()assigns it toNetQueue->deliverwhich is only used withinqueue.c.Functions using that ->deliver function pointer are the static functions
qemu_net_queue_deliver()andqemu_net_queue_deliver_iov(). First one only uses one iov, so I don't think we can overflow the size here. Second one is used in turn are used by the public functionqemu_net_queue_send_iov(). This has two callers, one innet.cinqemu_sendv_packet_async()which you guard below ==> OK.
The other caller is inqemu_netfilter_pass_to_next()infilter.c- and this function is called from many more other places ... but as far as I can see, these don't call it in a way where the size could overflow.
Quoted 2 lines
==> Removing the check in
qemu_deliver_packet_iov()sounds ok to me, if it is checked inqemu_sendv_packet_async()instead.
Yes. Let me add more in the commit message.
Quoted 17 lines
net/net.c ssize_t qemu_sendv_packet_async(NetClientState *sender, · line 749
Quoted 12 lines
net/net.c +4
-0if (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; + }It's a little bit unfortunate that the unsigned size will be cast to
ssize_t, so a very large size could suddenly change sign. But as Eric already wrote in his mail, it seems like the callers are either ignoring the return value, or just checking for != 0, so it should be ok for now.
Quoted 2 lines
To be more consistent, maybe it would be better to always return an negative error code here instead?
Yes, and we can do it for 4.0.
Quoted 8 lines
net/net.c ssize_t qemu_sendv_packet_async(NetClientState *sender, · line 755
Quoted 8 lines
net/net.c +1
-1if (sender->link_down || !sender->peer) { -return iov_size (iov, iovcnt); +return size; }/* Let filters handle the packet first */
Quoted 7 lines
I think I'm fine if this patch is applied in its current shape for rc4, so:
Reviewed-by Thomas Huth <[email protected]>... but please consider some follow-up patches to make
qemu_deliver_packet_iov()static, and maybe to return an error code inqemu_sendv_packet_async()instead.
Yes.
Thanks for the reviewing.
PATCH v4 2/4 virtio-net-test: accept variable length argument in pci_test_start()
In reply to Jason WangThis 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
@@ -52,17 +52,21 @@ static QVirtioPCIDevice *virtio_net_pci_init(QPCIBus *bus, int slot)return dev; } -static QOSState *pci_test_start (int socket) +GCC_FMT_ATTR(1, 2) +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 );
@@ -223,7 +227,8 @@ static void pci_basic(gconstpointer data) ret =socketpair (PF_UNIX ,SOCK_STREAM ,0 , sv); g_assert_cmpint(ret, !=, -1); - qs =pci_test_start (sv[1 ]); + qs =pci_test_start ("-netdev socket,fd=%d,id=hs0 -device " +"virtio-net-pci,netdev=hs0" , 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
Re: [Qemu-devel] [PATCH V4 for 3.1 2/4] virtio-net-test: accept variable length argument in pci_test_start()
Quoted 8 lines · Jason Wang
This allows flexibility to be reused for all kinds of command line used by other tests.
Signed-off-by Jason Wang <[email protected]>--- tests/virtio-net-test.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH V4 for 3.1 2/4] virtio-net-test: accept variable length argument in pci_test_start()
In reply to Jason WangQuoted 7 lines · Jason Wang
This allows flexibility to be reused for all kinds of command line used by other tests.
Signed-off-by Jason Wang <[email protected]>--- tests/virtio-net-test.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)
PATCH v4 3/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
@@ -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
Re: [Qemu-devel] [PATCH V4 for 3.1 3/4] virtio-net-test: remove unused macro
Quoted 6 lines · Jason Wang
Reviewed-by Thomas Huth <[email protected]>Signed-off-by Jason Wang <[email protected]>--- tests/virtio-net-test.c | 1 - 1 file changed, 1 deletion(-)
Quoted 13 lines
tests/virtio-net-test.c +0
-1@@ -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 · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
PATCH v4 4/4 virtio-net-test: add large tx buffer test
In reply to Jason WangThis 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 | 44 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)
tests/virtio-net-test.c +44 -0
@@ -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);
Signature · 1 line
2.17.1
Re: [Qemu-devel] [PATCH V4 for 3.1 4/4] virtio-net-test: add large tx buffer test
Quoted 3 lines · Jason Wang
This test tries to build a packet whose size is greater than
INT_MAXwhich tries to trigger integer overflow inqemu_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?
Quoted 16 lines
Signed-off-by Jason Wang <[email protected]>--- tests/virtio-net-test.c | 44 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)+++
b/tests/virtio-net-test.ctests/virtio-net-test.c +4
-0@@ -245,6 +245,49 @@ static void pci_basic(gconstpointer data)g_free (dev);qtest_shutdown (qs); } + +static void large_tx (gconstpointer data) +{ +QVirtioPCIDevice *dev;
tests/virtio-net-test.c static void pci_basic(gconstpointer data) · line 261
Quoted 10 lines
tests/virtio-net-test.c +10
-0+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?
Quoted 6 lines
tests/virtio-net-test.c +6
-0+ 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);
tests/virtio-net-test.c static void pci_basic(gconstpointer data) · line 277
Quoted 10 lines
tests/virtio-net-test.c +10
-0+ 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'.
Quoted 17 lines
tests/virtio-net-test.c +13
-0+ } +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 )
tests/virtio-net-test.c int main(int argc, char **argv) · line 319
Quoted 7 lines
tests/virtio-net-test.c +1
-0@@ -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 pipetests/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:
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH V4 for 3.1 4/4] virtio-net-test: add large tx buffer test
Quoted 6 lines · Eric Blake
Quoted 3 lines · Jason Wang
This test tries to build a packet whose size is greater than
INT_MAXwhich tries to trigger integer overflow inqemu_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.
Quoted 21 lines
Quoted 21 lines
Signed-off-by Jason Wang <[email protected]>---
tests/virtio-net-test.c| 44 +++++++++++++++++++++++++++++++++++++++++1 file changed, 44 insertions(+)+++
b/tests/virtio-net-test.ctests/virtio-net-test.c +8
-0@@ -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;
Quoted 8 lines
tests/virtio-net-test.c static void pci_basic(gconstpointer data) · line 261
Quoted 6 lines
tests/virtio-net-test.c +6
-0+ 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.
Quoted 12 lines
Quoted 12 lines
tests/virtio-net-test.c +9
-0+ 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 asingle
+ * smaller area */
Quoted 9 lines
Quoted 6 lines
+
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.
Quoted 17 lines
Quoted 17 lines
+ }
+qvirtqueue_kick(&dev->vdev, vq, free_head);tests/virtio-net-test.c +11
-0+ + 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)tests/virtio-net-test.c +0
-0@@ -270,6 +313,7 @@ int main(int argc, char **argv) qtest_add_data_func("/virtio/net/pci/basic", send_recv_test, pci_basic);
Quoted 8 lines
tests/virtio-net-test.c int main(int argc, char **argv) · line 318
Quoted 5 lines
tests/virtio-net-test.c +1
-0qtest_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.
Quoted 11 lines
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: R02S9569aabcb2d834a9dadcce272c5588dbso 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 <[email protected]>
Thanks
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 thanINT_MAXwhich may lead integer overflow. We've fixed similar issue in the past duringqemu_net_queue_deliver_iov()by ignoring large packets there. Let's just move the check earlier toqemu_sendv_packet_async()and reduce the limitation toNET_BUFSIZE. A simple qtest were also added this.
Quoted 1 line
Please review.
I did a test build and run, and the new test generates warning messages during "make check":
/ppc64/virtio/net/pci/large_tx:
qemu-system-ppc64: warning: hub 0 is not connected to host network (similarly for /i386/ and /x86_64/).
thank
-- PMM
Quoted 12 lines · Peter Maydell
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 thanINT_MAXwhich may lead integer overflow. We've fixed similar issue in the past duringqemu_net_queue_deliver_iov()by ignoring large packets there. Let's just move the check earlier toqemu_sendv_packet_async()and reduce the limitation toNET_BUFSIZE. A simple qtest were also added this.Please review.
Quoted 8 lines
I did a test build and run, and the new test generates warning messages during "make check":
/ppc64/virtio/net/pci/large_tx:qemu-system-ppc64: warning: hub 0 is not connected to host network (similarly for /i386/ and /x86_64/).
thank
-- PMM
This is intended, we don't need any host networking device. This makes it very simpler and easier to trigger the queuing at hub. I can add a patch to suppress this warning if qtest is enabled.
Thanks