* [PATCH] KVM: VMX: Fix invalid guest state detection after task-switch emulation @ 2017-07-11 7:13 Wanpeng Li 2017-07-11 15:54 ` Radim Krčmář 2017-07-19 5:34 ` Wanpeng Li 0 siblings, 2 replies; 14+ messages in thread From: Wanpeng Li @ 2017-07-11 7:13 UTC (permalink / raw) To: linux-kernel, kvm Cc: Paolo Bonzini, Radim Krčmář, Wanpeng Li, Nadav Amit From: Wanpeng Li <wanpeng.li@hotmail.com> 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 in task-switch emulation. Suggested-by: Nadav Amit <nadav.amit@gmail.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Radim Krčmář <rkrcmar@redhat.com> Cc: Nadav Amit <nadav.amit@gmail.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 f50cbfd..70270a2 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -6255,6 +6255,7 @@ static int handle_task_switch(struct kvm_vcpu *vcpu) * TODO: What about debug traps on tss switch? * Are we supposed to inject them and update dr6? */ + vmx->emulation_required = emulation_required(vcpu); return 1; } -- 2.7.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: VMX: Fix invalid guest state detection after task-switch emulation 2017-07-11 7:13 [PATCH] KVM: VMX: Fix invalid guest state detection after task-switch emulation Wanpeng Li @ 2017-07-11 15:54 ` Radim Krčmář 2017-07-11 16:09 ` Paolo Bonzini 2017-07-19 5:34 ` Wanpeng Li 1 sibling, 1 reply; 14+ messages in thread From: Radim Krčmář @ 2017-07-11 15:54 UTC (permalink / raw) To: Wanpeng Li; +Cc: linux-kernel, kvm, Paolo Bonzini, Wanpeng Li, Nadav Amit 2017-07-11 00:13-0700, Wanpeng Li: > From: Wanpeng Li <wanpeng.li@hotmail.com> > > 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 in task-switch emulation. > > Suggested-by: Nadav Amit <nadav.amit@gmail.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Radim Krčmář <rkrcmar@redhat.com> > Cc: Nadav Amit <nadav.amit@gmail.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 f50cbfd..70270a2 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -6255,6 +6255,7 @@ static int handle_task_switch(struct kvm_vcpu *vcpu) > * TODO: What about debug traps on tss switch? > * Are we supposed to inject them and update dr6? > */ > + vmx->emulation_required = emulation_required(vcpu); Hm, so the problem happened because changes to rflags can flip the value of emulation_required(). I would add this line to vmx_set_rflags() to make sure that we fixed everything, thanks. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: VMX: Fix invalid guest state detection after task-switch emulation 2017-07-11 15:54 ` Radim Krčmář @ 2017-07-11 16:09 ` Paolo Bonzini 2017-07-12 13:50 ` Wanpeng Li 0 siblings, 1 reply; 14+ messages in thread From: Paolo Bonzini @ 2017-07-11 16:09 UTC (permalink / raw) To: Radim Krčmář, Wanpeng Li Cc: linux-kernel, kvm, Wanpeng Li, Nadav Amit On 11/07/2017 17:54, Radim Krčmář wrote: > 2017-07-11 00:13-0700, Wanpeng Li: >> From: Wanpeng Li <wanpeng.li@hotmail.com> >> >> 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 in task-switch emulation. >> >> Suggested-by: Nadav Amit <nadav.amit@gmail.com> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Cc: Radim Krčmář <rkrcmar@redhat.com> >> Cc: Nadav Amit <nadav.amit@gmail.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 f50cbfd..70270a2 100644 >> --- a/arch/x86/kvm/vmx.c >> +++ b/arch/x86/kvm/vmx.c >> @@ -6255,6 +6255,7 @@ static int handle_task_switch(struct kvm_vcpu *vcpu) >> * TODO: What about debug traps on tss switch? >> * Are we supposed to inject them and update dr6? >> */ >> + vmx->emulation_required = emulation_required(vcpu); > > Hm, so the problem happened because changes to rflags can flip the value > of emulation_required(). I would add this line to vmx_set_rflags() to > make sure that we fixed everything, thanks. Note that there is some extra complication, because emulation_required is expensive and you'll want to run it only when EFLAGS.VM changes. Paolo ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: VMX: Fix invalid guest state detection after task-switch emulation 2017-07-11 16:09 ` Paolo Bonzini @ 2017-07-12 13:50 ` Wanpeng Li 0 siblings, 0 replies; 14+ messages in thread From: Wanpeng Li @ 2017-07-12 13:50 UTC (permalink / raw) To: Paolo Bonzini Cc: Radim Krčmář, linux-kernel, kvm, Wanpeng Li, Nadav Amit 2017-07-12 0:09 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>: > On 11/07/2017 17:54, Radim Krčmář wrote: >> 2017-07-11 00:13-0700, Wanpeng Li: >>> From: Wanpeng Li <wanpeng.li@hotmail.com> >>> >>> 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 in task-switch emulation. >>> >>> Suggested-by: Nadav Amit <nadav.amit@gmail.com> >>> Cc: Paolo Bonzini <pbonzini@redhat.com> >>> Cc: Radim Krčmář <rkrcmar@redhat.com> >>> Cc: Nadav Amit <nadav.amit@gmail.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 f50cbfd..70270a2 100644 >>> --- a/arch/x86/kvm/vmx.c >>> +++ b/arch/x86/kvm/vmx.c >>> @@ -6255,6 +6255,7 @@ static int handle_task_switch(struct kvm_vcpu *vcpu) >>> * TODO: What about debug traps on tss switch? >>> * Are we supposed to inject them and update dr6? >>> */ >>> + vmx->emulation_required = emulation_required(vcpu); >> >> Hm, so the problem happened because changes to rflags can flip the value >> of emulation_required(). I would add this line to vmx_set_rflags() to >> make sure that we fixed everything, thanks. > > Note that there is some extra complication, because emulation_required > is expensive and you'll want to run it only when EFLAGS.VM changes. Agreed. Regards, Wanpeng Li ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: VMX: Fix invalid guest state detection after task-switch emulation 2017-07-11 7:13 [PATCH] KVM: VMX: Fix invalid guest state detection after task-switch emulation Wanpeng Li 2017-07-11 15:54 ` Radim Krčmář @ 2017-07-19 5:34 ` Wanpeng Li 2017-07-19 11:29 ` Radim Krčmář 1 sibling, 1 reply; 14+ messages in thread From: Wanpeng Li @ 2017-07-19 5:34 UTC (permalink / raw) To: linux-kernel, kvm Cc: Paolo Bonzini, Radim Krčmář, Wanpeng Li, Nadav Amit Ping, :) 2017-07-11 15:13 GMT+08:00 Wanpeng Li <kernellwp@gmail.com>: > From: Wanpeng Li <wanpeng.li@hotmail.com> > > 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 in task-switch emulation. > > Suggested-by: Nadav Amit <nadav.amit@gmail.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Radim Krčmář <rkrcmar@redhat.com> > Cc: Nadav Amit <nadav.amit@gmail.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 f50cbfd..70270a2 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -6255,6 +6255,7 @@ static int handle_task_switch(struct kvm_vcpu *vcpu) > * TODO: What about debug traps on tss switch? > * Are we supposed to inject them and update dr6? > */ > + vmx->emulation_required = emulation_required(vcpu); > > return 1; > } > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] KVM: VMX: Fix invalid guest state detection after task-switch emulation 2017-07-19 5:34 ` Wanpeng Li @ 2017-07-19 11:29 ` Radim Krčmář 2017-07-19 15:14 ` Nadav Amit 0 siblings, 1 reply; 14+ messages in thread From: Radim Krčmář @ 2017-07-19 11:29 UTC (permalink / raw) To: Wanpeng Li; +Cc: linux-kernel, kvm, Paolo Bonzini, Wanpeng Li, Nadav Amit 2017-07-19 13:34+0800, Wanpeng Li: > Ping, :) I misunderstood where the ball is ... is the patch below ok? ---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@gmail.com> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com> [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@redhat.com> --- 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 = to_vmx(vcpu)->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 +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); } static u32 vmx_get_pkru(struct kvm_vcpu *vcpu) @@ -3857,11 +3867,6 @@ static __init int alloc_kvm_area(void) return 0; } -static bool emulation_required(struct kvm_vcpu *vcpu) -{ - return emulate_invalid_guest_state && !guest_state_valid(vcpu); -} - static void fix_pmode_seg(struct kvm_vcpu *vcpu, int seg, struct kvm_segment *save) { -- 2.13.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: VMX: Fix invalid guest state detection after task-switch emulation 2017-07-19 11:29 ` Radim Krčmář @ 2017-07-19 15:14 ` Nadav Amit 2017-07-19 16:19 ` [PATCH v2] " Radim Krčmář 0 siblings, 1 reply; 14+ messages in thread From: Nadav Amit @ 2017-07-19 15:14 UTC (permalink / raw) To: Radim Krčmář Cc: Wanpeng Li, linux-kernel, kvm, Paolo Bonzini, Wanpeng Li Radim Krčmář <rkrcmar@redhat.com> wrote: > 2017-07-19 13:34+0800, Wanpeng Li: >> Ping, :) > > I misunderstood where the ball is ... is the patch below ok? > > ---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@gmail.com> > Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com> > [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@redhat.com> > --- > 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 = to_vmx(vcpu)->rflags; It assumes rflags was decached from the VMCS before. Probably it is true, but… ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2] KVM: VMX: Fix invalid guest state detection after task-switch emulation 2017-07-19 15:14 ` Nadav Amit @ 2017-07-19 16:19 ` Radim Krčmář 2017-07-19 16:25 ` Nadav Amit 0 siblings, 1 reply; 14+ messages in thread From: Radim Krčmář @ 2017-07-19 16:19 UTC (permalink / raw) To: Nadav Amit; +Cc: Wanpeng Li, linux-kernel, kvm, Paolo Bonzini, Wanpeng Li 2017-07-19 08:14-0700, Nadav Amit: > Radim Krčmář <rkrcmar@redhat.com> 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@gmail.com> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com> [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@redhat.com> --- 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); } static u32 vmx_get_pkru(struct kvm_vcpu *vcpu) @@ -3857,11 +3867,6 @@ static __init int alloc_kvm_area(void) return 0; } -static bool emulation_required(struct kvm_vcpu *vcpu) -{ - return emulate_invalid_guest_state && !guest_state_valid(vcpu); -} - static void fix_pmode_seg(struct kvm_vcpu *vcpu, int seg, struct kvm_segment *save) { -- 2.13.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2] KVM: VMX: Fix invalid guest state detection after task-switch emulation 2017-07-19 16:19 ` [PATCH v2] " Radim Krčmář @ 2017-07-19 16:25 ` Nadav Amit 2017-07-19 22:48 ` Wanpeng Li 0 siblings, 1 reply; 14+ messages in thread From: Nadav Amit @ 2017-07-19 16:25 UTC (permalink / raw) To: Radim Krčmář Cc: Wanpeng Li, linux-kernel, kvm, Paolo Bonzini, Wanpeng Li Radim Krčmář <rkrcmar@redhat.com> wrote: > 2017-07-19 08:14-0700, Nadav Amit: >> Radim Krčmář <rkrcmar@redhat.com> 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@gmail.com> > Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com> > [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@redhat.com> > --- > 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”. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] KVM: VMX: Fix invalid guest state detection after task-switch emulation 2017-07-19 16:25 ` Nadav Amit @ 2017-07-19 22:48 ` Wanpeng Li 2017-07-19 22:53 ` Nadav Amit 0 siblings, 1 reply; 14+ messages in thread From: Wanpeng Li @ 2017-07-19 22:48 UTC (permalink / raw) To: Nadav Amit Cc: Radim Krčmář, linux-kernel, kvm, Paolo Bonzini, Wanpeng Li 2017-07-20 0:25 GMT+08:00 Nadav Amit <nadav.amit@gmail.com>: > Radim Krčmář <rkrcmar@redhat.com> wrote: > >> 2017-07-19 08:14-0700, Nadav Amit: >>> Radim Krčmář <rkrcmar@redhat.com> 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@gmail.com> >> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com> >> [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@redhat.com> >> --- >> 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] KVM: VMX: Fix invalid guest state detection after task-switch emulation 2017-07-19 22:48 ` Wanpeng Li @ 2017-07-19 22:53 ` Nadav Amit 2017-07-19 23:01 ` Wanpeng Li 0 siblings, 1 reply; 14+ messages in thread From: Nadav Amit @ 2017-07-19 22:53 UTC (permalink / raw) To: Wanpeng Li Cc: Radim Krčmář, linux-kernel, kvm, Paolo Bonzini, Wanpeng Li Wanpeng Li <kernellwp@gmail.com> wrote: > 2017-07-20 0:25 GMT+08:00 Nadav Amit <nadav.amit@gmail.com>: >> Radim Krčmář <rkrcmar@redhat.com> wrote: >> >>> 2017-07-19 08:14-0700, Nadav Amit: >>>> Radim Krčmář <rkrcmar@redhat.com> 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@gmail.com> >>> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com> >>> [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@redhat.com> >>> --- >>> 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] KVM: VMX: Fix invalid guest state detection after task-switch emulation 2017-07-19 22:53 ` Nadav Amit @ 2017-07-19 23:01 ` Wanpeng Li 2017-07-19 23:06 ` Nadav Amit 0 siblings, 1 reply; 14+ messages in thread From: Wanpeng Li @ 2017-07-19 23:01 UTC (permalink / raw) To: Nadav Amit Cc: Radim Krčmář, linux-kernel, kvm, Paolo Bonzini, Wanpeng Li 2017-07-20 6:53 GMT+08:00 Nadav Amit <nadav.amit@gmail.com>: > Wanpeng Li <kernellwp@gmail.com> wrote: > >> 2017-07-20 0:25 GMT+08:00 Nadav Amit <nadav.amit@gmail.com>: >>> Radim Krčmář <rkrcmar@redhat.com> wrote: >>> >>>> 2017-07-19 08:14-0700, Nadav Amit: >>>>> Radim Krčmář <rkrcmar@redhat.com> 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@gmail.com> >>>> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com> >>>> [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@redhat.com> >>>> --- >>>> 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] KVM: VMX: Fix invalid guest state detection after task-switch emulation 2017-07-19 23:01 ` Wanpeng Li @ 2017-07-19 23:06 ` Nadav Amit 2017-07-19 23:09 ` Wanpeng Li 0 siblings, 1 reply; 14+ messages in thread From: Nadav Amit @ 2017-07-19 23:06 UTC (permalink / raw) To: Wanpeng Li Cc: Radim Krčmář, linux-kernel, kvm, Paolo Bonzini, Wanpeng Li Wanpeng Li <kernellwp@gmail.com> wrote: > 2017-07-20 6:53 GMT+08:00 Nadav Amit <nadav.amit@gmail.com>: >> Wanpeng Li <kernellwp@gmail.com> wrote: >> >>> 2017-07-20 0:25 GMT+08:00 Nadav Amit <nadav.amit@gmail.com>: >>>> Radim Krčmář <rkrcmar@redhat.com> wrote: >>>> >>>>> 2017-07-19 08:14-0700, Nadav Amit: >>>>>> Radim Krčmář <rkrcmar@redhat.com> 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@gmail.com> >>>>> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com> >>>>> [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@redhat.com> >>>>> --- >>>>> 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] KVM: VMX: Fix invalid guest state detection after task-switch emulation 2017-07-19 23:06 ` Nadav Amit @ 2017-07-19 23:09 ` Wanpeng Li 0 siblings, 0 replies; 14+ messages in thread From: Wanpeng Li @ 2017-07-19 23:09 UTC (permalink / raw) To: Nadav Amit Cc: Radim Krčmář, linux-kernel, kvm, Paolo Bonzini, Wanpeng Li 2017-07-20 7:06 GMT+08:00 Nadav Amit <nadav.amit@gmail.com>: > Wanpeng Li <kernellwp@gmail.com> wrote: > >> 2017-07-20 6:53 GMT+08:00 Nadav Amit <nadav.amit@gmail.com>: >>> Wanpeng Li <kernellwp@gmail.com> wrote: >>> >>>> 2017-07-20 0:25 GMT+08:00 Nadav Amit <nadav.amit@gmail.com>: >>>>> Radim Krčmář <rkrcmar@redhat.com> wrote: >>>>> >>>>>> 2017-07-19 08:14-0700, Nadav Amit: >>>>>>> Radim Krčmář <rkrcmar@redhat.com> 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@gmail.com> >>>>>> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com> >>>>>> [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@redhat.com> >>>>>> --- >>>>>> 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2017-07-19 23:10 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-07-11 7:13 [PATCH] KVM: VMX: Fix invalid guest state detection after task-switch emulation Wanpeng Li 2017-07-11 15:54 ` Radim Krčmář 2017-07-11 16:09 ` Paolo Bonzini 2017-07-12 13:50 ` Wanpeng Li 2017-07-19 5:34 ` Wanpeng Li 2017-07-19 11:29 ` Radim Krčmář 2017-07-19 15:14 ` Nadav Amit 2017-07-19 16:19 ` [PATCH v2] " Radim Krčmář 2017-07-19 16:25 ` Nadav Amit 2017-07-19 22:48 ` Wanpeng Li 2017-07-19 22:53 ` Nadav Amit 2017-07-19 23:01 ` Wanpeng Li 2017-07-19 23:06 ` Nadav Amit 2017-07-19 23:09 ` Wanpeng Li
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.