diff mbox

[for-1.4,v2] target-i386: kvm: prevent buffer overflow if -cpu foo, [x]level is too big

Message ID 1359373766-19201-1-git-send-email-imammedo@redhat.com
State New
Headers show

Commit Message

Igor Mammedov Jan. 28, 2013, 11:49 a.m. UTC 
Stack corruption may occur if too big 'level' or 'xlevel' values passed
on command line with KVM enabled, due to limited size of cpuid_data
in kvm_arch_init_vcpu().

reproduces with:
 qemu -enable-kvm -cpu qemu64,level=4294967295
or
 qemu -enable-kvm -cpu qemu64,xlevel=4294967295

Check if there is space in cpuid_data before passing it to cpu_x86_cpuid()
or abort() if there is not space.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
  * v2:
    * use macro instead of const int max_cpuid_entries to fix build breakage
      in C99 mode. Suggested-By: Laszlo Ersek <lersek@redhat.com>
    * compare with array index instead of address of the last element
          Sugested-By: Marcelo Tosatti <mtosatti@redhat.com>

---
 target-i386/kvm.c |   25 ++++++++++++++++++++++++-
 1 files changed, 24 insertions(+), 1 deletions(-)

Comments

Laszlo Ersek Jan. 28, 2013, 12:01 p.m. UTC | #1 
On 01/28/13 12:49, Igor Mammedov wrote:
> Stack corruption may occur if too big 'level' or 'xlevel' values passed
> on command line with KVM enabled, due to limited size of cpuid_data
> in kvm_arch_init_vcpu().
> 
> reproduces with:
>  qemu -enable-kvm -cpu qemu64,level=4294967295
> or
>  qemu -enable-kvm -cpu qemu64,xlevel=4294967295
> 
> Check if there is space in cpuid_data before passing it to cpu_x86_cpuid()
> or abort() if there is not space.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>   * v2:
>     * use macro instead of const int max_cpuid_entries to fix build breakage
>       in C99 mode. Suggested-By: Laszlo Ersek <lersek@redhat.com>
>     * compare with array index instead of address of the last element
>           Sugested-By: Marcelo Tosatti <mtosatti@redhat.com>
> 
> ---
>  target-i386/kvm.c |   25 ++++++++++++++++++++++++-
>  1 files changed, 24 insertions(+), 1 deletions(-)

Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Andreas Färber Jan. 28, 2013, 1:37 p.m. UTC | #2 
Am 28.01.2013 12:49, schrieb Igor Mammedov:
> Stack corruption may occur if too big 'level' or 'xlevel' values passed
> on command line with KVM enabled, due to limited size of cpuid_data
> in kvm_arch_init_vcpu().
> 
> reproduces with:
>  qemu -enable-kvm -cpu qemu64,level=4294967295
> or
>  qemu -enable-kvm -cpu qemu64,xlevel=4294967295
> 
> Check if there is space in cpuid_data before passing it to cpu_x86_cpuid()
> or abort() if there is not space.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Andreas Färber <afaerber@suse.de>

CC'ing Gleb and KVM list.

Andreas

