1. Patchew
  2. Libvirt
  3. View series

[PATCH] security_manager: Don't leak seclabel in virSecurityManagerGenLabel()

Michal Privoznik via Devel posted 1 patch 2 months, 3 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/b8cb2fe2f5811369bf6ee1a83f032695acaadd5d.1750069040.git.mprivozn@redhat.com
src/security/security_manager.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
[PATCH] security_manager: Don't leak seclabel in virSecurityManagerGenLabel()
Posted by Michal Privoznik via Devel 2 months, 3 weeks ago 
From: Michal Privoznik <mprivozn@redhat.com>

When a domain is being started, seclabels are generated for it.
This is handled in virSecurityManagerGenLabel() which can either
find pre-existing seclabel in domain def or generate a new one.
At any rate, domainGenSecurityLabel() callback is called and if
it fails then the seclabel is removed from domain definition
using VIR_DELETE_ELEMENT(). While this shrinks down the seclabels
array, it does not free individual item. It has to be freed
manually.

80 bytes in 2 blocks are definitely lost in loss record 1,359 of 1,876
   at 0x484CEF3: calloc (vg_replace_malloc.c:1675)
   by 0x4F19B29: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.8200.5)
   by 0x49E4953: virSecurityLabelDefNew (virseclabel.c:59)
   by 0x4BDE0A4: virSecurityManagerGenLabel (security_manager.c:638)
   by 0xBA029B7: qemuProcessPrepareDomain (qemu_process.c:6760)
   by 0xBA07DF2: qemuProcessStart (qemu_process.c:8369)
   by 0xB93DAC0: qemuDomainObjStart (qemu_driver.c:6371)
   by 0xB93DE08: qemuDomainCreateWithFlags (qemu_driver.c:6420)
   by 0xB93DE86: qemuDomainCreate (qemu_driver.c:6438)
   by 0x4CECEA8: virDomainCreate (libvirt-domain.c:7142)

Now, you might think this may lead to a double free, because
@seclabel is freed under the 'cleanup' label (if @generated is
true). But if @generated is true, then just before calling the
callback there's VIR_APPEND_ELEMENT() which clears @seclabel out
turning the free under 'cleanup' label into a NOP.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/security/security_manager.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index c2460eae37..5fc4eb4872 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -669,9 +669,14 @@ virSecurityManagerGenLabel(virSecurityManager *mgr,
                 VIR_APPEND_ELEMENT(vm->seclabels, vm->nseclabels, seclabel);
 
             if (sec_managers[i]->drv->domainGenSecurityLabel(sec_managers[i], vm) < 0) {
+                virSecurityLabelDef *tmp = vm->seclabels[vm->nseclabels - 1];
+
                 if (VIR_DELETE_ELEMENT(vm->seclabels,
-                                       vm->nseclabels -1, vm->nseclabels) < 0)
+                                       vm->nseclabels - 1, vm->nseclabels) < 0) {
                     vm->nseclabels--;
+                }
+
+                virSecurityLabelDefFree(tmp);
                 goto cleanup;
             }
 
-- 
2.49.0
Re: [PATCH] security_manager: Don't leak seclabel in virSecurityManagerGenLabel()
Posted by Ján Tomko via Devel 2 months, 3 weeks ago 
On a Monday in 2025, Michal Privoznik via Devel wrote:
>From: Michal Privoznik <mprivozn@redhat.com>
>
>When a domain is being started, seclabels are generated for it.
>This is handled in virSecurityManagerGenLabel() which can either
>find pre-existing seclabel in domain def or generate a new one.
>At any rate, domainGenSecurityLabel() callback is called and if
>it fails then the seclabel is removed from domain definition
>using VIR_DELETE_ELEMENT(). While this shrinks down the seclabels
>array, it does not free individual item. It has to be freed
>manually.
>
>80 bytes in 2 blocks are definitely lost in loss record 1,359 of 1,876
>   at 0x484CEF3: calloc (vg_replace_malloc.c:1675)
>   by 0x4F19B29: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.8200.5)
>   by 0x49E4953: virSecurityLabelDefNew (virseclabel.c:59)
>   by 0x4BDE0A4: virSecurityManagerGenLabel (security_manager.c:638)
>   by 0xBA029B7: qemuProcessPrepareDomain (qemu_process.c:6760)
>   by 0xBA07DF2: qemuProcessStart (qemu_process.c:8369)
>   by 0xB93DAC0: qemuDomainObjStart (qemu_driver.c:6371)
>   by 0xB93DE08: qemuDomainCreateWithFlags (qemu_driver.c:6420)
>   by 0xB93DE86: qemuDomainCreate (qemu_driver.c:6438)
>   by 0x4CECEA8: virDomainCreate (libvirt-domain.c:7142)
>
>Now, you might think this may lead to a double free, because
>@seclabel is freed under the 'cleanup' label (if @generated is
>true). But if @generated is true, then just before calling the
>callback there's VIR_APPEND_ELEMENT() which clears @seclabel out
>turning the free under 'cleanup' label into a NOP.
>
>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
> src/security/security_manager.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
>diff --git a/src/security/security_manager.c b/src/security/security_manager.c
>index c2460eae37..5fc4eb4872 100644
>--- a/src/security/security_manager.c
>+++ b/src/security/security_manager.c
>@@ -669,9 +669,14 @@ virSecurityManagerGenLabel(virSecurityManager *mgr,
>                 VIR_APPEND_ELEMENT(vm->seclabels, vm->nseclabels, seclabel);
>
>             if (sec_managers[i]->drv->domainGenSecurityLabel(sec_managers[i], vm) < 0) {
>+                virSecurityLabelDef *tmp = vm->seclabels[vm->nseclabels - 1];
>+
>                 if (VIR_DELETE_ELEMENT(vm->seclabels,
>-                                       vm->nseclabels -1, vm->nseclabels) < 0)
>+                                       vm->nseclabels - 1, vm->nseclabels) < 0) {
>                     vm->nseclabels--;
>+                }
>+

