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

[Alex Jia]

On 01/04/2012 05:04 PM, Hu Tao wrote:
> 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.
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<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;
>                   }