Sean Christopherson <sean.j.christopherson@xxxxxxxxx> writes:
> 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).
I see, thank you for the suggestion, will do in the next version. I'll
split vmrun fix from removing hardcoded instruction lengths from other
intercepts.
>
> 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
>>
--
Vitaly
