Re: [PATCH v2] KVM: VMX: Fix invalid guest state detection after task-switch emulation

[Wanpeng Li]

2017-07-20 7:06 GMT+08:00 Nadav Amit <nadav.amit@xxxxxxxxx>:
> Wanpeng Li <kernellwp@xxxxxxxxx> wrote:
>
>> 2017-07-20 6:53 GMT+08:00 Nadav Amit <nadav.amit@xxxxxxxxx>:
>>> Wanpeng Li <kernellwp@xxxxxxxxx> wrote:
>>>
>>>> 2017-07-20 0:25 GMT+08:00 Nadav Amit <nadav.amit@xxxxxxxxx>:
>>>>> Radim KrÄmÃÅ <rkrcmar@xxxxxxxxxx> wrote:
>>>>>
>>>>>> 2017-07-19 08:14-0700, Nadav Amit:
>>>>>>> Radim KrÄmÃÅ <rkrcmar@xxxxxxxxxx> wrote:
>>>>>>>> @@ -2363,6 +2368,8 @@ static unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu)
>>>>>>>>
>>>>>>>> static void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags)
>>>>>>>> {
>>>>>>>> +   unsigned long old_rflags = to_vmx(vcpu)->rflags;
>>>>>>>
>>>>>>> It assumes rflags was decached from the VMCS before. Probably it is true, butâ
>>>>>>
>>>>>> Right, it's better to use accessors everywhere, thanks.
>>>>>> The line should read:
>>>>>>
>>>>>> +     unsigned long old_rflags = vmx_get_rflags(vcpu);
>>>>>>
>>>>>> ---8<---
>>>>>> This can be reproduced by EPT=1, unrestricted_guest=N, emulate_invalid_state=Y
>>>>>> or EPT=0, the trace of kvm-unit-tests/taskswitch2.flat is like below, it
>>>>>> tries to emulate invalid guest state task-switch:
>>>>>>
>>>>>> kvm_exit: reason TASK_SWITCH rip 0x0 info 40000058 0
>>>>>> kvm_emulate_insn: 42000:0:0f 0b (0x2)
>>>>>> kvm_emulate_insn: 42000:0:0f 0b (0x2) failed
>>>>>> kvm_inj_exception: #UD (0x0)
>>>>>> kvm_entry: vcpu 0
>>>>>> kvm_exit: reason TASK_SWITCH rip 0x0 info 40000058 0
>>>>>> kvm_emulate_insn: 42000:0:0f 0b (0x2)
>>>>>> kvm_emulate_insn: 42000:0:0f 0b (0x2) failed
>>>>>> kvm_inj_exception: #UD (0x0)
>>>>>>
>>>>>> It appears that the task-switch emulation updates rflags (and vm86 flag)
>>>>>> only after the segments are loaded, causing vmx->emulation_required to
>>>>>> be set, when in fact invalid guest state emulation is not needed.
>>>>>>
>>>>>> This patch fixes it by updating vmx->emulation_required after the rflags
>>>>>> (and vm86 flag) is updated.
>>>>>>
>>>>>> Suggested-by: Nadav Amit <nadav.amit@xxxxxxxxx>
>>>>>> Signed-off-by: Wanpeng Li <wanpeng.li@xxxxxxxxxxx>
>>>>>> [Wanpeng wrote the commit message with initial patch and Radim moved the
>>>>>> update to vmx_set_rflags and added Paolo's suggestion for the check.]
>>>>>> Signed-off-by: Radim KrÄmÃÅ <rkrcmar@xxxxxxxxxx>
>>>>>> ---
>>>>>> arch/x86/kvm/vmx.c | 15 ++++++++++-----
>>>>>> 1 file changed, 10 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>>>>> index 84e62acf2dd8..a776aea0043a 100644
>>>>>> --- a/arch/x86/kvm/vmx.c
>>>>>> +++ b/arch/x86/kvm/vmx.c
>>>>>> @@ -2326,6 +2326,11 @@ static void vmx_vcpu_put(struct kvm_vcpu *vcpu)
>>>>>>     __vmx_load_host_state(to_vmx(vcpu));
>>>>>> }
>>>>>>
>>>>>> +static bool emulation_required(struct kvm_vcpu *vcpu)
>>>>>> +{
>>>>>> +     return emulate_invalid_guest_state && !guest_state_valid(vcpu);
>>>>>> +}
>>>>>> +
>>>>>> static void vmx_decache_cr0_guest_bits(struct kvm_vcpu *vcpu);
>>>>>>
>>>>>> /*
>>>>>> @@ -2363,6 +2368,8 @@ static unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu)
>>>>>>
>>>>>> static void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags)
>>>>>> {
>>>>>> +     unsigned long old_rflags = vmx_get_rflags(vcpu);
>>>>>> +
>>>>>>     __set_bit(VCPU_EXREG_RFLAGS, (ulong *)&vcpu->arch.regs_avail);
>>>>>>     to_vmx(vcpu)->rflags = rflags;
>>>>>>     if (to_vmx(vcpu)->rmode.vm86_active) {
>>>>>> @@ -2370,6 +2377,9 @@ static void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags)
>>>>>>             rflags |= X86_EFLAGS_IOPL | X86_EFLAGS_VM;
>>>>>>     }
>>>>>>     vmcs_writel(GUEST_RFLAGS, rflags);
>>>>>> +
>>>>>> +     if ((old_rflags ^ rflags) & X86_EFLAGS_VM)
>>>>>> +             to_vmx(vcpu)->emulation_required = emulation_required(vcpu);
>>>>>
>>>>> Sorry for not pointing it before, but here you compare the old_rflags with
>>>>> the new rflags but after you already âmassagedâ it. So the value you compare
>>>>> with is not what the guest âseesâ.
>>>>
>>>> So you mean we should use unsigned long old_rflags =
>>>> vmcs_readl(GUEST_RFLAGS); right?
>>>
>>> No. The problem is not with old_rflags now, but with rflags. If vm86_active,
>>> then rflags is changed and you donât compare the guest-visible rflags
>>> anymore.
>>
>> Ah, I see. So we should compare the old_flags with the
>> rmode->save_rflags(guest-visible rflags) instead of the rflags (shadow
>> rflags), right?
>
> Not exactly, since rmode->save_rflags are invalid if !vm86_active. Instead,
> I think you should have a save_rflags variable on the stack that would hold
> the rflags before âmassagingâ and use it instead.

Thanks for pointing out :) I will send out a new version, in addition,
thanks Radim's help for these two versions. :)

Regards,
Wanpeng Li