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

[Marcelo Tosatti]

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 <address@hidden>
> ---
>  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.