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

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>
---
 target-i386/kvm.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

comments in-line

On 01/24/13 00:05, 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>
> ---
>  target-i386/kvm.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 3acff40..8885b22 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -413,10 +413,13 @@ static void cpu_update_state(void *opaque, int running, RunState state)
>  
>  int kvm_arch_init_vcpu(CPUState *cs)
>  {
> +    const int max_cpuid_entries = 100;
>      struct {
>          struct kvm_cpuid2 cpuid;
> -        struct kvm_cpuid_entry2 entries[100];
> +        struct kvm_cpuid_entry2 entries[max_cpuid_entries];
>      } QEMU_PACKED cpuid_data;

This does not conform to C99 (it would probably conform to ISO C++); it
violates 6.7.5.2 Array declarators:

2 Only an ordinary identifier (as defined in 6.2.3) with both block
  scope or function prototype scope and no linkage shall have a
  variably modified type.

"entries" here is not an ordinary identifier; it is in the "members of
structures or unions" namespace (6.2.3 Name spaces of identifiers).

If you compile such code with "gcc -std=c99 -pedantic -Wall -Wextra",
gcc emits

  warning: a member of a structure or union cannot have a variably
           modified type

Anyway a #define easily fixes this.

> +    const struct kvm_cpuid_entry2 *cpuid_last_entry =
> +        &cpuid_data.entries[max_cpuid_entries - 1];

Consider const-qualifying not only the target of the pointer, but the
pointer itself:

    const struct kvm_cpuid_entry2 * const cpuid_last_entry = ...

>      X86CPU *cpu = X86_CPU(cs);
>      CPUX86State *env = &cpu->env;
>      uint32_t limit, i, j, cpuid_i;
> @@ -503,6 +506,10 @@ int kvm_arch_init_vcpu(CPUState *cs)
>  
>      for (i = 0; i <= limit; i++) {
>          c = &cpuid_data.entries[cpuid_i++];
> +        if (c > cpuid_last_entry) {
> +            fprintf(stderr, "unsupported level value: 0x%x\n", limit);
> +            abort();
> +        }

These comparisons are fine. If "c" points just one past the last element
in the array, then "c" is still valid for evaluation (but not
dereferencing), and it can be compared against another pointer into the
same array.

Also, the patch seems to catch all

  c = &cpuid_data.entries[cpuid_i++];

statements that are inside loops, and the rest (a low fixed number) is
covered by an array size like 100. If you introduce the #define (or
someone tells me we're not pedantic) you'll have my (not really relevant
:)) nod.

Thanks
Laszlo

On Thu, Jan 24, 2013 at 12:05:48AM +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>
> ---
>  target-i386/kvm.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 3acff40..8885b22 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -413,10 +413,13 @@ static void cpu_update_state(void *opaque, int running, RunState state)
>  
>  int kvm_arch_init_vcpu(CPUState *cs)
>  {
> +    const int max_cpuid_entries = 100;
>      struct {
>          struct kvm_cpuid2 cpuid;
> -        struct kvm_cpuid_entry2 entries[100];
> +        struct kvm_cpuid_entry2 entries[max_cpuid_entries];
>      } QEMU_PACKED cpuid_data;
> +    const struct kvm_cpuid_entry2 *cpuid_last_entry =
> +        &cpuid_data.entries[max_cpuid_entries - 1];
>      X86CPU *cpu = X86_CPU(cs);
>      CPUX86State *env = &cpu->env;
>      uint32_t limit, i, j, cpuid_i;
> @@ -503,6 +506,10 @@ int kvm_arch_init_vcpu(CPUState *cs)
>  
>      for (i = 0; i <= limit; i++) {
>          c = &cpuid_data.entries[cpuid_i++];
> +        if (c > cpuid_last_entry) {
> +            fprintf(stderr, "unsupported level value: 0x%x\n", limit);
> +            abort();
> +        }

Check directly for cpuid_i > max_cpuid_entries, no need for variable.
Also max_cpuid_entries can be a #define.