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

On Wed, Jan 04, 2012 at 03:53:19PM +0800, ajia(a)redhat.com wrote: > From: Alex Jia&lt;ajia(a)redhat.com&gt; > > 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&lt;ajia(a)redhat.com&gt; > --- > 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&lt;hutao(a)cn.fujitsu.com&gt; 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; }