* [patch] KVM: always initialize *pdata in get_msr()
@ 2015-01-23 14:32 ` Dan Carpenter
0 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2015-01-23 14:32 UTC (permalink / raw)
To: Gleb Natapov
Cc: Paolo Bonzini, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
kvm, kernel-janitors
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 <dan.carpenter@oracle.com>
---
From static analysis only, not from real life.
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [patch] KVM: always initialize *pdata in get_msr()
@ 2015-01-23 14:32 ` Dan Carpenter
0 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2015-01-23 14:32 UTC (permalink / raw)
To: Gleb Natapov
Cc: Paolo Bonzini, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
kvm, kernel-janitors
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 <dan.carpenter@oracle.com>
---
>From static analysis only, not from real life.
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [patch] KVM: always initialize *pdata in get_msr()
2015-01-23 14:32 ` Dan Carpenter
@ 2015-01-23 16:08 ` Radim Krčmář
-1 siblings, 0 replies; 8+ messages in thread
From: Radim Krčmář @ 2015-01-23 16:08 UTC (permalink / raw)
To: Dan Carpenter
Cc: Gleb Natapov, Paolo Bonzini, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, x86, kvm, kernel-janitors
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ář <rkrcmar@redhat.com>
> +++ 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;
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch] KVM: always initialize *pdata in get_msr()
@ 2015-01-23 16:08 ` Radim Krčmář
0 siblings, 0 replies; 8+ messages in thread
From: Radim Krčmář @ 2015-01-23 16:08 UTC (permalink / raw)
To: Dan Carpenter
Cc: Gleb Natapov, Paolo Bonzini, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, x86, kvm, kernel-janitors
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ář <rkrcmar@redhat.com>
> +++ 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;
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch] KVM: always initialize *pdata in get_msr()
2015-01-23 14:32 ` Dan Carpenter
@ 2015-01-23 17:50 ` Paolo Bonzini
-1 siblings, 0 replies; 8+ messages in thread
From: Paolo Bonzini @ 2015-01-23 17:50 UTC (permalink / raw)
To: Dan Carpenter, Gleb Natapov
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, kvm, kernel-janitors
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 <dan.carpenter@oracle.com>
> ---
> From static analysis only, not from real life.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch] KVM: always initialize *pdata in get_msr()
@ 2015-01-23 17:50 ` Paolo Bonzini
0 siblings, 0 replies; 8+ messages in thread
From: Paolo Bonzini @ 2015-01-23 17:50 UTC (permalink / raw)
To: Dan Carpenter, Gleb Natapov
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, kvm, kernel-janitors
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 <dan.carpenter@oracle.com>
> ---
> From static analysis only, not from real life.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch] KVM: always initialize *pdata in get_msr()
2015-01-23 16:08 ` Radim Krčmář
@ 2015-01-24 9:26 ` Dan Carpenter
-1 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2015-01-24 9:26 UTC (permalink / raw)
To: Radim Krčmář
Cc: Gleb Natapov, Paolo Bonzini, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, x86, kvm, kernel-janitors
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch] KVM: always initialize *pdata in get_msr()
@ 2015-01-24 9:26 ` Dan Carpenter
0 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2015-01-24 9:26 UTC (permalink / raw)
To: Radim Krčmář
Cc: Gleb Natapov, Paolo Bonzini, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, x86, kvm, kernel-janitors
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 kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-01-24 9:26 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-23 14:32 [patch] KVM: always initialize *pdata in get_msr() Dan Carpenter
2015-01-23 14:32 ` Dan Carpenter
2015-01-23 16:08 ` Radim Krčmář
2015-01-23 16:08 ` Radim Krčmář
2015-01-24 9:26 ` Dan Carpenter
2015-01-24 9:26 ` Dan Carpenter
2015-01-23 17:50 ` Paolo Bonzini
2015-01-23 17:50 ` Paolo Bonzini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.