[PATCH v2] KVM: VMX: Fix rflags cache during vCPU reset

[PATCH v2] KVM: VMX: Fix rflags cache during vCPU reset

[Wanpeng Li]

From: Wanpeng Li <wanpeng.li@hotmail.com>

Reported by syzkaller:

   *** Guest State ***
   CR0: actual=0x0000000080010031, shadow=0x0000000060000010, gh_mask=fffffffffffffff7
   CR4: actual=0x0000000000002061, shadow=0x0000000000000000, gh_mask=ffffffffffffe8f1
   CR3 = 0x000000002081e000
   RSP = 0x000000000000fffa  RIP = 0x0000000000000000
   RFLAGS=0x00023000         DR7 = 0x00000000000000
          ^^^^^^^^^^         
   ------------[ cut here ]------------
   WARNING: CPU: 6 PID: 24431 at /home/kernel/linux/arch/x86/kvm//x86.c:7302 kvm_arch_vcpu_ioctl_run+0x651/0x2ea0 [kvm]
   CPU: 6 PID: 24431 Comm: reprotest Tainted: G        W  OE   4.14.0+ #26
   RIP: 0010:kvm_arch_vcpu_ioctl_run+0x651/0x2ea0 [kvm]
   RSP: 0018:ffff880291d179e0 EFLAGS: 00010202
   Call Trace:
    kvm_vcpu_ioctl+0x479/0x880 [kvm]
    do_vfs_ioctl+0x142/0x9a0
    SyS_ioctl+0x74/0x80
    entry_SYSCALL_64_fastpath+0x23/0x9a

The BUG is triggered while doing:

*(uint32_t*)0x20afd000 = (uint32_t)0xf0403;
*(uint32_t*)0x20afd004 = (uint32_t)0x0;
*(uint64_t*)0x20afd008 = (uint64_t)0x65;
*(uint64_t*)0x20afd010 = (uint64_t)0x5;
*(uint64_t*)0x20afd018 = (uint64_t)0xfffffffffffffffe;
*(uint64_t*)0x20afd020 = (uint64_t)0xfff;
*(uint64_t*)0x20afd038 = (uint64_t)0x2;
*(uint64_t*)0x20afd040 = (uint64_t)0x2;
r[15] = syscall(__NR_ioctl, r[4], 0x4048ae9bul, 0x20afd000ul);

i.e. KVM_SET_GUEST_DEBUG ioctl with

struct kvm_guest_debug {
    .control = 0xf0403,
    .pad = 0,
    .arch = {
        .debugreg[0] = 0x65,
        .debugreg[1] = 0x5,
        .debugreg[2] = 0xfffffffffffffffe,
        .debugreg[3] = 0xfff,
        .debugreg[6] = 0x2,
        .debugreg[7] = 0x2 
    }

The syzkaller testcase tries to setup the processor specific debug registers 
and configure vCPU for handling guest debug events through KVM_SET_GUEST_DEBUG.
The KVM_SET_GUEST_DEBUG ioctl will get and set rflags in order to set TF bit 
if single step is needed. All regs' caches are reset to avail and GUEST_RFLAGS 
vmcs field is reset to 0x2 during vCPU reset. However, the cache of rflags is 
not reset during vCPU reset. The function vmx_get_rflags() returns an unreset 
rflags cache value since the cache is marked avail, it is 0 after boot. Vmentry 
fails if the rflags reserved bit 1 is 0.

This patch fixes it by resetting both the GUEST_RFLAGS vmcs field and its cache 
to 0x2 during vCPU reset.

Reported-by: Dmitry Vyukov <dvyukov@google.com>
Tested-by: Dmitry Vyukov <dvyukov@google.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Nadav Amit <nadav.amit@gmail.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
 arch/x86/kvm/vmx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index b348920..131fa1c 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -5590,6 +5590,7 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 		vmcs_write64(GUEST_IA32_DEBUGCTL, 0);
 	}
 
