On 19/05/2017 15:48, Radim Krčmář wrote: > kvm_skip_emulated_instruction() will return 0 if userspace is > single-stepping the guest. > > kvm_skip_emulated_instruction() uses return status convention of exit > handler: 0 means "exit to userspace" and 1 means "continue vm entries". > The problem is that nested_vmx_check_vmptr() return status means > something else: 0 is ok, 1 is error. > > This means we would continue executing after a failure. Static checker > noticed it because vmptr was not initialized. > > Reported-by: Dan Carpenter <[email protected]> > Fixes: 6affcbedcac7 ("KVM: x86: Add kvm_skip_emulated_instruction and use > it.") > Signed-off-by: Radim Krčmář <[email protected]> > --- > Second try -- moves a lot of code around to make it less ugly and keep > the same behavior as v1, hopefully. > > arch/x86/kvm/vmx.c | 140 > ++++++++++++++++++++++------------------------------- > 1 file changed, 57 insertions(+), 83 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 9ff1b04502c9..dd274db9bf77 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -6914,97 +6914,21 @@ static int get_vmx_mem_address(struct kvm_vcpu *vcpu, > return 0; > } > > -/* > - * This function performs the various checks including > - * - if it's 4KB aligned > - * - No bits beyond the physical address width are set > - * - Returns 0 on success or else 1 > - * (Intel SDM Section 30.3) > - */ > -static int nested_vmx_check_vmptr(struct kvm_vcpu *vcpu, int exit_reason, > - gpa_t *vmpointer) > +static int nested_vmx_get_vmptr(struct kvm_vcpu *vcpu, gpa_t *vmpointer) > { > gva_t gva; > - gpa_t vmptr; > struct x86_exception e; > - struct page *page; > - struct vcpu_vmx *vmx = to_vmx(vcpu); > - int maxphyaddr = cpuid_maxphyaddr(vcpu); > > if (get_vmx_mem_address(vcpu, vmcs_readl(EXIT_QUALIFICATION), > vmcs_read32(VMX_INSTRUCTION_INFO), false, &gva)) > return 1; > > - if (kvm_read_guest_virt(&vcpu->arch.emulate_ctxt, gva, &vmptr, > - sizeof(vmptr), &e)) { > + if (kvm_read_guest_virt(&vcpu->arch.emulate_ctxt, gva, vmpointer, > + sizeof(*vmpointer), &e)) { > kvm_inject_page_fault(vcpu, &e); > return 1; > } > > - switch (exit_reason) { > - case EXIT_REASON_VMON: > - /* > - * SDM 3: 24.11.5 > - * The first 4 bytes of VMXON region contain the supported > - * VMCS revision identifier > - * > - * Note - IA32_VMX_BASIC[48] will never be 1 > - * for the nested case; > - * which replaces physical address width with 32 > - * > - */ > - if (!PAGE_ALIGNED(vmptr) || (vmptr >> maxphyaddr)) { > - nested_vmx_failInvalid(vcpu); > - return kvm_skip_emulated_instruction(vcpu); > - } > - > - page = nested_get_page(vcpu, vmptr); > - if (page == NULL) { > - nested_vmx_failInvalid(vcpu); > - return kvm_skip_emulated_instruction(vcpu); > - } > - if (*(u32 *)kmap(page) != VMCS12_REVISION) { > - kunmap(page); > - nested_release_page_clean(page); > - nested_vmx_failInvalid(vcpu); > - return kvm_skip_emulated_instruction(vcpu); > - } > - kunmap(page); > - nested_release_page_clean(page); > - vmx->nested.vmxon_ptr = vmptr; > - break; > - case EXIT_REASON_VMCLEAR: > - if (!PAGE_ALIGNED(vmptr) || (vmptr >> maxphyaddr)) { > - nested_vmx_failValid(vcpu, > - VMXERR_VMCLEAR_INVALID_ADDRESS); > - return kvm_skip_emulated_instruction(vcpu); > - } > - > - if (vmptr == vmx->nested.vmxon_ptr) { > - nested_vmx_failValid(vcpu, > - VMXERR_VMCLEAR_VMXON_POINTER); > - return kvm_skip_emulated_instruction(vcpu); > - } > - break; > - case EXIT_REASON_VMPTRLD: > - if (!PAGE_ALIGNED(vmptr) || (vmptr >> maxphyaddr)) { > - nested_vmx_failValid(vcpu, > - VMXERR_VMPTRLD_INVALID_ADDRESS); > - return kvm_skip_emulated_instruction(vcpu); > - } > - > - if (vmptr == vmx->nested.vmxon_ptr) { > - nested_vmx_failValid(vcpu, > - VMXERR_VMPTRLD_VMXON_POINTER); > - return kvm_skip_emulated_instruction(vcpu); > - } > - break; > - default: > - return 1; /* shouldn't happen */ > - } > - > - if (vmpointer) > - *vmpointer = vmptr; > return 0; > } > > @@ -7066,6 +6990,8 @@ static int enter_vmx_operation(struct kvm_vcpu *vcpu) > static int handle_vmon(struct kvm_vcpu *vcpu) > { > int ret; > + gpa_t vmptr; > + struct page *page; > struct vcpu_vmx *vmx = to_vmx(vcpu); > const u64 VMXON_NEEDED_FEATURES = FEATURE_CONTROL_LOCKED > | FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX; > @@ -7095,9 +7021,37 @@ static int handle_vmon(struct kvm_vcpu *vcpu) > return 1; > } > > - if (nested_vmx_check_vmptr(vcpu, EXIT_REASON_VMON, NULL)) > + if (nested_vmx_get_vmptr(vcpu, &vmptr)) > return 1; > - > + > + /* > + * SDM 3: 24.11.5 > + * The first 4 bytes of VMXON region contain the supported > + * VMCS revision identifier > + * > + * Note - IA32_VMX_BASIC[48] will never be 1 for the nested case; > + * which replaces physical address width with 32 > + */ > + if (!PAGE_ALIGNED(vmptr) || (vmptr >> cpuid_maxphyaddr(vcpu))) { > + nested_vmx_failInvalid(vcpu); > + return kvm_skip_emulated_instruction(vcpu); > + } > + > + page = nested_get_page(vcpu, vmptr); > + if (page == NULL) { > + nested_vmx_failInvalid(vcpu); > + return kvm_skip_emulated_instruction(vcpu); > + } > + if (*(u32 *)kmap(page) != VMCS12_REVISION) { > + kunmap(page); > + nested_release_page_clean(page); > + nested_vmx_failInvalid(vcpu); > + return kvm_skip_emulated_instruction(vcpu); > + } > + kunmap(page); > + nested_release_page_clean(page); > + > + vmx->nested.vmxon_ptr = vmptr; > ret = enter_vmx_operation(vcpu); > if (ret) > return ret; > @@ -7213,9 +7167,19 @@ static int handle_vmclear(struct kvm_vcpu *vcpu) > if (!nested_vmx_check_permission(vcpu)) > return 1; > > - if (nested_vmx_check_vmptr(vcpu, EXIT_REASON_VMCLEAR, &vmptr)) > + if (nested_vmx_get_vmptr(vcpu, &vmptr)) > return 1; > > + if (!PAGE_ALIGNED(vmptr) || (vmptr >> cpuid_maxphyaddr(vcpu))) { > + nested_vmx_failValid(vcpu, VMXERR_VMCLEAR_INVALID_ADDRESS); > + return kvm_skip_emulated_instruction(vcpu); > + } > + > + if (vmptr == vmx->nested.vmxon_ptr) { > + nested_vmx_failValid(vcpu, VMXERR_VMCLEAR_VMXON_POINTER); > + return kvm_skip_emulated_instruction(vcpu); > + } > + > if (vmptr == vmx->nested.current_vmptr) > nested_release_vmcs12(vmx); > > @@ -7545,9 +7509,19 @@ static int handle_vmptrld(struct kvm_vcpu *vcpu) > if (!nested_vmx_check_permission(vcpu)) > return 1; > > - if (nested_vmx_check_vmptr(vcpu, EXIT_REASON_VMPTRLD, &vmptr)) > + if (nested_vmx_get_vmptr(vcpu, &vmptr)) > return 1; > > + if (!PAGE_ALIGNED(vmptr) || (vmptr >> cpuid_maxphyaddr(vcpu))) { > + nested_vmx_failValid(vcpu, VMXERR_VMPTRLD_INVALID_ADDRESS); > + return kvm_skip_emulated_instruction(vcpu); > + } > + > + if (vmptr == vmx->nested.vmxon_ptr) { > + nested_vmx_failValid(vcpu, VMXERR_VMPTRLD_VMXON_POINTER); > + return kvm_skip_emulated_instruction(vcpu); > + } > + > if (vmx->nested.current_vmptr != vmptr) { > struct vmcs12 *new_vmcs12; > struct page *page; >
Nice, diffstat speaks for itself. Queuing it. Thanks, Paolo
