From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mailout3.w1.samsung.com (mailout3.w1.samsung.com [210.118.77.13]) by dpdk.org (Postfix) with ESMTP id 9F0A02BB9 for ; Tue, 31 May 2016 11:10:49 +0200 (CEST) Received: from eucpsbgm2.samsung.com (unknown [203.254.199.245]) by mailout3.w1.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTP id <0O81002UZATZXM70@mailout3.w1.samsung.com> for dev@dpdk.org; Tue, 31 May 2016 10:10:47 +0100 (BST) X-AuditID: cbfec7f5-f792a6d000001302-46-574d55176696 Received: from eusync2.samsung.com ( [203.254.199.212]) by eucpsbgm2.samsung.com (EUCPMTA) with SMTP id E7.01.04866.7155D475; Tue, 31 May 2016 10:10:47 +0100 (BST) Received: from [106.109.129.180] by eusync2.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTPA id <0O81003JKATYBFA0@eusync2.samsung.com>; Tue, 31 May 2016 10:10:47 +0100 (BST) To: "Tan, Jianfeng" , "dev@dpdk.org" , "Xie, Huawei" , Yuanhan Liu References: <1463748604-27251-1-git-send-email-i.maximets@samsung.com> <574C3107.1070305@samsung.com> <1bb65424-fda0-8e60-554d-66ccc19e7d90@intel.com> Cc: Dyasly Sergey , Heetae Ahn From: Ilya Maximets Message-id: <574D5516.3010405@samsung.com> Date: Tue, 31 May 2016 12:10:46 +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: <1bb65424-fda0-8e60-554d-66ccc19e7d90@intel.com> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrGLMWRmVeSWpSXmKPExsVy+t/xK7riob7hBk87VCzefdrOZDHt8212 i/aZZ5ksrrT/ZLfonv2FzWLybCmL6xMusDqwe/xasJTVY/Gel0we804GevRtWcUYwBLFZZOS mpNZllqkb5fAlbFy7hfWgjlqFcvvHGNtYHwk28XIySEhYCLx7/BZFghbTOLCvfVsXYxcHEIC SxklZsy5zQ7hvGCU+NCymRGkSljAWWLC2T5GkISIwDxGibbn2xghqh4xSiy9dBRsFrOAv8SX GxuYQWw2AR2JU6uPgHXzCmhJ9N6ZB1bDIqAqMf/qc7C4qECExKztP5ggagQlfky+B1bDKWAr saj3EVCcA2imnsT9i1oQ4+UlNq95yzyBUWAWko5ZCFWzkFQtYGRexSiaWppcUJyUnmukV5yY W1yal66XnJ+7iRES2l93MC49ZnWIUYCDUYmHN6LbJ1yINbGsuDL3EKMEB7OSCG+9l2+4EG9K YmVValF+fFFpTmrxIUZpDhYlcd6Zu96HCAmkJ5akZqemFqQWwWSZODilGhjj0w5ts5H7Kl7a O3mq5AbvbauyXOMPygikSdpk77rscNT54ZWr7+S7mstqVx4PENN9fd/AMzjqfmyw45n1htJy T7jOdm9RmL7l8pnZehONT3V6ahw2779z8f39juy/PdqZ67k2bFBcGdjFP+fmdtfVN5Je8D+Z 8NXCxFlTSrHTeB6HzE1dl6VKLMUZiYZazEXFiQARZx8uaQIAAA== 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: Tue, 31 May 2016 09:10:49 -0000 On 31.05.2016 09:53, Tan, Jianfeng wrote: > Hi, > > > On 5/30/2016 8:24 PM, Ilya Maximets wrote: >> 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). > > Yes, currently there is no way to return these invalid desc back to virtio because there's no invalid flag in virtio_net_hdr to indicate this desc contains no pkt. I see kernel just skips those descriptors with bad addr. I think it may rely on reset of the virtio device to improve such situation. > > Another thing is that, your patch only checks the desc->addr, but we should check desc->addr + desc->len too, right? To do it fast we need to check whole range inside gpa_to_vva(), but even more refactoring is required for that. Also, this can be a different patch because checking of addr + len not required to fix original issue with virtio reconfiguration. > > Thanks, > Jianfeng > >> >> >> 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.