qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] target-i386: kvm: prevent buffer overflow if -c


From: Laszlo Ersek
Subject: Re: [Qemu-devel] [PATCH] target-i386: kvm: prevent buffer overflow if -cpu foo, [x]level is too big
Date: Thu, 24 Jan 2013 10:56:14 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:10.0.11) Gecko/20121116 Thunderbird/10.0.11

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 <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;

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]