Re: [PATCH] KVM: X86: Fix residual mmio emulation request to userspace

[Jim Mattson]

I'm not convinced that this plugs all of the mmio_needed/pio.count
leaks, but it's a start. Syzkaller will eventually let us know about
the others.

On Thu, Aug 10, 2017 at 7:23 AM, Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
> On 10/08/2017 16:09, Dmitry Vyukov wrote:
>> On Thu, Aug 10, 2017 at 3:44 PM, Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
>>> On 10/08/2017 07:33, Wanpeng Li wrote:
>>>> Reported by syzkaller:
>>>>
>>>> The kvm-intel.unrestricted_guest=0
>>>>
>>>>    WARNING: CPU: 5 PID: 1014 at /home/kernel/data/kvm/arch/x86/kvm//x86.c:7227 kvm_arch_vcpu_ioctl_run+0x38b/0x1be0 [kvm]
>>>>    CPU: 5 PID: 1014 Comm: warn_test Tainted: G        W  OE   4.13.0-rc3+ #8
>>>>    RIP: 0010:kvm_arch_vcpu_ioctl_run+0x38b/0x1be0 [kvm]
>>>>    Call Trace:
>>>>     ? put_pid+0x3a/0x50
>>>>     ? rcu_read_lock_sched_held+0x79/0x80
>>>>     ? kmem_cache_free+0x2f2/0x350
>>>>     kvm_vcpu_ioctl+0x340/0x700 [kvm]
>>>>     ? kvm_vcpu_ioctl+0x340/0x700 [kvm]
>>>>     ? __fget+0xfc/0x210
>>>>     do_vfs_ioctl+0xa4/0x6a0
>>>>     ? __fget+0x11d/0x210
>>>>     SyS_ioctl+0x79/0x90
>>>>     entry_SYSCALL_64_fastpath+0x23/0xc2
>>>>     ? __this_cpu_preempt_check+0x13/0x20
>>>>
>>>> The syszkaller folks reported a residual mmio emulation request to userspace
>>>> due to vm86 fails to emulate inject real mode interrupt(fails to read CS) and
>>>> incurs a triple fault. The vCPU returns to userspace with vcpu->mmio_needed == true
>>>> and KVM_EXIT_SHUTDOWN exit reason. However, the syszkaller testcase constructs
>>>> several threads to launch the same vCPU, the thread which lauch this vCPU after
>>>> the thread whichs get the vcpu->mmio_needed == true and KVM_EXIT_SHUTDOWN will
>>>> trigger the warning.
>>>>
>>>>    #define _GNU_SOURCE
>>>>    #include <pthread.h>
>>>>    #include <stdio.h>
>>>>    #include <stdlib.h>
>>>>    #include <string.h>
>>>>    #include <sys/wait.h>
>>>>    #include <sys/types.h>
>>>>    #include <sys/stat.h>
>>>>    #include <sys/mman.h>
>>>>    #include <fcntl.h>
>>>>    #include <unistd.h>
>>>>    #include <linux/kvm.h>
>>>>    #include <stdio.h>
>>>>
>>>>    int kvmcpu;
>>>>    struct kvm_run *run;
>>>>
>>>>    void* thr(void* arg)
>>>>    {
>>>>      int res;
>>>>      res = ioctl(kvmcpu, KVM_RUN, 0);
>>>>      printf("ret1=%d exit_reason=%d suberror=%d\n",
>>>>          res, run->exit_reason, run->internal.suberror);
>>>>      return 0;
>>>>    }
>>>>
>>>>    void test()
>>>>    {
>>>>      int i, kvm, kvmvm;
>>>>      pthread_t th[4];
>>>>
>>>>      kvm = open("/dev/kvm", O_RDWR);
>>>>      kvmvm = ioctl(kvm, KVM_CREATE_VM, 0);
>>>>      kvmcpu = ioctl(kvmvm, KVM_CREATE_VCPU, 0);
>>>>      run = (struct kvm_run*)mmap(0, 4096, PROT_READ|PROT_WRITE, MAP_SHARED, kvmcpu, 0);
>>>>      srand(getpid());
>>>>      for (i = 0; i < 4; i++) {
>>>>        pthread_create(&th[i], 0, thr, 0);
>>>>        usleep(rand() % 10000);
>>>>      }
>>>>      for (i = 0; i < 4; i++)
>>>>        pthread_join(th[i], 0);
>>>>    }
>>>>
>>>>    int main()
>>>>    {
>>>>      for (;;) {
>>>>        int pid = fork();
>>>>        if (pid < 0)
>>>>          exit(1);
>>>>        if (pid == 0) {
>>>>          test();
>>>>          exit(0);
>>>>        }
>>>>        int status;
>>>>        while (waitpid(pid, &status, __WALL) != pid) {}
>>>>      }
>>>>      return 0;
>>>>    }
>>>>
>>>> This patch fixes it by resetting the vcpu->mmio_needed once we receive
>>>> the triple fault to avoid the residue.
>>>>
>>>> Reported-by: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
>>>> Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
>>>> Cc: Radim KrÄmÃÅ <rkrcmar@xxxxxxxxxx>
>>>> Cc: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
>>>> Signed-off-by: Wanpeng Li <wanpeng.li@xxxxxxxxxxx>
>>>> ---
>>>>  arch/x86/kvm/vmx.c | 1 +
>>>>  arch/x86/kvm/x86.c | 1 +
>>>>  2 files changed, 2 insertions(+)
>>>>
>>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>>> index 8e4a2dc..77ab10b 100644
>>>> --- a/arch/x86/kvm/vmx.c
>>>> +++ b/arch/x86/kvm/vmx.c
>>>> @@ -5864,6 +5864,7 @@ static int handle_external_interrupt(struct kvm_vcpu *vcpu)
>>>>  static int handle_triple_fault(struct kvm_vcpu *vcpu)
>>>>  {
>>>>       vcpu->run->exit_reason = KVM_EXIT_SHUTDOWN;
>>>> +     vcpu->mmio_needed = 0;
>>>>       return 0;
>>>>  }
>>>>
>>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>>> index 72d82ab..1e143f7 100644
>>>> --- a/arch/x86/kvm/x86.c
>>>> +++ b/arch/x86/kvm/x86.c
>>>> @@ -6776,6 +6776,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>>>>               }
>>>>               if (kvm_check_request(KVM_REQ_TRIPLE_FAULT, vcpu)) {
>>>>                       vcpu->run->exit_reason = KVM_EXIT_SHUTDOWN;
>>>> +                     vcpu->mmio_needed = 0;
>>>>                       r = 0;
>>>>                       goto out;
>>>>               }
>>>>
>>>
>>>
>>> Queued, thanks.
>>
>> Hi Paolo,
>>
>> Where is it queued? I've checked
>> git://git.kernel.org/pub/scm/virt/kvm/kvm.git
>> {next,master,fixes,queue} and can't find it.
>
> I do run some tests before pushing. :)  Sometimes I don't have time the
> same day, so I just run kvm-unit-tests and push to the queue branch,
> which is rebased.  Sometimes I do, and it takes a few hours before it
> ends up with a definitive SHA1 commit hash on the master and next branches.
>
> Today it's the latter, so you'll find it in a couple hours if everything
> goes according to the plan.
>
> Paolo
>