From: Denis Mukhin <dmukhin@ford.com>

Fix potential memory leak in domain_create() in late hardware domain case.

Fixes: b959f3b820f5 ("xen: introduce hardware domain create flag")
Signed-off-by: Denis Mukhin <dmukhin@ford.com>
---
I think that no memory allocation is required before performing late hwdom
checks (ID range and hwdom existance).

Looks like sanitise_domain_config() could better fit for performing such
configuration checks.

Alternatively, hardware_domid range could be checked via custom parser
instead of code in domain_create() and then hwdom existance can be moved
before alloc_domain_struct().

Thoughts?
---
 xen/common/domain.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 2f6b0af50dd3..5ad1ac872798 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -825,7 +825,10 @@ struct domain *domain_create(domid_t domid,
 
         /* late_hwdom is only allowed for dom0. */
         if ( hardware_domain && hardware_domain->domain_id )
+        {
+            free_domain_struct(d);
             return ERR_PTR(-EINVAL);
+        }
 
         old_hwdom = hardware_domain;
         hardware_domain = d;
-- 
2.34.1

On 23.06.2025 03:16, dmkhn@proton.me wrote:
> From: Denis Mukhin <dmukhin@ford.com>
> 
> Fix potential memory leak in domain_create() in late hardware domain case.
> 
> Fixes: b959f3b820f5 ("xen: introduce hardware domain create flag")
> Signed-off-by: Denis Mukhin <dmukhin@ford.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

It may be relevant to mention that we still can't very well use "goto fail"
on this error path, as insufficient struct initialization was done just yet.

What we may want to consider is to move down the get_unique_id() invocation.
It's not the end of the world to lose one, but that may better be avoided
when we easily can.

> ---
> I think that no memory allocation is required before performing late hwdom
> checks (ID range and hwdom existance).
> 
> Looks like sanitise_domain_config() could better fit for performing such
> configuration checks.
> 
> Alternatively, hardware_domid range could be checked via custom parser
> instead of code in domain_create() and then hwdom existance can be moved
> before alloc_domain_struct().
> 
> Thoughts?

Keeping related things together is the other desire we commonly have.

Jan

On Mon, Jun 23, 2025 at 10:09:54AM +0200, Jan Beulich wrote:
> On 23.06.2025 03:16, dmkhn@proton.me wrote:
> > From: Denis Mukhin <dmukhin@ford.com>
> >
> > Fix potential memory leak in domain_create() in late hardware domain case.
> >
> > Fixes: b959f3b820f5 ("xen: introduce hardware domain create flag")
> > Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

Just in case, I've seen the patch is committed as
  3491e85a1505  ("xen/domain: fix memory leak in domain_create()")
but there's no R-b tag.

> 
> It may be relevant to mention that we still can't very well use "goto fail"
> on this error path, as insufficient struct initialization was done just yet.
> 
> What we may want to consider is to move down the get_unique_id() invocation.
> It's not the end of the world to lose one, but that may better be avoided
> when we easily can.
> 
> > ---
> > I think that no memory allocation is required before performing late hwdom
> > checks (ID range and hwdom existance).
> >
> > Looks like sanitise_domain_config() could better fit for performing such
> > configuration checks.
> >
> > Alternatively, hardware_domid range could be checked via custom parser
> > instead of code in domain_create() and then hwdom existance can be moved
> > before alloc_domain_struct().
> >
> > Thoughts?
> 
> Keeping related things together is the other desire we commonly have.

Yeah, I see this is to avoid duplicated checks, but verifying hardware_domid
range definitely can be moved outside of domain_create() 

This superfluous memory allocation will need a test case in cert world since
this is arch-common code :-/
So, IMO, it is better to avoid it.

> 
> Jan