KVM: always initialize *pdata in get_msr()

Commit Message

Dan Carpenter Jan. 23, 2015, 2:32 p.m. UTC

Smatch complains that there are some paths where we use uninitialized
data in em_sysenter().

        arch/x86/kvm/emulate.c:2410 em_sysenter()
        error: potentially using uninitialized 'msr_data'.

A couple examples of paths which don't set "pdata" are found in
get_msr_hyperv() and kvm_x2apic_msr_read().  I looked at this code and
it seems like setting it to zero is a common default behaviour.

Signed-off-by: Dan Carpenter <[e-mail]>
---
From static analysis only, not from real life.

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Radim Krčmář Jan. 23, 2015, 4:08 p.m. UTC | #1

2015-01-23 17:32+0300, Dan Carpenter:
> Smatch complains that there are some paths where we use uninitialized
> data in em_sysenter().
> 
>         arch/x86/kvm/emulate.c:2410 em_sysenter()
>         error: potentially using uninitialized 'msr_data'.
> 

I have two hypotheses why smatch thinks so
 1) vmcs_readl() return value is treated as potentially uninitialized
 2) even non-reachable switch cases are considered

Both of them would likely throw other false positives and I think the
rest is covered in analysis at the bottom ... have I missed something?
(I presume that this patch fixes that message.)

> A couple examples of paths which don't set "pdata" are found in
> get_msr_hyperv() and kvm_x2apic_msr_read().  I looked at this code and
> it seems like setting it to zero is a common default behaviour.

This patch will prevent future detection of functional bugs :(
But those bugs could be CVEs (the fear!) and zero is sane,

Reviewed-by: Radim Kr?má? <[e-mail]>

> +++ b/arch/x86/kvm/svm.c
> @@ -3063,6 +3063,8 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, unsigned ecx, u64 *data)
>  {
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  
> +	*data = 0;

(Not sure why we don't check data for NULL too, or don't remove the
 check from vmx_get_msr() ...)


---
msr_data is always initialized.
It is set with

  ops->get_msr(ctxt, MSR_IA32_SYSENTER_CS, &msr_data);
   emulator_get_msr(ctxt, MSR_IA32_SYSENTER_CS, &msr_data);
    kvm_get_msr(emul_to_vcpu(ctxt), MSR_IA32_SYSENTER_CS, &msr_data);
     kvm_x86_ops->get_msr(emul_to_vcpu(ctxt), MSR_IA32_SYSENTER_CS, &msr_data);

where kvm_x86_ops->get_msr is either svm_get_msr or vmx_get_msr.
(If smatch thinks that it can also be something else, fixing it there
 won't shut it up.)

svm_get_msr: msr_data is always initialized.

  svm_get_msr(struct kvm_vcpu *vcpu, unsigned ecx, u64 *data) {
    [...]
    switch (ecx) {
    [...]
    case MSR_IA32_SYSENTER_CS:
      *data = svm->vmcb->save.sysenter_cs;
      break;
    [...]
    }

    return 0;
  }

vmx_get_msr: msr_data can only remain uninitialized if msr_data is
located at memory offset 0.  (Won't happen in Linux and still wouldn't
suppress the warning, because the patch sets msr_data after the check.)

  vmx_get_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata) {
  [...]
  if (!pdata) {
    [...]
    return -EINVAL;
  }

  switch (msr_index) {
  [...]
  case MSR_IA32_SYSENTER_CS:
    data = vmcs_read32(GUEST_SYSENTER_CS);
    break;
  [...]
  }

  *pdata = data;
  return 0;
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Paolo Bonzini Jan. 23, 2015, 5:50 p.m. UTC | #2

On 23/01/2015 15:32, Dan Carpenter wrote:
> Smatch complains that there are some paths where we use uninitialized
> data in em_sysenter().
> 
>         arch/x86/kvm/emulate.c:2410 em_sysenter()
>         error: potentially using uninitialized 'msr_data'.

The right thing to do is to ensure that the value is set if get_msr
returns 0.  If it returns 1, msr_data is not used.

This one is obviously a false positive, so I'm not applying the patch
for now.

Paolo

> A couple examples of paths which don't set "pdata" are found in
> get_msr_hyperv() and kvm_x2apic_msr_read().  I looked at this code and
> it seems like setting it to zero is a common default behaviour.
> 
> Signed-off-by: Dan Carpenter <[e-mail]>
> ---
> From static analysis only, not from real life.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dan Carpenter Jan. 24, 2015, 9:26 a.m. UTC | #3

On Fri, Jan 23, 2015 at 05:08:03PM +0100, Radim Kr?má? wrote:
> This patch will prevent future detection of functional bugs :(

Hm.  You're right.  Smatch should be able to follow the logic.  The code
to handle this is pretty new.  I'll look into why it wasn't working.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index a17d848..f0e30d2 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -3063,6 +3063,8 @@  static int svm_get_msr(struct kvm_vcpu *vcpu, unsigned ecx, u64 *data)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
 
+	*data = 0;
+
 	switch (ecx) {
 	case MSR_IA32_TSC: {
 		*data = svm->vmcb->control.tsc_offset +
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index ce35071..3bdb9ca 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2544,6 +2544,7 @@  static int vmx_get_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata)
 		printk(KERN_ERR "BUG: get_msr called with NULL pdata\n");
 		return -EINVAL;
 	}
+	*pdata = 0;
 
 	switch (msr_index) {
 #ifdef CONFIG_X86_64