From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 9C4BE5A84 for ; Tue, 31 May 2016 08:53:06 +0200 (CEST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga102.fm.intel.com with ESMTP; 30 May 2016 23:53:05 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,394,1459839600"; d="scan'208";a="965573485" Received: from shwdeisgchi083.ccr.corp.intel.com (HELO [10.239.67.193]) ([10.239.67.193]) by orsmga001.jf.intel.com with ESMTP; 30 May 2016 23:53:04 -0700 To: Ilya Maximets , "dev@dpdk.org" , "Xie, Huawei" , Yuanhan Liu References: <1463748604-27251-1-git-send-email-i.maximets@samsung.com> <574C3107.1070305@samsung.com> Cc: Dyasly Sergey , Heetae Ahn From: "Tan, Jianfeng" Message-ID: <1bb65424-fda0-8e60-554d-66ccc19e7d90@intel.com> Date: Tue, 31 May 2016 14:53:03 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.1.0 MIME-Version: 1.0 In-Reply-To: <574C3107.1070305@samsung.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit 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 06:53:07 -0000 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? 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.