From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ilya Maximets Subject: Re: [PATCH] vhost: fix segfault on bad descriptor address. Date: Mon, 30 May 2016 15:24:39 +0300 Message-ID: <574C3107.1070305@samsung.com> References: <1463748604-27251-1-git-send-email-i.maximets@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Cc: Dyasly Sergey , Heetae Ahn To: "Tan, Jianfeng" , "dev@dpdk.org" , "Xie, Huawei" , Yuanhan Liu Return-path: Received: from mailout4.w1.samsung.com (mailout4.w1.samsung.com [210.118.77.14]) by dpdk.org (Postfix) with ESMTP id AC6AC2C45 for ; Mon, 30 May 2016 14:24:42 +0200 (CEST) Received: from eucpsbgm2.samsung.com (unknown [203.254.199.245]) by mailout4.w1.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTP id <0O7Z00L4RP54SV20@mailout4.w1.samsung.com> for dev@dpdk.org; Mon, 30 May 2016 13:24:40 +0100 (BST) In-reply-to: List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 30.05.2016 15:00, Tan, Jianfeng wrote: > Hi, > >> -----Original Message----- >> From: Ilya Maximets [mailto:i.maximets@samsung.com] >> Sent: Friday, May 20, 2016 8:50 PM >> To: dev@dpdk.org; Xie, Huawei; Yuanhan Liu >> Cc: Dyasly Sergey; Heetae Ahn; Tan, Jianfeng; Ilya Maximets >> Subject: [PATCH] vhost: fix segfault on bad descriptor address. >> >> In current implementation guest application can reinitialize vrings >> by executing start after stop. In the same time host application >> can still poll virtqueue while device stopped in guest and it will >> crash with segmentation fault while vring reinitialization because >> of dereferencing of bad descriptor addresses. >> >> OVS crash for example: >> <------------------------------------------------------------------------> >> [test-pmd inside guest VM] >> >> testpmd> port stop all >> Stopping ports... >> Checking link statuses... >> Port 0 Link Up - speed 10000 Mbps - full-duplex >> Done >> testpmd> port config all rxq 2 >> testpmd> port config all txq 2 >> testpmd> port start all >> Configuring Port 0 (socket 0) >> Port 0: 52:54:00:CB:44:C8 >> Checking link statuses... >> Port 0 Link Up - speed 10000 Mbps - full-duplex >> Done >> >> [OVS on host] >> Program received signal SIGSEGV, Segmentation fault. >> rte_memcpy (n=2056, src=0xc, dst=0x7ff4d5247000) at >> rte_memcpy.h >> >> (gdb) bt >> #0 rte_memcpy (n=2056, src=0xc, dst=0x7ff4d5247000) >> #1 copy_desc_to_mbuf >> #2 rte_vhost_dequeue_burst >> #3 netdev_dpdk_vhost_rxq_recv >> ... >> >> (gdb) bt full >> #0 rte_memcpy >> ... >> #1 copy_desc_to_mbuf >> desc_addr = 0 >> mbuf_offset = 0 >> desc_offset = 12 >> ... >> <------------------------------------------------------------------------> >> >> Fix that by checking addresses of descriptors before using them. >> >> Note: For mergeable buffers this patch checks only guest's address for >> zero, but in non-meargeable case host's address checked. This is done >> because checking of host's address in mergeable case requires additional >> refactoring to keep virtqueue in consistent state in case of error. > > > I agree with you that it should be fixed because malicious guest could launch > DOS attack on vswitch with the current implementation. > > But I don't understand why you do not fix the mergable case in > copy_mbuf_to_desc_mergable() on where gpa_to_vva() happens? And the change in > fill_vec_buf(), checking !vq->desc[idx].addr, make any sense? > > Thanks, > Jianfeng Hi. As I said inside commit-message, checking of host's address in mergeable case requires additional refactoring to keep virtqueue in consistent state. There are few issues with checking inside copy_mbuf_to_desc_mergable() : 1. Ring elements already reserved and we must fill them with some sane data before going out of virtio_dev_merge_rx(). 2. copy_mbuf_to_desc_mergable() can't return an error in current implementation (additional checking needed), otherwise used->idx will be decremented (I think, it's bad). Checking !vq->desc[idx].addr inside fill_vec_buf() make sense in case of virtio reinitialization, because guest's address will be zero (case described in commit-message). Checking of guest's address will not help in case of bad and not NULL address, but useful in this common case. Also, we can't catch bad address what we able to map, so, malicious guest could break vhost anyway. I agree, that checking of host's address is better, but this may be done later together with resolving above issues. Best regards, Ilya Maximets.