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

On 01/04/2012 05:04 PM, Hu Tao wrote: >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. In fact, I can successfully set nodeset value: # virsh numatune foo --nodeset 0-1 # virsh numatune foo numa_mode : strict numa_nodeset : 0-1

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