Re: sanitizing kvmtool

[Sasha Levin <sasha.levin@oracle.com>]

On 10/19/2015 11:15 AM, Dmitry Vyukov wrote:
> On Mon, Oct 19, 2015 at 5:08 PM, Sasha Levin <sasha.levin@oracle.com> 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?


Thanks,
Sasha