From mboxrd@z Thu Jan 1 00:00:00 1970 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) X-AuditID: cbfec7f5-f792a6d000001302-2f-574c310856bd Received: from eusync1.samsung.com ( [203.254.199.211]) by eucpsbgm2.samsung.com (EUCPMTA) with SMTP id 5A.5A.04866.8013C475; Mon, 30 May 2016 13:24:40 +0100 (BST) Received: from [106.109.129.180] by eusync1.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTPA id <0O7Z00M62P53MK90@eusync1.samsung.com>; Mon, 30 May 2016 13:24:40 +0100 (BST) To: "Tan, Jianfeng" , "dev@dpdk.org" , "Xie, Huawei" , Yuanhan Liu References: <1463748604-27251-1-git-send-email-i.maximets@samsung.com> Cc: Dyasly Sergey , Heetae Ahn From: Ilya Maximets Message-id: <574C3107.1070305@samsung.com> Date: Mon, 30 May 2016 15:24:39 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 MIME-version: 1.0 In-reply-to: Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrGLMWRmVeSWpSXmKPExsVy+t/xy7ochj7hBq/OCli8+7SdyWLa59vs Fu0zzzJZXGn/yW7RPfsLm8Xk2VIW1ydcYHVg9/i1YCmrx+I9L5k85p0M9OjbsooxgCWKyyYl NSezLLVI3y6BK2PK2jamgj2yFS1/X7E3MM4W72Lk5JAQMJHYufQSK4QtJnHh3nq2LkYuDiGB pYwSbW9XM0I4Lxgl/lw5zwZSJSzgLDHhbB9YQkRgHlDV821QVRMYJaZc2sAIUsUs4C/x5cYG ZhCbTUBH4tTqI2BxXgEtiYaHM5lAbBYBVYn9x6ewgNiiAhESs7b/YIKoEZT4MfkeWJxTIEzi +MU9QDYH0Ew9ifsXtSDGy0tsXvOWeQKjwCwkHbMQqmYhqVrAyLyKUTS1NLmgOCk910ivODG3 uDQvXS85P3cTIyS0v+5gXHrM6hCjAAejEg9vgaZ3uBBrYllxZe4hRgkOZiUR3lJ9n3Ah3pTE yqrUovz4otKc1OJDjNIcLErivDN3vQ8REkhPLEnNTk0tSC2CyTJxcEo1MFao64ozbZVV+M// Ym+GuMvRBg4h14MauoZ/TB/sb5yVYPxxx7wFqk2fUme1vmdiP/GS5aDoPvtp0hc7M9Xvf9Vb 5vhH6ujiSb5/UgKS7qn9PPkkburmT+eiZHkdFT3VxZeU2q9aJzeBI3Yeo3VUPqdJhlD71Vu7 ApY8VHv49ml09Zw37zdPOKnEUpyRaKjFXFScCACHLjDNaQIAAA== Subject: Re: [dpdk-dev] [PATCH] vhost: fix segfault on bad descriptor address. X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 30 May 2016 12:24:42 -0000 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.