[libvirt] [PATCH] qemu: fix a bug in numatune (was Re: [PATCH] qemu: Prevent crash of libvirtd when setting numa parameters)

[Hu Tao]

On Wed, Jan 04, 2012 at 03:53:19PM +0800, ajia at redhat.com wrote:
> From: Alex Jia <ajia at redhat.com>
> 
> It's a NULL pointer deref issue, which leads to libvirtd crash. This patch 
> directly use 'params[i].value.s' value instead of derefing a NULL pointer
> on memcpy.
> 
> * how to reproduce?
> % virsh numatune <domain> --nodeset 0

The domain must have no nodeset set previously (to crash in this example).

> % service libvirtd status
> 
> * src/qemu/qemu_driver.c (qemuDomainSetNumaParameters): avoid a NULL pointer deref.
> 
> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=771562
> 
> Signed-off-by: Alex Jia <ajia at redhat.com>
> ---
>  src/qemu/qemu_driver.c |    6 ++----
>  1 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 82bab67..1bd93f6 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -6721,14 +6721,12 @@ qemuDomainSetNumaParameters(virDomainPtr dom,
>              }
>  
>              if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
> -                memcpy(oldnodemask, persistentDef->numatune.memory.nodemask,
> -                       VIR_DOMAIN_CPUMASK_LEN);
> +                memcpy(oldnodemask, params[i].value.s, VIR_DOMAIN_CPUMASK_LEN);
>                  if (virDomainCpuSetParse(params[i].value.s,
>                                           0,
>                                           persistentDef->numatune.memory.nodemask,

Not correct. In this case persistentDef->numatune.memory.nodemask is
null, and virDomainCpuSetParse will always fail, thus the nodeset will
never be set.

>                                           VIR_DOMAIN_CPUMASK_LEN) < 0) {
> -                    memcpy(persistentDef->numatune.memory.nodemask,
> -                           oldnodemask, VIR_DOMAIN_CPUMASK_LEN);
> +                    memcpy(params[i].value.s, oldnodemask, VIR_DOMAIN_CPUMASK_LEN);
>                      ret = -1;
>                      continue;
>                  }

>From f2fe7c7d0041779895330416dca6ac97fcbec88a Mon Sep 17 00:00:00 2001
From: Hu Tao <hutao at cn.fujitsu.com>
Date: Wed, 4 Jan 2012 17:00:27 +0800
Subject: [PATCH] qemu: fix a bug in numatune

When setting numa nodeset for a domain which has no nodeset set
before, libvirtd crashes by dereferring the pointer to the old
nodemask which is null in the case.
---
 src/qemu/qemu_driver.c |   22 ++++++++++++++++++----
 1 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 82bab67..37330b4 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6721,14 +6721,28 @@ qemuDomainSetNumaParameters(virDomainPtr dom,
             }
 
             if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
-                memcpy(oldnodemask, persistentDef->numatune.memory.nodemask,
-                       VIR_DOMAIN_CPUMASK_LEN);
+                bool savedmask = false;
+                if (!persistentDef->numatune.memory.nodemask) {
+                    if (VIR_ALLOC_N(persistentDef->numatune.memory.nodemask,
+                                    VIR_DOMAIN_CPUMASK_LEN) < 0) {
+                        virReportOOMError();
+                        ret = -1;
+                        goto cleanup;
+                    }
+                } else {
+                    memcpy(oldnodemask, persistentDef->numatune.memory.nodemask,
+                           VIR_DOMAIN_CPUMASK_LEN);
+                    savedmask = true;
+                }
                 if (virDomainCpuSetParse(params[i].value.s,
                                          0,
                                          persistentDef->numatune.memory.nodemask,
                                          VIR_DOMAIN_CPUMASK_LEN) < 0) {
-                    memcpy(persistentDef->numatune.memory.nodemask,
-                           oldnodemask, VIR_DOMAIN_CPUMASK_LEN);
+                    if (savedmask)
+                        memcpy(persistentDef->numatune.memory.nodemask,
+                               oldnodemask, VIR_DOMAIN_CPUMASK_LEN);
+                    else
+                        VIR_FREE(persistentDef->numatune.memory.nodemask);
                     ret = -1;
                     continue;
                 }
-- 
1.7.4.4


-- 
Thanks,
Hu Tao