linux-kernel - Re: kvm: WARNING in kvm_arch_vcpu_ioctl

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 9 Aug 2017 22:35:53 +0200
From:   Dmitry Vyukov <dvyukov@...gle.com>
To:     David Hildenbrand <david@...hat.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Radim Krčmář <rkrcmar@...hat.com>,
        KVM list <kvm@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>, P J P <ppandit@...hat.com>,
        James Mattson <jmattson@...gle.com>,
        Xiao Guangrong <guangrong.xiao@...ux.intel.com>,
        Haozhong Zhang <haozhong.zhang@...el.com>,
        Wanpeng Li <kernellwp@...il.com>,
        Steve Rutherford <srutherford@...gle.com>,
        syzkaller <syzkaller@...glegroups.com>
Subject: Re: kvm: WARNING in kvm_arch_vcpu_ioctl_run

On Wed, Aug 9, 2017 at 10:24 PM, David Hildenbrand <david@...hat.com> wrote:
> On 09.08.2017 19:07, Dmitry Vyukov wrote:
>> Hello,
>>
>> syzkaller fuzzer has hit the following WARNING in kvm_arch_vcpu_ioctl_run.
>> This is easily reproducible and reproducer is attached at the bottom.
>> The report is on upstream commit
>> 26c5cebfdb6ca799186f1e56be7d6f2480c5012c. This requires setting
>> kvm-intel.unrestricted_guest=0 on the machine, with
>> unrestricted_guest=1 the WARNING does not happen. Output of the
>> program is:
>>
>> ret1=0 exit_reason=17 suberror=1
>> ret2=0 exit_reason=8 suberror=65530
>>
>>
>
> I wonder if the following is possible:
>
> 1) we set vcpu->arch.pio.count != 0
>
> 2) we set vcpu->arch.complete_userspace_io = complete_emulated_pio;
> (in x86_emulate_instruction)
>
> 3) in kvm_arch_vcpu_ioctl_run(), we execute
> vcpu->arch.complete_userspace_io and set it to NULL.
>
> 4. complete_emulated_pio() calls complete_emulated_io(), which fails
> (in some different way) and leaves vcpu->arch.pio.count set.
>
> 5. we now have vcpu->arch.pio.count set but
> vcpu->arch.complete_userspace_io cleared.
>
> Same should not apply to vcpu->mmio_needed, as we clear it before
> calling complete_emulated_io() in complete_emulated_mmio().
>
> AFAIK, we cannot simply clear vcpu->arch.pio.count before calling
> complete_emulated_io().
>
> Maybe something like this could help: (untested, wild guess)
>
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 86242ee..60296fef 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -7227,10 +7227,14 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>                 int (*cui)(struct kvm_vcpu *) = vcpu->arch.complete_userspace_io;
>                 vcpu->arch.complete_userspace_io = NULL;
>                 r = cui(vcpu);
> +               if (!vcpu->arch.complete_userspace_io) {
> +                       /* no new handler -> everything handled */
> +                       vcpu->arch.pio.count = 0;
> +                       vcpu->mmio_needed = 0;
> +               }
>                 if (r <= 0)
>                         goto out;
> -       } else
> -               WARN_ON(vcpu->arch.pio.count || vcpu->mmio_needed);
> +       }
>
>         if (kvm_run->immediate_exit)
>                 r = -EINTR;
>
>
> Alternatively, something like this
>
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 86242ee..58be388 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -7127,9 +7127,15 @@ static inline int complete_emulated_io(struct kvm_vcpu *vcpu)
>
>  static int complete_emulated_pio(struct kvm_vcpu *vcpu)
>  {
> +       int ret;
> +
>         BUG_ON(!vcpu->arch.pio.count);
>
> -       return complete_emulated_io(vcpu);
> +       ret = complete_emulated_io(vcpu);
> +       if (ret && !vcpu->arch.complete_userspace_io)
> +               vcpu->arch.pio.count = 0;
> +
> +       return ret;
>  }
>
>  /*
>
>
> Haven't tried to reproduce on upstream yet (at least on my Fedora 25
> I can't trigger it), so only wild guesses.


It is know to happen back to at least 4.3. Make sure you have
kvm-intel.unrestricted_guest=0, it does not reproduce without it (at
least not with this program).