Patchwork [for,1.7] kvm: Fix uninitialized cpuid_data

login
register
mail settings
Submitter Stefan Weil
Date Nov. 6, 2013, 9:35 p.m.
Message ID <1383773727-29327-1-git-send-email-sw@weilnetz.de>
Download mbox | patch
Permalink /patch/289029/
State Accepted
Headers show

Comments

Stefan Weil - Nov. 6, 2013, 9:35 p.m.
This error was reported by valgrind when running qemu-system-x86_64
with kvm:

==975== Conditional jump or move depends on uninitialised value(s)
==975==    at 0x521C38: cpuid_find_entry (kvm.c:176)
==975==    by 0x5235BA: kvm_arch_init_vcpu (kvm.c:686)
==975==    by 0x4D5175: kvm_init_vcpu (kvm-all.c:267)
==975==    by 0x45035B: qemu_kvm_cpu_thread_fn (cpus.c:858)
==975==    by 0xD361E0D: start_thread (pthread_create.c:311)
==975==    by 0xD65E9EC: clone (clone.S:113)
==975==  Uninitialised value was created by a stack allocation
==975==    at 0x5226E4: kvm_arch_init_vcpu (kvm.c:446)

Instead of adding more memset calls for parts of cpuid_data, the existing
calls were removed and cpuid_data is now initialized completely in one
call.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---

I did not check whether older versions also need this fix.

Stefan W.

 target-i386/kvm.c |    9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)
Gleb Natapov - Nov. 7, 2013, 11:15 a.m.
On Wed, Nov 06, 2013 at 10:35:27PM +0100, Stefan Weil wrote:
> This error was reported by valgrind when running qemu-system-x86_64
> with kvm:
> 
> ==975== Conditional jump or move depends on uninitialised value(s)
> ==975==    at 0x521C38: cpuid_find_entry (kvm.c:176)
> ==975==    by 0x5235BA: kvm_arch_init_vcpu (kvm.c:686)
> ==975==    by 0x4D5175: kvm_init_vcpu (kvm-all.c:267)
> ==975==    by 0x45035B: qemu_kvm_cpu_thread_fn (cpus.c:858)
> ==975==    by 0xD361E0D: start_thread (pthread_create.c:311)
> ==975==    by 0xD65E9EC: clone (clone.S:113)
> ==975==  Uninitialised value was created by a stack allocation
> ==975==    at 0x5226E4: kvm_arch_init_vcpu (kvm.c:446)
> 
> Instead of adding more memset calls for parts of cpuid_data, the existing
> calls were removed and cpuid_data is now initialized completely in one
> call.
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
Applied, thanks.

