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 at samsung.com] >>> Sent: Friday, May 20, 2016 8:50 PM >>> To: dev at 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.