On Tue, Aug 06, 2019 at 08:01:50AM +0200, Vitaly Kuznetsov wrote:
> Various intercepts hard-code the respective instruction lengths to optimize
> skip_emulated_instruction(): when next_rip is pre-set we skip
> kvm_emulate_instruction(vcpu, EMULTYPE_SKIP). The optimization is, however,
> incorrect: different (redundant) prefixes could be used to enlarge the
> instruction. We can't really avoid decoding.
>
> svm->next_rip is not used when CPU supports 'nrips' (X86_FEATURE_NRIPS)
> feature: next RIP is provided in VMCB. The feature is not really new
> (Opteron G3s had it already) and the change should have zero affect.
>
> Remove manual svm->next_rip setting with hard-coded instruction lengths.
> The only case where we now use svm->next_rip is EXIT_IOIO: the instruction
> length is provided to us by hardware.
>
> Reported-by: Jim Mattson <jmattson@xxxxxxxxxx>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
> ---
> arch/x86/kvm/svm.c | 15 ++-------------
> 1 file changed, 2 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 793a60461abe..dce215250d1f 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -2905,13 +2905,11 @@ static int nop_on_interception(struct vcpu_svm *svm)
>
> static int halt_interception(struct vcpu_svm *svm)
> {
> - svm->next_rip = kvm_rip_read(&svm->vcpu) + 1;
> return kvm_emulate_halt(&svm->vcpu);
> }
>
> static int vmmcall_interception(struct vcpu_svm *svm)
> {
> - svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
> return kvm_emulate_hypercall(&svm->vcpu);
> }
>
> @@ -3699,7 +3697,6 @@ static int vmload_interception(struct vcpu_svm *svm)
>
> nested_vmcb = map.hva;
>
> - svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
> ret = kvm_skip_emulated_instruction(&svm->vcpu);
>
> nested_svm_vmloadsave(nested_vmcb, svm->vmcb);
> @@ -3726,7 +3723,6 @@ static int vmsave_interception(struct vcpu_svm *svm)
>
> nested_vmcb = map.hva;
>
> - svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
> ret = kvm_skip_emulated_instruction(&svm->vcpu);
>
> nested_svm_vmloadsave(svm->vmcb, nested_vmcb);
> @@ -3740,8 +3736,8 @@ static int vmrun_interception(struct vcpu_svm *svm)
> if (nested_svm_check_permissions(svm))
> return 1;
>
> - /* Save rip after vmrun instruction */
> - kvm_rip_write(&svm->vcpu, kvm_rip_read(&svm->vcpu) + 3);
> + if (!kvm_skip_emulated_instruction(&svm->vcpu))
> + return 1;
This is broken, e.g. KVM shouldn't resume the guest on single-step strap,
nor should it skip the meat of VMRUN emulation. There are also several
pre-existing bugs, e.g. #GP can be injected due to invalid vmcb_gpa after
%rip is incremented and single-step #DB can be suppressed on failed
VMRUN (assuming that's not architectural behavior?).
Calling kvm_skip_emulated_instruction() after nested_svm_vmrun() looks like
it'd cause all kinds of problems, so I think the correct fix is to put the
call to kvm_skip_emulated_instruction() inside nested_svm_vmrun(), after
it maps the vmcb_gpa, e.g. something like this in the end (optionally
moving nested_svm_vmrun_msrpm() inside nested_svm_run() to eliminate the
weird goto handling).
static int nested_svm_vmrun(struct vcpu_svm *svm)
{
int rc, ret;
struct vmcb *nested_vmcb;
struct vmcb *hsave = svm->nested.hsave;
struct vmcb *vmcb = svm->vmcb;
struct kvm_host_map map;
u64 vmcb_gpa;
vmcb_gpa = svm->vmcb->save.rax;
rc = kvm_vcpu_map(&svm->vcpu, gfn_to_gpa(vmcb_gpa), &map);
if (rc == -EINVAL)
kvm_inject_gp(&svm->vcpu, 0);
return 1;
}
ret = kvm_skip_emulated_instruction(&svm->vcpu);
if (rc)
return ret;
nested_vmcb = map.hva;
if (!nested_vmcb_checks(nested_vmcb)) {
nested_vmcb->control.exit_code = SVM_EXIT_ERR;
nested_vmcb->control.exit_code_hi = 0;
nested_vmcb->control.exit_info_1 = 0;
nested_vmcb->control.exit_info_2 = 0;
kvm_vcpu_unmap(&svm->vcpu, &map, true);
return ret;
}
< ... more existing code ... >
enter_svm_guest_mode(svm, vmcb_gpa, nested_vmcb, &map);
if (!nested_svm_vmrun_msrpm(svm)) {
svm->vmcb->control.exit_code = SVM_EXIT_ERR;
svm->vmcb->control.exit_code_hi = 0;
svm->vmcb->control.exit_info_1 = 0;
svm->vmcb->control.exit_info_2 = 0;
nested_svm_vmexit(svm);
}
return ret;
}
static int vmrun_interception(struct vcpu_svm *svm)
{
if (nested_svm_check_permissions(svm))
return 1;
return nested_svm_vmrun(svm)
}
>
> if (!nested_svm_vmrun(svm))
> return 1;
> @@ -3777,7 +3773,6 @@ static int stgi_interception(struct vcpu_svm *svm)
> if (vgif_enabled(svm))
> clr_intercept(svm, INTERCEPT_STGI);
>
> - svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
> ret = kvm_skip_emulated_instruction(&svm->vcpu);
> kvm_make_request(KVM_REQ_EVENT, &svm->vcpu);
>
> @@ -3793,7 +3788,6 @@ static int clgi_interception(struct vcpu_svm *svm)
> if (nested_svm_check_permissions(svm))
> return 1;
>
> - svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
> ret = kvm_skip_emulated_instruction(&svm->vcpu);
>
> disable_gif(svm);
> @@ -3818,7 +3812,6 @@ static int invlpga_interception(struct vcpu_svm *svm)
> /* Let's treat INVLPGA the same as INVLPG (can be optimized!) */
> kvm_mmu_invlpg(vcpu, kvm_rax_read(&svm->vcpu));
>
> - svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
> return kvm_skip_emulated_instruction(&svm->vcpu);
> }
>
> @@ -3841,7 +3834,6 @@ static int xsetbv_interception(struct vcpu_svm *svm)
> u32 index = kvm_rcx_read(&svm->vcpu);
>
> if (kvm_set_xcr(&svm->vcpu, index, new_bv) == 0) {
> - svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
> return kvm_skip_emulated_instruction(&svm->vcpu);
> }
>
> @@ -3918,7 +3910,6 @@ static int task_switch_interception(struct vcpu_svm *svm)
>
> static int cpuid_interception(struct vcpu_svm *svm)
> {
> - svm->next_rip = kvm_rip_read(&svm->vcpu) + 2;
> return kvm_emulate_cpuid(&svm->vcpu);
> }
>
> @@ -4248,7 +4239,6 @@ static int rdmsr_interception(struct vcpu_svm *svm)
>
> kvm_rax_write(&svm->vcpu, msr_info.data & 0xffffffff);
> kvm_rdx_write(&svm->vcpu, msr_info.data >> 32);
> - svm->next_rip = kvm_rip_read(&svm->vcpu) + 2;
> return kvm_skip_emulated_instruction(&svm->vcpu);
> }
> }
> @@ -4454,7 +4444,6 @@ static int wrmsr_interception(struct vcpu_svm *svm)
> return 1;
> } else {
> trace_kvm_msr_write(ecx, data);
> - svm->next_rip = kvm_rip_read(&svm->vcpu) + 2;
> return kvm_skip_emulated_instruction(&svm->vcpu);
> }
> }
> --
> 2.20.1
>
