[bug report] KVM: nVMX: move vmclear and vmptrld pre-checks to nested_vmx_check_vmptr

[bug report] KVM: nVMX: move vmclear and vmptrld pre-checks to nested_vmx_check_vmptr

[Dan Carpenter]

Hello Bandan Das,

The patch 4291b58885f5: "KVM: nVMX: move vmclear and vmptrld
pre-checks to nested_vmx_check_vmptr" from May 6, 2014, leads to the
following static checker warning:

	arch/x86/kvm/vmx.c:7219 handle_vmclear()
	error: uninitialized symbol 'vmptr'.

arch/x86/kvm/vmx.c
  7206  /* Emulate the VMCLEAR instruction */
  7207  static int handle_vmclear(struct kvm_vcpu *vcpu)
  7208  {
  7209          struct vcpu_vmx *vmx = to_vmx(vcpu);
  7210          u32 zero = 0;
  7211          gpa_t vmptr;
  7212  
  7213          if (!nested_vmx_check_permission(vcpu))
  7214                  return 1;
  7215  
  7216          if (nested_vmx_check_vmptr(vcpu, EXIT_REASON_VMCLEAR, &vmptr))
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
There are several success paths which don't initialize vmptr.  This feels
like a quite serious bug, but maybe handle_vmclear() is not used often
so it doesn't show up in testing?  I don't know.  Or it could be that this
code is just too complicated for my static checker and for my cursory analysis.

  7217                  return 1;
  7218  
  7219          if (vmptr == vmx->nested.current_vmptr)
  7220                  nested_release_vmcs12(vmx);
  7221  
  7222          kvm_vcpu_write_guest(vcpu,
  7223                          vmptr + offsetof(struct vmcs12, launch_state),
                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

  7224                          &zero, sizeof(zero));
  7225  
  7226          nested_free_vmcs02(vmx, vmptr);
  7227  
  7228          nested_vmx_succeed(vcpu);
  7229          return kvm_skip_emulated_instruction(vcpu);
  7230  }

Similar issue in:
arch/x86/kvm/vmx.c:7551 handle_vmptrld() error: uninitialized symbol 'vmptr'.

regards,
dan carpenter

Re: [bug report] KVM: nVMX: move vmclear and vmptrld pre-checks to nested_vmx_check_vmptr

[Radim Krčmář]

2017-05-18 10:41+0300, Dan Carpenter:
> Hello Bandan Das,
> 
> The patch 4291b58885f5: "KVM: nVMX: move vmclear and vmptrld
> pre-checks to nested_vmx_check_vmptr" from May 6, 2014, leads to the
> following static checker warning:
> 
> 	arch/x86/kvm/vmx.c:7219 handle_vmclear()
> 	error: uninitialized symbol 'vmptr'.
> 
> arch/x86/kvm/vmx.c
>   7206  /* Emulate the VMCLEAR instruction */
>   7207  static int handle_vmclear(struct kvm_vcpu *vcpu)
>   7208  {
>   7209          struct vcpu_vmx *vmx = to_vmx(vcpu);
>   7210          u32 zero = 0;
>   7211          gpa_t vmptr;
>   7212  
>   7213          if (!nested_vmx_check_permission(vcpu))
>   7214                  return 1;
>   7215  
>   7216          if (nested_vmx_check_vmptr(vcpu, EXIT_REASON_VMCLEAR, &vmptr))
>                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> There are several success paths which don't initialize vmptr.  This feels
> like a quite serious bug, but maybe handle_vmclear() is not used often
> so it doesn't show up in testing?  I don't know.  Or it could be that this
> code is just too complicated for my static checker and for my cursory analysis.

The success path can only happen when single-stepping the guest from
userspace (a rare scenario), which is why it didn't show up in testing.

I'll prepare a patch, thanks for all the reports.

>   7217                  return 1;
>   7218  
>   7219          if (vmptr == vmx->nested.current_vmptr)
>   7220                  nested_release_vmcs12(vmx);
>   7221  
>   7222          kvm_vcpu_write_guest(vcpu,
>   7223                          vmptr + offsetof(struct vmcs12, launch_state),
>                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
>   7224                          &zero, sizeof(zero));
>   7225  
>   7226          nested_free_vmcs02(vmx, vmptr);
>   7227  
>   7228          nested_vmx_succeed(vcpu);
>   7229          return kvm_skip_emulated_instruction(vcpu);
>   7230  }
> 
> Similar issue in:
> arch/x86/kvm/vmx.c:7551 handle_vmptrld() error: uninitialized symbol 'vmptr'.
> 
> regards,
> dan carpenter