[v1] xen/domctl: Fix double domid_free in XEN_DOMCTL_createdomain error path

[PATCH] xen/domctl: Fix double domid_free in XEN_DOMCTL_createdomain error path

Posted by Oleksii Moisieiev 4 months, 4 weeks ago

Remove redundant domid_free() call in the XEN_DOMCTL_createdomain error
handling path to prevent a double-free condition.

When domain_create() fails, it internally calls _domain_destroy() during
its cleanup routine, which already invokes domid_free() to release the
allocated domain ID. The additional domid_free() call in the domctl error
path creates a double-free scenario, triggering an assertion failure in
domid.c:

    Assertion 'rc' failed at common/domid.c:84

The domain creation flow is:
1. domid_alloc() allocates a domain ID
2. domain_create() is called with the allocated ID
3. If domain_create() fails:
   a) domain_create() calls _domain_destroy() internally
   b) _domain_destroy() calls domid_free() to release the ID
   c) domctl incorrectly calls domid_free() again

This double-free violates the domain ID management invariants and causes
system instability. The fix ensures domid_free() is called exactly once
per allocated domain ID, maintaining proper resource cleanup
semantics.

Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
---

 xen/common/domctl.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 71e712c1f3..954d790226 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -421,7 +421,6 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
         d = domain_create(domid, &op->u.createdomain, false);
         if ( IS_ERR(d) )
         {
-            domid_free(domid);
             ret = PTR_ERR(d);
             d = NULL;
             break;
-- 
2.34.1

Re: [PATCH] xen/domctl: Fix double domid_free in XEN_DOMCTL_createdomain error path

Posted by dmukhin@xen.org 4 months, 4 weeks ago

On Sat, Sep 13, 2025 at 10:44:39AM +0000, Oleksii Moisieiev wrote:
> Remove redundant domid_free() call in the XEN_DOMCTL_createdomain error
> handling path to prevent a double-free condition.
> 
> When domain_create() fails, it internally calls _domain_destroy() during
> its cleanup routine, which already invokes domid_free() to release the
> allocated domain ID. The additional domid_free() call in the domctl error
> path creates a double-free scenario, triggering an assertion failure in
> domid.c:
> 
>     Assertion 'rc' failed at common/domid.c:84
> 
> The domain creation flow is:
> 1. domid_alloc() allocates a domain ID
> 2. domain_create() is called with the allocated ID
> 3. If domain_create() fails:
>    a) domain_create() calls _domain_destroy() internally
>    b) _domain_destroy() calls domid_free() to release the ID
>    c) domctl incorrectly calls domid_free() again
> 
> This double-free violates the domain ID management invariants and causes
> system instability. The fix ensures domid_free() is called exactly once
> per allocated domain ID, maintaining proper resource cleanup
> semantics.
> 
> Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>

Thanks a quick fix and sorry for the breakage.

--
Denis

Re: [PATCH] xen/domctl: Fix double domid_free in XEN_DOMCTL_createdomain error path

Posted by Andrew Cooper 4 months, 4 weeks ago

On 13/09/2025 11:44 am, Oleksii Moisieiev wrote:
> Remove redundant domid_free() call in the XEN_DOMCTL_createdomain error
> handling path to prevent a double-free condition.
>
> When domain_create() fails, it internally calls _domain_destroy() during
> its cleanup routine, which already invokes domid_free() to release the
> allocated domain ID. The additional domid_free() call in the domctl error
> path creates a double-free scenario, triggering an assertion failure in
> domid.c:
>
>     Assertion 'rc' failed at common/domid.c:84
>
> The domain creation flow is:
> 1. domid_alloc() allocates a domain ID
> 2. domain_create() is called with the allocated ID
> 3. If domain_create() fails:
>    a) domain_create() calls _domain_destroy() internally
>    b) _domain_destroy() calls domid_free() to release the ID
>    c) domctl incorrectly calls domid_free() again
>
> This double-free violates the domain ID management invariants and causes
> system instability. The fix ensures domid_free() is called exactly once
> per allocated domain ID, maintaining proper resource cleanup
> semantics.

Fixes: 2d5065060710 ("xen/domain: unify domain ID allocation")

> Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

the tl;dr is that domain_create() either inserts the domain into the
domlist, or cleans up after itself.

The domid alloc infrastructure is problematic in multiple ways, not
least because it now means there are two sources of truth for which
domain's exist, and they are not interlocked.

I would have blocked this from being committed if I'd had any time to
look at it.  It will need remediating one way or another before 4.21
goes out.

