Fix possible OOB during queuing packets
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
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
@@ -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
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
@@ -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
PATCH v2 3/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
@@ -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
PATCH v2 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 | 45 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+)
tests/virtio-net-test.c +45 -0
@@ -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
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_MAXwhich tries to trigger integer overflow inqemu_net_queue_append_iov()which may result OOB.Signed-off-by Jason Wang <[email protected]>--- tests/virtio-net-test.c | 45 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+)
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.
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
+-- 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
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
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 :|