On Thu, Oct 22, 2015 at 1:07 AM, Sasha Levin <sasha.levin@xxxxxxxxxx> wrote: > On 10/19/2015 11:15 AM, Dmitry Vyukov wrote: >> On Mon, Oct 19, 2015 at 5:08 PM, Sasha Levin <sasha.levin@xxxxxxxxxx> wrote: >>> > On 10/19/2015 10:47 AM, Dmitry Vyukov wrote: >>>>> >>> Right, the memory areas that are accessed both by the hypervisor and the guest >>>>>> >>> > should be treated as untrusted input, but the hypervisor is supposed to validate >>>>>> >>> > the input carefully before using it - so I'm not sure how data races would >>>>>> >>> > introduce anything new that we didn't catch during validation. >>>> >> >>>> >> One possibility would be: if result of a racy read is passed to guest, >>>> >> that can leak arbitrary host data into guest. Does not sound good. >>>> >> Also, without usage of proper atomic operations, it is basically >>>> >> impossible to verify untrusted data, as it can be changing under your >>>> >> feet. And storing data into a local variable does not prevent the data >>>> >> from changing. >>> > >>> > What's missing here is that the guest doesn't directly read/write the memory: >>> > every time it accesses a memory that is shared with the host it will trigger >>> > an exit, which will stop the vcpu thread that made the access and kernel side >>> > kvm will pass the hypervisor the value the guest wrote (or the memory address >>> > it attempted to read). The value/address can't change under us in that scenario. >> But still: if result of a racy read is passed to guest, that can leak >> arbitrary host data into guest. > > I see what you're saying. I need to think about it a bit, maybe we do need locking > for each of the virtio devices we emulate. > > > On an unrelated note, a few of the reports are pointing to ioport__unregister(): > > ================== > WARNING: ThreadSanitizer: data race (pid=109228) > Write of size 8 at 0x7d1c0000df40 by main thread: > #0 free tsan/rtl/tsan_interceptors.cc:570 (lkvm+0x000000443376) > #1 ioport__unregister ioport.c:138:2 (lkvm+0x0000004a9ff9) > #2 pci__exit pci.c:247:2 (lkvm+0x0000004ac857) > #3 init_list__exit util/init.c:59:8 (lkvm+0x0000004bca6e) > #4 kvm_cmd_run_exit builtin-run.c:645:2 (lkvm+0x0000004a68a7) > #5 kvm_cmd_run builtin-run.c:661 (lkvm+0x0000004a68a7) > #6 handle_command kvm-cmd.c:84:8 (lkvm+0x0000004bc40c) > #7 handle_kvm_command main.c:11:9 (lkvm+0x0000004ac0b4) > #8 main main.c:18 (lkvm+0x0000004ac0b4) > > Previous read of size 8 at 0x7d1c0000df40 by thread T55: > #0 rb_int_search_single util/rbtree-interval.c:14:17 (lkvm+0x0000004bf968) > #1 ioport_search ioport.c:41:9 (lkvm+0x0000004aa05f) > #2 kvm__emulate_io ioport.c:186 (lkvm+0x0000004aa05f) > #3 kvm_cpu__emulate_io x86/include/kvm/kvm-cpu-arch.h:41:9 (lkvm+0x0000004aa718) > #4 kvm_cpu__start kvm-cpu.c:126 (lkvm+0x0000004aa718) > #5 kvm_cpu_thread builtin-run.c:174:6 (lkvm+0x0000004a6e3e) > > Thread T55 'kvm-vcpu-2' (tid=109285, finished) created by main thread at: > #0 pthread_create tsan/rtl/tsan_interceptors.cc:848 (lkvm+0x0000004478a3) > #1 kvm_cmd_run_work builtin-run.c:633:7 (lkvm+0x0000004a683f) > #2 kvm_cmd_run builtin-run.c:660 (lkvm+0x0000004a683f) > #3 handle_command kvm-cmd.c:84:8 (lkvm+0x0000004bc40c) > #4 handle_kvm_command main.c:11:9 (lkvm+0x0000004ac0b4) > #5 main main.c:18 (lkvm+0x0000004ac0b4) > > SUMMARY: ThreadSanitizer: data race ioport.c:138:2 in ioport__unregister > ================== > > I think this is because we don't perform locking using pthread, but rather pause > the vm entirely - so the cpu threads it's pointing to aren't actually running when > we unregister ioports. Is there a way to annotate that for tsan? I've looked at brlock and I think should understand it. Reader threads write to the eventfd to notify that they are stopped and writer reads from the event fd and tsan considers this write->read as synchronization. I suspect that this can be caused by the same use-after-free on cpu array, probably kvm__pause takes fast path when it should not. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