~Andrew

Re: [PATCH] xen/domctl: Fix double domid_free in XEN_DOMCTL_createdomain error path

Posted by Alejandro Vallejo 3 months, 4 weeks ago

On Sat Sep 13, 2025 at 1:56 PM CEST, Andrew Cooper wrote:
> On 13/09/2025 11:44 am, Oleksii Moisieiev wrote:
>> Remove redundant domid_free() call in the XEN_DOMCTL_createdomain error
>> handling path to prevent a double-free condition.
>>
>> When domain_create() fails, it internally calls _domain_destroy() during
>> its cleanup routine, which already invokes domid_free() to release the
>> allocated domain ID. The additional domid_free() call in the domctl error
>> path creates a double-free scenario, triggering an assertion failure in
>> domid.c:
>>
>>     Assertion 'rc' failed at common/domid.c:84
>>
>> The domain creation flow is:
>> 1. domid_alloc() allocates a domain ID
>> 2. domain_create() is called with the allocated ID
>> 3. If domain_create() fails:
>>    a) domain_create() calls _domain_destroy() internally
>>    b) _domain_destroy() calls domid_free() to release the ID
>>    c) domctl incorrectly calls domid_free() again
>>
>> This double-free violates the domain ID management invariants and causes
>> system instability. The fix ensures domid_free() is called exactly once
>> per allocated domain ID, maintaining proper resource cleanup
>> semantics.
>
> Fixes: 2d5065060710 ("xen/domain: unify domain ID allocation")
>
>> Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> the tl;dr is that domain_create() either inserts the domain into the
> domlist, or cleans up after itself.
>
> The domid alloc infrastructure is problematic in multiple ways, not
> least because it now means there are two sources of truth for which
> domain's exist, and they are not interlocked.

The source of truth of existing domains is the domlist. The source of truth
of domids is the domid bitmap; and they need not match.

A domid being allocated doesn't mean the domain exists. Merely that the domid
is unavailable for allocation.

This is fairly important to allow fallible boots, where some boot-domains are
allowed to fail construction for one reason or another while preventing their
domids from being hijacked by a later domain, and thus prevent some resources
granted to those domains that failed construction from being usable by others.

The actual problem is that the lifetime of certain resources assigned to domains
extends past the lifetime of the domain, and the mere possesion of a domid
grants authority over resources assigned to the domid, even after its associated
domain died. This is wrong and bad, and has been wrong and bad since before the
time_t was 0.

If it wasn't for this messed up means of resource management we wouldn't have a
need for preventing certain domids from being used. We need saner semantics with
the lifetimes of event channels and grants. And possibly more resources.

Cheers,
Alejandro

Re: [PATCH] xen/domctl: Fix double domid_free in XEN_DOMCTL_createdomain error path

Posted by Demi Marie Obenour 4 months, 4 weeks ago

On 9/13/25 07:56, Andrew Cooper wrote:
> On 13/09/2025 11:44 am, Oleksii Moisieiev wrote:
>> Remove redundant domid_free() call in the XEN_DOMCTL_createdomain error
>> handling path to prevent a double-free condition.
>>
>> When domain_create() fails, it internally calls _domain_destroy() during
>> its cleanup routine, which already invokes domid_free() to release the
>> allocated domain ID. The additional domid_free() call in the domctl error
>> path creates a double-free scenario, triggering an assertion failure in
>> domid.c:
>>
>>     Assertion 'rc' failed at common/domid.c:84
>>
>> The domain creation flow is:
>> 1. domid_alloc() allocates a domain ID
>> 2. domain_create() is called with the allocated ID
>> 3. If domain_create() fails:
>>    a) domain_create() calls _domain_destroy() internally
>>    b) _domain_destroy() calls domid_free() to release the ID
>>    c) domctl incorrectly calls domid_free() again
>>
>> This double-free violates the domain ID management invariants and causes
>> system instability. The fix ensures domid_free() is called exactly once
>> per allocated domain ID, maintaining proper resource cleanup
>> semantics.
> 
> Fixes: 2d5065060710 ("xen/domain: unify domain ID allocation")
> 
>> Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> the tl;dr is that domain_create() either inserts the domain into the
> domlist, or cleans up after itself.
> 
> The domid alloc infrastructure is problematic in multiple ways, not
> least because it now means there are two sources of truth for which
> domain's exist, and they are not interlocked.
> 
> I would have blocked this from being committed if I'd had any time to
> look at it.  It will need remediating one way or another before 4.21
> goes out.
Revert time?
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)