[libvirt] [PATCH] cpu_x86.c: avoid NULL-deref for invalid arguments
Passing a NULL "models" pointer along with a contradictory "nmodels >= 1" would cause a NULL-dereference. An alternative to the fix below would be simply to guard the NULL-derferencing strcmp with "if (models ...", but that wouldn't tell the caller that they're passing bogus arguments.
From f57bd1fbe7a41b1b9d8ba1be61790e95b5060ddc Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Tue, 26 Jan 2010 19:58:48 +0100 Subject: [PATCH] cpu_x86.c: avoid NULL-deref for invalid arguments
* src/cpu/cpu_x86.c (x86Decode): Do not dereference NULL when "models" is NULL and nmodels is 1 or greater. --- src/cpu/cpu_x86.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index dae7c90..47dc400 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -1,7 +1,7 @@ /* * cpu_x86.c: CPU driver for CPUs with x86 compatible CPUID instruction * - * Copyright (C) 2009 Red Hat, Inc. + * Copyright (C) 2009-2010 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -954,6 +954,9 @@ x86Decode(virCPUDefPtr cpu, if (data == NULL || (map = x86LoadMap()) == NULL) return -1; + if (models == NULL && nmodels != 0) + return -1; + candidate = map->models; while (candidate != NULL) { bool allowed = (models == NULL); -- 1.7.0.rc0.140.gfbe7
On Tue, Jan 26, 2010 at 08:06:03PM +0100, Jim Meyering wrote:
Passing a NULL "models" pointer along with a contradictory "nmodels >= 1" would cause a NULL-dereference.
An alternative to the fix below would be simply to guard the NULL-derferencing strcmp with "if (models ...", but that wouldn't tell the caller that they're passing bogus arguments.
From f57bd1fbe7a41b1b9d8ba1be61790e95b5060ddc Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Tue, 26 Jan 2010 19:58:48 +0100 Subject: [PATCH] cpu_x86.c: avoid NULL-deref for invalid arguments
* src/cpu/cpu_x86.c (x86Decode): Do not dereference NULL when "models" is NULL and nmodels is 1 or greater. --- src/cpu/cpu_x86.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index dae7c90..47dc400 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -1,7 +1,7 @@ /* * cpu_x86.c: CPU driver for CPUs with x86 compatible CPUID instruction * - * Copyright (C) 2009 Red Hat, Inc. + * Copyright (C) 2009-2010 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -954,6 +954,9 @@ x86Decode(virCPUDefPtr cpu, if (data == NULL || (map = x86LoadMap()) == NULL) return -1;
+ if (models == NULL && nmodels != 0) + return -1; + candidate = map->models; while (candidate != NULL) { bool allowed = (models == NULL);
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Passing a NULL "models" pointer along with a contradictory "nmodels >= 1" would cause a NULL-dereference.
An alternative to the fix below would be simply to guard the NULL-derferencing strcmp with "if (models ...", but that wouldn't tell the caller that they're passing bogus arguments. ... diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index dae7c90..47dc400 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -1,7 +1,7 @@ /* * cpu_x86.c: CPU driver for CPUs with x86 compatible CPUID instruction * - * Copyright (C) 2009 Red Hat, Inc. + * Copyright (C) 2009-2010 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -954,6 +954,9 @@ x86Decode(virCPUDefPtr cpu, if (data == NULL || (map = x86LoadMap()) == NULL) return -1;
+ if (models == NULL && nmodels != 0) + return -1; +
Hmm, this check introduces a possible memory leak, as it exists the function without freeing map. We could just move the check at the beginning of the function but since this is a private architecture specific implementation for cpuDecode, I'd rather move the check one level up to the arch independent entry point. A patch for that is attached. Jirka
Jiri Denemark wrote: ...
@@ -954,6 +954,9 @@ x86Decode(virCPUDefPtr cpu, if (data == NULL || (map = x86LoadMap()) == NULL) return -1;
+ if (models == NULL && nmodels != 0) + return -1; +
Hmm, this check introduces a possible memory leak, as it exists the function without freeing map. We could just move the check at the beginning of the function but since this is a private architecture specific implementation for cpuDecode, I'd rather move the check one level up to the arch independent entry point. A patch for that is attached.
Subject: [PATCH] Move models/nmodels mismatch checking one level up
Hi Jirka, Good catch. That's a fine change: plugs a leak and improves the higher-level interface. Thank you.
Jiri Denemark wrote:
Passing a NULL "models" pointer along with a contradictory "nmodels >= 1" would cause a NULL-dereference.
An alternative to the fix below would be simply to guard the NULL-derferencing strcmp with "if (models ...", but that wouldn't tell the caller that they're passing bogus arguments. ... diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index dae7c90..47dc400 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -1,7 +1,7 @@ /* * cpu_x86.c: CPU driver for CPUs with x86 compatible CPUID instruction * - * Copyright (C) 2009 Red Hat, Inc. + * Copyright (C) 2009-2010 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -954,6 +954,9 @@ x86Decode(virCPUDefPtr cpu, if (data == NULL || (map = x86LoadMap()) == NULL) return -1;
+ if (models == NULL && nmodels != 0) + return -1; +
Hmm, this check introduces a possible memory leak, as it exists the function without freeing map. We could just move the check at the beginning of the function but since this is a private architecture specific implementation for cpuDecode, I'd rather move the check one level up to the arch independent entry point. A patch for that is attached.
BTW, I've just pushed this.
participants (3)
-
Daniel P. Berrange -
Jim Meyering -
Jiri Denemark