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