The braces are not necessary in this case:
https://libvirt.org/coding-style.html#curly-braces

>+                virSecurityLabelDefFree(tmp);
>                 goto cleanup;
>             }

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano
Re: [PATCH] security_manager: Don't leak seclabel in virSecurityManagerGenLabel()
Posted by Michal Prívozník via Devel 2 months, 3 weeks ago 
On 6/17/25 5:46 PM, Ján Tomko wrote:
> On a Monday in 2025, Michal Privoznik via Devel wrote:
>> From: Michal Privoznik <mprivozn@redhat.com>
>>
>> When a domain is being started, seclabels are generated for it.
>> This is handled in virSecurityManagerGenLabel() which can either
>> find pre-existing seclabel in domain def or generate a new one.
>> At any rate, domainGenSecurityLabel() callback is called and if
>> it fails then the seclabel is removed from domain definition
>> using VIR_DELETE_ELEMENT(). While this shrinks down the seclabels
>> array, it does not free individual item. It has to be freed
>> manually.
>>
>> 80 bytes in 2 blocks are definitely lost in loss record 1,359 of 1,876
>>   at 0x484CEF3: calloc (vg_replace_malloc.c:1675)
>>   by 0x4F19B29: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.8200.5)
>>   by 0x49E4953: virSecurityLabelDefNew (virseclabel.c:59)
>>   by 0x4BDE0A4: virSecurityManagerGenLabel (security_manager.c:638)
>>   by 0xBA029B7: qemuProcessPrepareDomain (qemu_process.c:6760)
>>   by 0xBA07DF2: qemuProcessStart (qemu_process.c:8369)
>>   by 0xB93DAC0: qemuDomainObjStart (qemu_driver.c:6371)
>>   by 0xB93DE08: qemuDomainCreateWithFlags (qemu_driver.c:6420)
>>   by 0xB93DE86: qemuDomainCreate (qemu_driver.c:6438)
>>   by 0x4CECEA8: virDomainCreate (libvirt-domain.c:7142)
>>
>> Now, you might think this may lead to a double free, because
>> @seclabel is freed under the 'cleanup' label (if @generated is
>> true). But if @generated is true, then just before calling the
>> callback there's VIR_APPEND_ELEMENT() which clears @seclabel out
>> turning the free under 'cleanup' label into a NOP.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>> src/security/security_manager.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/security/security_manager.c b/src/security/
>> security_manager.c
>> index c2460eae37..5fc4eb4872 100644
>> --- a/src/security/security_manager.c
>> +++ b/src/security/security_manager.c
>> @@ -669,9 +669,14 @@ virSecurityManagerGenLabel(virSecurityManager *mgr,
>>                 VIR_APPEND_ELEMENT(vm->seclabels, vm->nseclabels,
>> seclabel);
>>
>>             if (sec_managers[i]->drv-
>> >domainGenSecurityLabel(sec_managers[i], vm) < 0) {
>> +                virSecurityLabelDef *tmp = vm->seclabels[vm-
>> >nseclabels - 1];
>> +
>>                 if (VIR_DELETE_ELEMENT(vm->seclabels,
>> -                                       vm->nseclabels -1, vm-
>> >nseclabels) < 0)
>> +                                       vm->nseclabels - 1, vm-
>> >nseclabels) < 0) {
>>                     vm->nseclabels--;
>> +                }
>> +
> 
> The braces are not necessary in this case:
> https://libvirt.org/coding-style.html#curly-braces

Yeah, I know, it's a personal preference.

> 
>> +                virSecurityLabelDefFree(tmp);
>>                 goto cleanup;
>>             }
> 
> Reviewed-by: Ján Tomko <jtomko@redhat.com>

Merged, thanks.

Michal

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.