+	kvm_set_rflags(vcpu, 2);
 	vmcs_writel(GUEST_RFLAGS, 0x02);
 	kvm_rip_write(vcpu, 0xfff0);
 
-- 
2.7.4

Re: [PATCH v2] KVM: VMX: Fix rflags cache during vCPU reset

[Paolo Bonzini]

On 20/11/2017 23:52, Wanpeng Li wrote:
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index b348920..131fa1c 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -5590,6 +5590,7 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>  		vmcs_write64(GUEST_IA32_DEBUGCTL, 0);
>  	}
>  
> +	kvm_set_rflags(vcpu, 2);
>  	vmcs_writel(GUEST_RFLAGS, 0x02);

I think the vmcs_writel can go, kvm_set_rflags ends up calling
vmx_set_rflags.

>  	kvm_rip_write(vcpu, 0xfff0);
>  
> 

Beautified testcase:

    #include <unistd.h>
    #include <sys/syscall.h>
    #include <string.h>
    #include <stdint.h>
    #include <linux/kvm.h>
    #include <fcntl.h>
    #include <sys/ioctl.h>

    long r[5];
    int main()
    {
        struct kvm_debugregs dr = { 0 };

        r[2] = open("/dev/kvm", O_RDONLY);
        r[3] = ioctl(r[2], KVM_CREATE_VM, 0);
        r[4] = ioctl(r[3], KVM_CREATE_VCPU, 7);
        struct kvm_guest_debug debug = {
                .control = 0xf0403,
                .arch = {
                        .debugreg[6] = 0x2,
                        .debugreg[7] = 0x2
                }
        };
        ioctl(r[4], KVM_SET_GUEST_DEBUG, &debug);
        ioctl(r[4], KVM_RUN, 0);
    }

No need to do anything, I'll handle this patch after -rc1.

Paolo

Re: [PATCH v2] KVM: VMX: Fix rflags cache during vCPU reset

[Wanpeng Li]

2017-11-21 7:09 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
> On 20/11/2017 23:52, Wanpeng Li wrote:
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index b348920..131fa1c 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -5590,6 +5590,7 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>>               vmcs_write64(GUEST_IA32_DEBUGCTL, 0);
>>       }
>>
>> +     kvm_set_rflags(vcpu, 2);
>>       vmcs_writel(GUEST_RFLAGS, 0x02);
>
> I think the vmcs_writel can go, kvm_set_rflags ends up calling
> vmx_set_rflags.

Agreed.

>
>>       kvm_rip_write(vcpu, 0xfff0);
>>
>>
>
> Beautified testcase:
>
>     #include <unistd.h>
>     #include <sys/syscall.h>
>     #include <string.h>
>     #include <stdint.h>
>     #include <linux/kvm.h>
>     #include <fcntl.h>
>     #include <sys/ioctl.h>
>
>     long r[5];
>     int main()
>     {
>         struct kvm_debugregs dr = { 0 };
>
>         r[2] = open("/dev/kvm", O_RDONLY);
>         r[3] = ioctl(r[2], KVM_CREATE_VM, 0);
>         r[4] = ioctl(r[3], KVM_CREATE_VCPU, 7);
>         struct kvm_guest_debug debug = {
>                 .control = 0xf0403,
>                 .arch = {
>                         .debugreg[6] = 0x2,
>                         .debugreg[7] = 0x2
>                 }
>         };
>         ioctl(r[4], KVM_SET_GUEST_DEBUG, &debug);
>         ioctl(r[4], KVM_RUN, 0);
>     }
>
> No need to do anything, I'll handle this patch after -rc1.

Thanks for that. :)

Regards,
Wanpeng Li

Re: [PATCH v2] KVM: VMX: Fix rflags cache during vCPU reset

[Jim Mattson]

That seems like a convoluted path to produce an illegal RFLAGS value.
What's to prevent syzkaller from simply clearing bit 1 of RFLAGS with
the KVM_SET_REGS ioctl?