> ---
>   * v2:
>     * use macro instead of const int max_cpuid_entries to fix build breakage
>       in C99 mode. Suggested-By: Laszlo Ersek <lersek@redhat.com>
>     * compare with array index instead of address of the last element
>           Sugested-By: Marcelo Tosatti <mtosatti@redhat.com>
> 
> ---
>  target-i386/kvm.c |   25 ++++++++++++++++++++++++-
>  1 files changed, 24 insertions(+), 1 deletions(-)
> 
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 3acff40..4ecb728 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -411,11 +411,12 @@ static void cpu_update_state(void *opaque, int running, RunState state)
>      }
>  }
>  
> +#define KVM_MAX_CPUID_ENTRIES  100
>  int kvm_arch_init_vcpu(CPUState *cs)
>  {
>      struct {
>          struct kvm_cpuid2 cpuid;
> -        struct kvm_cpuid_entry2 entries[100];
> +        struct kvm_cpuid_entry2 entries[KVM_MAX_CPUID_ENTRIES];
>      } QEMU_PACKED cpuid_data;
>      X86CPU *cpu = X86_CPU(cs);
>      CPUX86State *env = &cpu->env;
> @@ -502,6 +503,10 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      cpu_x86_cpuid(env, 0, 0, &limit, &unused, &unused, &unused);
>  
>      for (i = 0; i <= limit; i++) {
> +        if (cpuid_i == KVM_MAX_CPUID_ENTRIES) {
> +            fprintf(stderr, "unsupported level value: 0x%x\n", limit);
> +            abort();
> +        }
>          c = &cpuid_data.entries[cpuid_i++];
>  
>          switch (i) {
> @@ -516,6 +521,11 @@ int kvm_arch_init_vcpu(CPUState *cs)
>              times = c->eax & 0xff;
>  
>              for (j = 1; j < times; ++j) {
> +                if (cpuid_i == KVM_MAX_CPUID_ENTRIES) {
> +                    fprintf(stderr, "cpuid_data is full, no space for "
> +                            "cpuid(eax:2):eax & 0xf = 0x%x\n", times);
> +                    abort();
> +                }
>                  c = &cpuid_data.entries[cpuid_i++];
>                  c->function = i;
>                  c->flags = KVM_CPUID_FLAG_STATEFUL_FUNC;
> @@ -544,6 +554,11 @@ int kvm_arch_init_vcpu(CPUState *cs)
>                  if (i == 0xd && c->eax == 0) {
>                      continue;
>                  }
> +                if (cpuid_i == KVM_MAX_CPUID_ENTRIES) {
> +                    fprintf(stderr, "cpuid_data is full, no space for "
> +                            "cpuid(eax:0x%x,ecx:0x%x)\n", i, j);
> +                    abort();
> +                }
>                  c = &cpuid_data.entries[cpuid_i++];
>              }
>              break;
> @@ -557,6 +572,10 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      cpu_x86_cpuid(env, 0x80000000, 0, &limit, &unused, &unused, &unused);
>  
>      for (i = 0x80000000; i <= limit; i++) {
> +        if (cpuid_i == KVM_MAX_CPUID_ENTRIES) {
> +            fprintf(stderr, "unsupported xlevel value: 0x%x\n", limit);
> +            abort();
> +        }
>          c = &cpuid_data.entries[cpuid_i++];
>  
>          c->function = i;
> @@ -569,6 +588,10 @@ int kvm_arch_init_vcpu(CPUState *cs)
>          cpu_x86_cpuid(env, 0xC0000000, 0, &limit, &unused, &unused, &unused);
>  
>          for (i = 0xC0000000; i <= limit; i++) {
> +            if (cpuid_i == KVM_MAX_CPUID_ENTRIES) {
> +                fprintf(stderr, "unsupported xlevel2 value: 0x%x\n", limit);
> +                abort();
> +            }
>              c = &cpuid_data.entries[cpuid_i++];
>  
>              c->function = i;
>
Gleb Natapov Jan. 29, 2013, 8:13 a.m. UTC | #3 
On Mon, Jan 28, 2013 at 12:49:26PM +0100, Igor Mammedov wrote:
> Stack corruption may occur if too big 'level' or 'xlevel' values passed
> on command line with KVM enabled, due to limited size of cpuid_data
> in kvm_arch_init_vcpu().
> 
> reproduces with:
>  qemu -enable-kvm -cpu qemu64,level=4294967295
> or
>  qemu -enable-kvm -cpu qemu64,xlevel=4294967295
> 
> Check if there is space in cpuid_data before passing it to cpu_x86_cpuid()
> or abort() if there is not space.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Applied to uq/master, thanks.

> ---
>   * v2:
>     * use macro instead of const int max_cpuid_entries to fix build breakage
>       in C99 mode. Suggested-By: Laszlo Ersek <lersek@redhat.com>
>     * compare with array index instead of address of the last element
>           Sugested-By: Marcelo Tosatti <mtosatti@redhat.com>
> 
> ---
>  target-i386/kvm.c |   25 ++++++++++++++++++++++++-
>  1 files changed, 24 insertions(+), 1 deletions(-)
> 
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 3acff40..4ecb728 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -411,11 +411,12 @@ static void cpu_update_state(void *opaque, int running, RunState state)
>      }
>  }
>  
> +#define KVM_MAX_CPUID_ENTRIES  100
>  int kvm_arch_init_vcpu(CPUState *cs)
>  {
>      struct {
>          struct kvm_cpuid2 cpuid;
> -        struct kvm_cpuid_entry2 entries[100];
> +        struct kvm_cpuid_entry2 entries[KVM_MAX_CPUID_ENTRIES];
>      } QEMU_PACKED cpuid_data;
>      X86CPU *cpu = X86_CPU(cs);
>      CPUX86State *env = &cpu->env;
> @@ -502,6 +503,10 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      cpu_x86_cpuid(env, 0, 0, &limit, &unused, &unused, &unused);
>  
>      for (i = 0; i <= limit; i++) {
> +        if (cpuid_i == KVM_MAX_CPUID_ENTRIES) {
> +            fprintf(stderr, "unsupported level value: 0x%x\n", limit);
> +            abort();
> +        }
>          c = &cpuid_data.entries[cpuid_i++];
>  
>          switch (i) {
> @@ -516,6 +521,11 @@ int kvm_arch_init_vcpu(CPUState *cs)
>              times = c->eax & 0xff;
>  
>              for (j = 1; j < times; ++j) {
> +                if (cpuid_i == KVM_MAX_CPUID_ENTRIES) {
> +                    fprintf(stderr, "cpuid_data is full, no space for "
> +                            "cpuid(eax:2):eax & 0xf = 0x%x\n", times);
> +                    abort();
> +                }
>                  c = &cpuid_data.entries[cpuid_i++];
>                  c->function = i;
>                  c->flags = KVM_CPUID_FLAG_STATEFUL_FUNC;
> @@ -544,6 +554,11 @@ int kvm_arch_init_vcpu(CPUState *cs)
>                  if (i == 0xd && c->eax == 0) {
>                      continue;
>                  }
> +                if (cpuid_i == KVM_MAX_CPUID_ENTRIES) {
> +                    fprintf(stderr, "cpuid_data is full, no space for "
> +                            "cpuid(eax:0x%x,ecx:0x%x)\n", i, j);
> +                    abort();
> +                }
>                  c = &cpuid_data.entries[cpuid_i++];
>              }
>              break;
> @@ -557,6 +572,10 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      cpu_x86_cpuid(env, 0x80000000, 0, &limit, &unused, &unused, &unused);
>  
>      for (i = 0x80000000; i <= limit; i++) {
> +        if (cpuid_i == KVM_MAX_CPUID_ENTRIES) {
> +            fprintf(stderr, "unsupported xlevel value: 0x%x\n", limit);
> +            abort();
> +        }
>          c = &cpuid_data.entries[cpuid_i++];
>  
>          c->function = i;
> @@ -569,6 +588,10 @@ int kvm_arch_init_vcpu(CPUState *cs)
>          cpu_x86_cpuid(env, 0xC0000000, 0, &limit, &unused, &unused, &unused);
>  
>          for (i = 0xC0000000; i <= limit; i++) {
> +            if (cpuid_i == KVM_MAX_CPUID_ENTRIES) {
> +                fprintf(stderr, "unsupported xlevel2 value: 0x%x\n", limit);
> +                abort();
> +            }
>              c = &cpuid_data.entries[cpuid_i++];
>  
>              c->function = i;
> -- 
> 1.7.1
> 

--
			Gleb.
diff mbox

Patch

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 3acff40..4ecb728 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -411,11 +411,12 @@  static void cpu_update_state(void *opaque, int running, RunState state)
     }
 }
 
+#define KVM_MAX_CPUID_ENTRIES  100
 int kvm_arch_init_vcpu(CPUState *cs)
 {
     struct {
         struct kvm_cpuid2 cpuid;
-        struct kvm_cpuid_entry2 entries[100];
+        struct kvm_cpuid_entry2 entries[KVM_MAX_CPUID_ENTRIES];
     } QEMU_PACKED cpuid_data;
     X86CPU *cpu = X86_CPU(cs);
     CPUX86State *env = &cpu->env;
@@ -502,6 +503,10 @@  int kvm_arch_init_vcpu(CPUState *cs)
     cpu_x86_cpuid(env, 0, 0, &limit, &unused, &unused, &unused);
 
     for (i = 0; i <= limit; i++) {
+        if (cpuid_i == KVM_MAX_CPUID_ENTRIES) {
+            fprintf(stderr, "unsupported level value: 0x%x\n", limit);
+            abort();
+        }
         c = &cpuid_data.entries[cpuid_i++];
 
         switch (i) {
@@ -516,6 +521,11 @@  int kvm_arch_init_vcpu(CPUState *cs)
             times = c->eax & 0xff;
 
             for (j = 1; j < times; ++j) {
+                if (cpuid_i == KVM_MAX_CPUID_ENTRIES) {
+                    fprintf(stderr, "cpuid_data is full, no space for "
+                            "cpuid(eax:2):eax & 0xf = 0x%x\n", times);
+                    abort();
+                }
                 c = &cpuid_data.entries[cpuid_i++];
                 c->function = i;
                 c->flags = KVM_CPUID_FLAG_STATEFUL_FUNC;
@@ -544,6 +554,11 @@  int kvm_arch_init_vcpu(CPUState *cs)
                 if (i == 0xd && c->eax == 0) {
                     continue;
                 }
+                if (cpuid_i == KVM_MAX_CPUID_ENTRIES) {
+                    fprintf(stderr, "cpuid_data is full, no space for "
+                            "cpuid(eax:0x%x,ecx:0x%x)\n", i, j);
+                    abort();
+                }
                 c = &cpuid_data.entries[cpuid_i++];
             }
             break;
@@ -557,6 +572,10 @@  int kvm_arch_init_vcpu(CPUState *cs)
     cpu_x86_cpuid(env, 0x80000000, 0, &limit, &unused, &unused, &unused);
 
     for (i = 0x80000000; i <= limit; i++) {
+        if (cpuid_i == KVM_MAX_CPUID_ENTRIES) {
+            fprintf(stderr, "unsupported xlevel value: 0x%x\n", limit);
+            abort();
+        }
         c = &cpuid_data.entries[cpuid_i++];
 
         c->function = i;
@@ -569,6 +588,10 @@  int kvm_arch_init_vcpu(CPUState *cs)
         cpu_x86_cpuid(env, 0xC0000000, 0, &limit, &unused, &unused, &unused);
 
         for (i = 0xC0000000; i <= limit; i++) {
+            if (cpuid_i == KVM_MAX_CPUID_ENTRIES) {
+                fprintf(stderr, "unsupported xlevel2 value: 0x%x\n", limit);
+                abort();
+            }
             c = &cpuid_data.entries[cpuid_i++];
 
             c->function = i;