Re: [PATCH v2] KVM: VMX: Fix invalid guest state detection after task-switch emulation
2017-07-20 15:57 GMT+08:00 Radim Krčmář : > 2017-07-19 16:40-0700, Wanpeng Li: >> From: Wanpeng Li >> >> 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 4058 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 4058 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 in task-switch emulation. >> >> Thanks Radim for moving the update to vmx__set_flags and adding Paolo's >> suggestion for the check. >> >> Suggested-by: Nadav Amit >> Cc: Paolo Bonzini >> Cc: Radim Krčmář >> Cc: Nadav Amit >> Signed-off-by: Wanpeng Li >> --- >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >> static void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags) >> { >> + unsigned long old_rflags = vmx_get_rflags(vcpu); >> + unsigned long save_rflags = rflags; >> + >> __set_bit(VCPU_EXREG_RFLAGS, (ulong *)&vcpu->arch.regs_avail); >> to_vmx(vcpu)->rflags = rflags; >> if (to_vmx(vcpu)->rmode.vm86_active) { >> @@ -2370,6 +2378,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 ^ save_rflags) & X86_EFLAGS_VM) > > Looks good, thanks to both. > > The desired rflags are saved in to_vmx(vcpu)->rflags, so we don't need > the temporary variable -- would using that or vmx_get_rflags(vcpu) > generate worse code? Yeah, I will send out a newer version. Regards, Wanpeng Li
Re: [PATCH v2] KVM: VMX: Fix invalid guest state detection after task-switch emulation
2017-07-19 16:40-0700, Wanpeng Li: > From: Wanpeng Li > > 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 4058 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 4058 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 in task-switch emulation. > > Thanks Radim for moving the update to vmx__set_flags and adding Paolo's > suggestion for the check. > > Suggested-by: Nadav Amit > Cc: Paolo Bonzini > Cc: Radim Krčmář > Cc: Nadav Amit > Signed-off-by: Wanpeng Li > --- > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > static void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags) > { > + unsigned long old_rflags = vmx_get_rflags(vcpu); > + unsigned long save_rflags = rflags; > + > __set_bit(VCPU_EXREG_RFLAGS, (ulong *)&vcpu->arch.regs_avail); > to_vmx(vcpu)->rflags = rflags; > if (to_vmx(vcpu)->rmode.vm86_active) { > @@ -2370,6 +2378,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 ^ save_rflags) & X86_EFLAGS_VM) Looks good, thanks to both. The desired rflags are saved in to_vmx(vcpu)->rflags, so we don't need the temporary variable -- would using that or vmx_get_rflags(vcpu) generate worse code?
Re: [PATCH v2] KVM: VMX: Fix invalid guest state detection after task-switch emulation
2017-07-20 7:06 GMT+08:00 Nadav Amit : > Wanpeng Li wrote: > >> 2017-07-20 6:53 GMT+08:00 Nadav Amit : >>> Wanpeng Li wrote: >>> 2017-07-20 0:25 GMT+08:00 Nadav Amit : > Radim Krčmář wrote: > >> 2017-07-19 08:14-0700, Nadav Amit: >>> Radim Krčmář 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 4058 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 4058 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 >> Signed-off-by: Wanpeng Li >> [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ář >> --- >> 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
Re: [PATCH v2] KVM: VMX: Fix invalid guest state detection after task-switch emulation
Wanpeng Li wrote: > 2017-07-20 6:53 GMT+08:00 Nadav Amit : >> Wanpeng Li wrote: >> >>> 2017-07-20 0:25 GMT+08:00 Nadav Amit : Radim Krčmář wrote: > 2017-07-19 08:14-0700, Nadav Amit: >> Radim Krčmář 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 4058 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 4058 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 > Signed-off-by: Wanpeng Li > [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ář > --- > 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.
Re: [PATCH v2] KVM: VMX: Fix invalid guest state detection after task-switch emulation
2017-07-20 6:53 GMT+08:00 Nadav Amit : > Wanpeng Li wrote: > >> 2017-07-20 0:25 GMT+08:00 Nadav Amit : >>> Radim Krčmář wrote: >>> 2017-07-19 08:14-0700, Nadav Amit: > Radim Krčmář 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 4058 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 4058 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 Signed-off-by: Wanpeng Li [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ář --- 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? Regards, Wanpeng Li
Re: [PATCH v2] KVM: VMX: Fix invalid guest state detection after task-switch emulation
Wanpeng Li wrote: > 2017-07-20 0:25 GMT+08:00 Nadav Amit : >> Radim Krčmář wrote: >> >>> 2017-07-19 08:14-0700, Nadav Amit: Radim Krčmář 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 4058 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 4058 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 >>> Signed-off-by: Wanpeng Li >>> [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ář >>> --- >>> 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.
Re: [PATCH v2] KVM: VMX: Fix invalid guest state detection after task-switch emulation
2017-07-20 0:25 GMT+08:00 Nadav Amit : > Radim Krčmář wrote: > >> 2017-07-19 08:14-0700, Nadav Amit: >>> Radim Krčmář 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 4058 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 4058 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 >> Signed-off-by: Wanpeng Li >> [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ář >> --- >> 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? Regards, Wanpeng Li
Re: [PATCH v2] KVM: VMX: Fix invalid guest state detection after task-switch emulation
Radim Krčmář wrote: > 2017-07-19 08:14-0700, Nadav Amit: >> Radim Krčmář 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 4058 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 4058 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 > Signed-off-by: Wanpeng Li > [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ář > --- > 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”.