vircgroup: fix NULL pointer dereferencing

[libvirt] [PATCH] vircgroup: fix NULL pointer dereferencing

Posted by Marc Hartmayer 5 years, 6 months ago

When virCgroupEnableMissingControllers fails it's possible that *group
is still set to NULL. Therefore let's add a guard and an attribute for
this.

 [#0] virCgroupRemove(group=0x0)
 [#1] virCgroupNewMachineSystemd
 [#2] virCgroupNewMachine
 [#3] qemuInitCgroup
 [#4] qemuSetupCgroup
 [#5] qemuProcessLaunch
 [#6] qemuProcessStart
 [#7] qemuDomainObjStart
 [#8] qemuDomainCreateWithFlags
 [#9] qemuDomainCreate
 ...

Fixes: 1602aa28f820ada66f707cef3e536e8572fbda1e
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com>
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
---
 src/util/vircgroup.c | 3 ++-
 src/util/vircgroup.h | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 23957c82c7fa..06e1d158febb 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -1104,7 +1104,8 @@ virCgroupNewMachineSystemd(const char *name,
 
  error:
     saved = virSaveLastError();
-    virCgroupRemove(*group);
+    if (*group)
+        virCgroupRemove(*group);
     virCgroupFree(group);
     if (saved) {
         virSetError(saved);
diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h
index 1f676f21c380..9e1ae3706b1e 100644
--- a/src/util/vircgroup.h
+++ b/src/util/vircgroup.h
@@ -268,7 +268,8 @@ int virCgroupGetCpusetMemoryMigrate(virCgroupPtr group, bool *migrate);
 int virCgroupSetCpusetCpus(virCgroupPtr group, const char *cpus);
 int virCgroupGetCpusetCpus(virCgroupPtr group, char **cpus);
 
-int virCgroupRemove(virCgroupPtr group);
+int virCgroupRemove(virCgroupPtr group)
+    ATTRIBUTE_NONNULL(1);
 
 int virCgroupKillRecursive(virCgroupPtr group, int signum);
 int virCgroupKillPainfully(virCgroupPtr group);
-- 
2.17.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] vircgroup: fix NULL pointer dereferencing

Posted by John Ferlan 5 years, 6 months ago

$subj:

util: Fix cgroup processing NULL pointer dereferencing


On 9/26/18 11:53 AM, Marc Hartmayer wrote:
> When virCgroupEnableMissingControllers fails it's possible that *group
> is still set to NULL. Therefore let's add a guard and an attribute for
> this.

Prefix paragraph with rather than at the bottom "Fixes:".

Introduced by commit 1602aa28f,

> 
>  [#0] virCgroupRemove(group=0x0)
>  [#1] virCgroupNewMachineSystemd
>  [#2] virCgroupNewMachine
>  [#3] qemuInitCgroup
>  [#4] qemuSetupCgroup
>  [#5] qemuProcessLaunch
>  [#6] qemuProcessStart
>  [#7] qemuDomainObjStart
>  [#8] qemuDomainCreateWithFlags
>  [#9] qemuDomainCreate
>  ...
> 
> Fixes: 1602aa28f820ada66f707cef3e536e8572fbda1e

I think it's safe to remove the stack trace...

> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com>
> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> ---
>  src/util/vircgroup.c | 3 ++-
>  src/util/vircgroup.h | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)

While this patch is correct to remove the NULL deref, there "may" be a
problem with the patch that introduced this. Rather than usurp this
thread, I'll respond to the other one and see where it takes us.

John

> 
> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
> index 23957c82c7fa..06e1d158febb 100644
> --- a/src/util/vircgroup.c
> +++ b/src/util/vircgroup.c
> @@ -1104,7 +1104,8 @@ virCgroupNewMachineSystemd(const char *name,
>  
>   error:
>      saved = virSaveLastError();
> -    virCgroupRemove(*group);
> +    if (*group)
> +        virCgroupRemove(*group);
>      virCgroupFree(group);
>      if (saved) {
>          virSetError(saved);
> diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h
> index 1f676f21c380..9e1ae3706b1e 100644
> --- a/src/util/vircgroup.h
> +++ b/src/util/vircgroup.h
> @@ -268,7 +268,8 @@ int virCgroupGetCpusetMemoryMigrate(virCgroupPtr group, bool *migrate);
>  int virCgroupSetCpusetCpus(virCgroupPtr group, const char *cpus);
>  int virCgroupGetCpusetCpus(virCgroupPtr group, char **cpus);
>  
> -int virCgroupRemove(virCgroupPtr group);
> +int virCgroupRemove(virCgroupPtr group)
> +    ATTRIBUTE_NONNULL(1);

FWIW: This only helps if someone tried to call virCgroupRemove(NULL); it
doesn't help if the parameter itself is NULL. One could add a "if
(!group) return 0" to virCgroupRemove to avoid.

>  
>  int virCgroupKillRecursive(virCgroupPtr group, int signum);
>  int virCgroupKillPainfully(virCgroupPtr group);
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] vircgroup: fix NULL pointer dereferencing

Posted by Marc Hartmayer 5 years, 6 months ago

On Thu, Sep 27, 2018 at 02:38 PM +0200, John Ferlan <jferlan@redhat.com> wrote:
> $subj:
>
> util: Fix cgroup processing NULL pointer dereferencing

I’m fine with this change :)

>
>
> On 9/26/18 11:53 AM, Marc Hartmayer wrote:
>> When virCgroupEnableMissingControllers fails it's possible that *group
>> is still set to NULL. Therefore let's add a guard and an attribute for
>> this.
>
> Prefix paragraph with rather than at the bottom "Fixes:".

Okay.

>
> Introduced by commit 1602aa28f,
>
>>
>>  [#0] virCgroupRemove(group=0x0)
>>  [#1] virCgroupNewMachineSystemd
>>  [#2] virCgroupNewMachine
>>  [#3] qemuInitCgroup
>>  [#4] qemuSetupCgroup
>>  [#5] qemuProcessLaunch
>>  [#6] qemuProcessStart
>>  [#7] qemuDomainObjStart
>>  [#8] qemuDomainCreateWithFlags
>>  [#9] qemuDomainCreate
>>  ...
>>
>> Fixes: 1602aa28f820ada66f707cef3e536e8572fbda1e
>
> I think it's safe to remove the stack trace...

Okay.

>
>> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
>> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com>
>> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>> ---
>>  src/util/vircgroup.c | 3 ++-
>>  src/util/vircgroup.h | 3 ++-
>>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> While this patch is correct to remove the NULL deref, there "may" be a
> problem with the patch that introduced this. Rather than usurp this
> thread, I'll respond to the other one and see where it takes us.

Okay.

>
> John
>
>>
>> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
>> index 23957c82c7fa..06e1d158febb 100644
>> --- a/src/util/vircgroup.c
>> +++ b/src/util/vircgroup.c
>> @@ -1104,7 +1104,8 @@ virCgroupNewMachineSystemd(const char *name,
>>
>>   error:
>>      saved = virSaveLastError();
>> -    virCgroupRemove(*group);
>> +    if (*group)
>> +        virCgroupRemove(*group);
>>      virCgroupFree(group);
>>      if (saved) {
>>          virSetError(saved);
>> diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h
>> index 1f676f21c380..9e1ae3706b1e 100644
>> --- a/src/util/vircgroup.h
>> +++ b/src/util/vircgroup.h
>> @@ -268,7 +268,8 @@ int virCgroupGetCpusetMemoryMigrate(virCgroupPtr group, bool *migrate);
>>  int virCgroupSetCpusetCpus(virCgroupPtr group, const char *cpus);
>>  int virCgroupGetCpusetCpus(virCgroupPtr group, char **cpus);
>>
>> -int virCgroupRemove(virCgroupPtr group);
>> +int virCgroupRemove(virCgroupPtr group)
>> +    ATTRIBUTE_NONNULL(1);
>
> FWIW: This only helps if someone tried to call virCgroupRemove(NULL); it
> doesn't help if the parameter itself is NULL. One could add a "if
> (!group) return 0" to virCgroupRemove to avoid.

Thanks for pointing this out! :) I thought it could help Coverity.

>
>>
>>  int virCgroupKillRecursive(virCgroupPtr group, int signum);
>>  int virCgroupKillPainfully(virCgroupPtr group);
>>
>
--
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list