> ---
> 
> I did not check whether older versions also need this fix.
> 
> Stefan W.
> 
>  target-i386/kvm.c |    9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 749aa09..3ffd3e0 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -456,11 +456,12 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      uint32_t signature[3];
>      int r;
>  
> +    memset(&cpuid_data, 0, sizeof(cpuid_data));
> +
>      cpuid_i = 0;
>  
>      /* Paravirtualization CPUIDs */
>      c = &cpuid_data.entries[cpuid_i++];
> -    memset(c, 0, sizeof(*c));
>      c->function = KVM_CPUID_SIGNATURE;
>      if (!hyperv_enabled(cpu)) {
>          memcpy(signature, "KVMKVMKVM\0\0\0", 12);
> @@ -474,7 +475,6 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      c->edx = signature[2];
>  
>      c = &cpuid_data.entries[cpuid_i++];
> -    memset(c, 0, sizeof(*c));
>      c->function = KVM_CPUID_FEATURES;
>      c->eax = env->features[FEAT_KVM];
>  
> @@ -483,13 +483,11 @@ int kvm_arch_init_vcpu(CPUState *cs)
>          c->eax = signature[0];
>  
>          c = &cpuid_data.entries[cpuid_i++];
> -        memset(c, 0, sizeof(*c));
>          c->function = HYPERV_CPUID_VERSION;
>          c->eax = 0x00001bbc;
>          c->ebx = 0x00060001;
>  
>          c = &cpuid_data.entries[cpuid_i++];
> -        memset(c, 0, sizeof(*c));
>          c->function = HYPERV_CPUID_FEATURES;
>          if (cpu->hyperv_relaxed_timing) {
>              c->eax |= HV_X64_MSR_HYPERCALL_AVAILABLE;
> @@ -500,7 +498,6 @@ int kvm_arch_init_vcpu(CPUState *cs)
>          }
>  
>          c = &cpuid_data.entries[cpuid_i++];
> -        memset(c, 0, sizeof(*c));
>          c->function = HYPERV_CPUID_ENLIGHTMENT_INFO;
>          if (cpu->hyperv_relaxed_timing) {
>              c->eax |= HV_X64_RELAXED_TIMING_RECOMMENDED;
> @@ -511,13 +508,11 @@ int kvm_arch_init_vcpu(CPUState *cs)
>          c->ebx = cpu->hyperv_spinlock_attempts;
>  
>          c = &cpuid_data.entries[cpuid_i++];
> -        memset(c, 0, sizeof(*c));
>          c->function = HYPERV_CPUID_IMPLEMENT_LIMITS;
>          c->eax = 0x40;
>          c->ebx = 0x40;
>  
>          c = &cpuid_data.entries[cpuid_i++];
> -        memset(c, 0, sizeof(*c));
>          c->function = KVM_CPUID_SIGNATURE_NEXT;
>          memcpy(signature, "KVMKVMKVM\0\0\0", 12);
>          c->eax = 0;
> -- 
> 1.7.10.4

--
			Gleb.
Stefan Weil - Nov. 20, 2013, 6:54 p.m.
Am 07.11.2013 12:15, schrieb Gleb Natapov:
> On Wed, Nov 06, 2013 at 10:35:27PM +0100, Stefan Weil wrote:
>> This error was reported by valgrind when running qemu-system-x86_64
>> with kvm:
>>
>> ==975== Conditional jump or move depends on uninitialised value(s)
>> ==975==    at 0x521C38: cpuid_find_entry (kvm.c:176)
>> ==975==    by 0x5235BA: kvm_arch_init_vcpu (kvm.c:686)
>> ==975==    by 0x4D5175: kvm_init_vcpu (kvm-all.c:267)
>> ==975==    by 0x45035B: qemu_kvm_cpu_thread_fn (cpus.c:858)
>> ==975==    by 0xD361E0D: start_thread (pthread_create.c:311)
>> ==975==    by 0xD65E9EC: clone (clone.S:113)
>> ==975==  Uninitialised value was created by a stack allocation
>> ==975==    at 0x5226E4: kvm_arch_init_vcpu (kvm.c:446)
>>
>> Instead of adding more memset calls for parts of cpuid_data, the existing
>> calls were removed and cpuid_data is now initialized completely in one
>> call.
>>
>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> Applied, thanks.


Ping. This bug fix for KVM is still missing in QEMU 1.7.

Regards,
Stefan

Patch

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 749aa09..3ffd3e0 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -456,11 +456,12 @@  int kvm_arch_init_vcpu(CPUState *cs)
     uint32_t signature[3];
     int r;
 
+    memset(&cpuid_data, 0, sizeof(cpuid_data));
+
     cpuid_i = 0;
 
     /* Paravirtualization CPUIDs */
     c = &cpuid_data.entries[cpuid_i++];
-    memset(c, 0, sizeof(*c));
     c->function = KVM_CPUID_SIGNATURE;
     if (!hyperv_enabled(cpu)) {
         memcpy(signature, "KVMKVMKVM\0\0\0", 12);
@@ -474,7 +475,6 @@  int kvm_arch_init_vcpu(CPUState *cs)
     c->edx = signature[2];
 
     c = &cpuid_data.entries[cpuid_i++];
-    memset(c, 0, sizeof(*c));
     c->function = KVM_CPUID_FEATURES;
     c->eax = env->features[FEAT_KVM];
 
@@ -483,13 +483,11 @@  int kvm_arch_init_vcpu(CPUState *cs)
         c->eax = signature[0];
 
         c = &cpuid_data.entries[cpuid_i++];
-        memset(c, 0, sizeof(*c));
         c->function = HYPERV_CPUID_VERSION;
         c->eax = 0x00001bbc;
         c->ebx = 0x00060001;
 
         c = &cpuid_data.entries[cpuid_i++];
-        memset(c, 0, sizeof(*c));
         c->function = HYPERV_CPUID_FEATURES;
         if (cpu->hyperv_relaxed_timing) {
             c->eax |= HV_X64_MSR_HYPERCALL_AVAILABLE;
@@ -500,7 +498,6 @@  int kvm_arch_init_vcpu(CPUState *cs)
         }
 
         c = &cpuid_data.entries[cpuid_i++];
-        memset(c, 0, sizeof(*c));
         c->function = HYPERV_CPUID_ENLIGHTMENT_INFO;
         if (cpu->hyperv_relaxed_timing) {
             c->eax |= HV_X64_RELAXED_TIMING_RECOMMENDED;
@@ -511,13 +508,11 @@  int kvm_arch_init_vcpu(CPUState *cs)
         c->ebx = cpu->hyperv_spinlock_attempts;
 
         c = &cpuid_data.entries[cpuid_i++];
-        memset(c, 0, sizeof(*c));
         c->function = HYPERV_CPUID_IMPLEMENT_LIMITS;
         c->eax = 0x40;
         c->ebx = 0x40;
 
         c = &cpuid_data.entries[cpuid_i++];
-        memset(c, 0, sizeof(*c));
         c->function = KVM_CPUID_SIGNATURE_NEXT;
         memcpy(signature, "KVMKVMKVM\0\0\0", 12);
         c->eax = 0;