Skip to contentSkip to conversation

KVM: always initialize *pdata in get_msr()

PATCH In review Dan Carpenter 3 participants 4 messages Jan 23, 2015 – Jan 24, 2015

Subsystems (4)
x86 KVM Kernel janitors KVM x86
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 KVM: always initialize *pdata in get_msr()

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 <[email protected]>

---
From static analysis only, not from real life.

arch/x86/kvm/svm.c +2 -0
index a17d848..f0e30d2 100644
@@ -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 +
arch/x86/kvm/vmx.c +1 -0
index ce35071..3bdb9ca 100644
@@ -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
Reviewed-by on v1 1/1
#1 lore raw

2015-01-23 17:32+0300, Dan Carpenter:

Quoted 6 lines

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.)

Quoted 3 lines

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ář <[email protected]>

arch/x86/kvm/svm.c static int svm_get_msr(struct kvm_vcpu *vcpu, unsigned ecx, u64 *data) · line 3066

Quoted 6 lines

+++ b/arch/x86/kvm/svm.c

arch/x86/kvm/svm.c +1 -0
@@ -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;
author
#2 lore raw
Quoted 1 line · Radim Krčmář

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

maintainer
#3 lore raw
In reply to Dan Carpenter
Quoted 5 lines · 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'.

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

Quoted 7 lines

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 <[email protected]>

---
From static analysis only, not from real life.