On Mon, Nov 20, 2017 at 4:34 PM, Wanpeng Li <kernellwp@gmail.com> wrote:
> 2017-11-21 7:09 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>> On 20/11/2017 23:52, Wanpeng Li wrote:
>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>> index b348920..131fa1c 100644
>>> --- a/arch/x86/kvm/vmx.c
>>> +++ b/arch/x86/kvm/vmx.c
>>> @@ -5590,6 +5590,7 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>>>               vmcs_write64(GUEST_IA32_DEBUGCTL, 0);
>>>       }
>>>
>>> +     kvm_set_rflags(vcpu, 2);
>>>       vmcs_writel(GUEST_RFLAGS, 0x02);
>>
>> I think the vmcs_writel can go, kvm_set_rflags ends up calling
>> vmx_set_rflags.
>
> Agreed.
>
>>
>>>       kvm_rip_write(vcpu, 0xfff0);
>>>
>>>
>>
>> Beautified testcase:
>>
>>     #include <unistd.h>
>>     #include <sys/syscall.h>
>>     #include <string.h>
>>     #include <stdint.h>
>>     #include <linux/kvm.h>
>>     #include <fcntl.h>
>>     #include <sys/ioctl.h>
>>
>>     long r[5];
>>     int main()
>>     {
>>         struct kvm_debugregs dr = { 0 };
>>
>>         r[2] = open("/dev/kvm", O_RDONLY);
>>         r[3] = ioctl(r[2], KVM_CREATE_VM, 0);
>>         r[4] = ioctl(r[3], KVM_CREATE_VCPU, 7);
>>         struct kvm_guest_debug debug = {
>>                 .control = 0xf0403,
>>                 .arch = {
>>                         .debugreg[6] = 0x2,
>>                         .debugreg[7] = 0x2
>>                 }
>>         };
>>         ioctl(r[4], KVM_SET_GUEST_DEBUG, &debug);
>>         ioctl(r[4], KVM_RUN, 0);
>>     }
>>
>> No need to do anything, I'll handle this patch after -rc1.
>
> Thanks for that. :)
>
> Regards,
> Wanpeng Li

Re: [PATCH v2] KVM: VMX: Fix rflags cache during vCPU reset

[Wanpeng Li]

2017-12-05 8:28 GMT+08:00 Jim Mattson <jmattson@google.com>:
> That seems like a convoluted path to produce an illegal RFLAGS value.
> What's to prevent syzkaller from simply clearing bit 1 of RFLAGS with
> the KVM_SET_REGS ioctl?

Yeah, it can happen. Which do you prefer, ioctl fails or |
X86_EFLAGS_FIXED unconditionally in the ioctl handler in kvm?

Regards,
Wanpeng Li

>
> On Mon, Nov 20, 2017 at 4:34 PM, Wanpeng Li <kernellwp@gmail.com> wrote:
>> 2017-11-21 7:09 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>>> On 20/11/2017 23:52, Wanpeng Li wrote:
>>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>>> index b348920..131fa1c 100644
>>>> --- a/arch/x86/kvm/vmx.c
>>>> +++ b/arch/x86/kvm/vmx.c
>>>> @@ -5590,6 +5590,7 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>>>>               vmcs_write64(GUEST_IA32_DEBUGCTL, 0);
>>>>       }
>>>>
>>>> +     kvm_set_rflags(vcpu, 2);
>>>>       vmcs_writel(GUEST_RFLAGS, 0x02);
>>>
>>> I think the vmcs_writel can go, kvm_set_rflags ends up calling
>>> vmx_set_rflags.
>>
>> Agreed.
>>
>>>
>>>>       kvm_rip_write(vcpu, 0xfff0);
>>>>
>>>>
>>>
>>> Beautified testcase:
>>>
>>>     #include <unistd.h>
>>>     #include <sys/syscall.h>
>>>     #include <string.h>
>>>     #include <stdint.h>
>>>     #include <linux/kvm.h>
>>>     #include <fcntl.h>
>>>     #include <sys/ioctl.h>
>>>
>>>     long r[5];
>>>     int main()
>>>     {
>>>         struct kvm_debugregs dr = { 0 };
>>>
>>>         r[2] = open("/dev/kvm", O_RDONLY);
>>>         r[3] = ioctl(r[2], KVM_CREATE_VM, 0);
>>>         r[4] = ioctl(r[3], KVM_CREATE_VCPU, 7);
>>>         struct kvm_guest_debug debug = {
>>>                 .control = 0xf0403,
>>>                 .arch = {
>>>                         .debugreg[6] = 0x2,
>>>                         .debugreg[7] = 0x2
>>>                 }
>>>         };
>>>         ioctl(r[4], KVM_SET_GUEST_DEBUG, &debug);
>>>         ioctl(r[4], KVM_RUN, 0);
>>>     }
>>>
>>> No need to do anything, I'll handle this patch after -rc1.
>>
>> Thanks for that. :)
>>
>> Regards,
>> Wanpeng Li

Re: [PATCH v2] KVM: VMX: Fix rflags cache during vCPU reset

[Paolo Bonzini]

On 05/12/2017 01:53, Wanpeng Li wrote:
>> That seems like a convoluted path to produce an illegal RFLAGS value.
>> What's to prevent syzkaller from simply clearing bit 1 of RFLAGS with
>> the KVM_SET_REGS ioctl?
> Yeah, it can happen. Which do you prefer, ioctl fails or |
> X86_EFLAGS_FIXED unconditionally in the ioctl handler in kvm?

I suspect somebody might be passing an all-zero regs struct to
KVM_SET_REGS, so ORing X86_EFLAGS_FIXED is better.

Thanks,

Paolo

Re: [PATCH v2] KVM: VMX: Fix rflags cache during vCPU reset

[Jim Mattson]

Sorry; I didn't mean to derail this patch thread. Setting bit 1 of
RFLAGS on CPU reset is clearly correct.

I was just noting that if syzkaller is complaining about illegal
RFLAGS, it's trivial for userspace to set RFLAGS to an illegal value.
User space can set all kinds of illegal RFLAGS state...bits 63:22, 15,
5, or 3 can be set; bit 1 can be cleared; RFLAGS.VM can be set in long
mode or real-address mode; RFLAGS.IF can be set while injecting an
interrupt via KVM_SET_VCPU_EVENTS. Let's not get hung up too much on
the one case of bit 1 being clear.

I don't think KVM_SET_REGS, KVM_SET_SREGS, KVM_SET_VCPU_EVENTS, and
friends should necessarily be in the validation business. There are
too many context-dependent validation checks that might pass or fail
depending on the order in which these ioctls are called. In a way,
these ioctls are comparable to the VMWRITE instruction, which does no
validity checking.

Better, perhaps, would be to defer the validity checks to KVM_VMRUN
(and let the hardware take care of them). Unfortunately, this doesn't
interact very well with emulate_invalid_guest_state = true and
enable_unrestricted_guest = false, in which case invalid guest state
is passed to the kernel emulator to see how well it deals with the
illegal state. (By the way, that's a really fun configuration for
syzkaller, because it opens the door to a myriad of kvm warnings that
are difficult to exercise otherwise).

So, how should illegal RFLAGS bits be handled by KVM_SET_REGS? Sure,
we could set bit 1, and we could clear bits 63:22, 15, 5, and 3. But
what about RFLAGS.VM and RFLAGS.IF?

On Tue, Dec 5, 2017 at 5:54 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 05/12/2017 01:53, Wanpeng Li wrote:
>>> That seems like a convoluted path to produce an illegal RFLAGS value.
>>> What's to prevent syzkaller from simply clearing bit 1 of RFLAGS with
>>> the KVM_SET_REGS ioctl?
>> Yeah, it can happen. Which do you prefer, ioctl fails or |
>> X86_EFLAGS_FIXED unconditionally in the ioctl handler in kvm?
>
> I suspect somebody might be passing an all-zero regs struct to
> KVM_SET_REGS, so ORing X86_EFLAGS_FIXED is better.
>
> Thanks,
>
> Paolo