domain_destroy() is the wrong position to be freeing colouring information.

The comment in context identifies how domain_destroy() can be called multiple
times on the same domain, leading to a double free of d->llc_colors as it's
the wrong side of the atomic_cmpxchg() to be made safe.

Furthermore, by freeing d->llc_colors but leaving d->num_llc_colors nonzero,
alloc_color_heap_page() can in principle use after free.

Make domain_llc_coloring_free() idempotent, zeroing d->num_llc_colors and
NULL-ing d->llc_colors after freeing, and call it from domain_teardown().

Fixes: 6985aa5e0c3c ("xen: extend domctl interface for cache coloring")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Anthony PERARD <anthony.perard@vates.tech>
CC: Michal Orzel <michal.orzel@amd.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Julien Grall <julien@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Carlo Nonato <carlo.nonato@minervasys.tech>
CC: Marco Solieri <marco.solieri@minervasys.tech>

Cache colouring is experimental, which is why no XSA is being allocated for
these bugs.
---
 xen/common/domain.c       | 5 +++--
 xen/common/llc-coloring.c | 4 +++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 303c338ef293..4f79ba39878c 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -626,6 +626,9 @@ static int domain_teardown(struct domain *d)
     case PROG_none:
         BUILD_BUG_ON(PROG_none != 0);
 
+        /* Trivial teardown, not long-running enough to need a preemption check. */
+        domain_llc_coloring_free(d);
+
     PROGRESS(gnttab_mappings):
         rc = gnttab_release_mappings(d);
         if ( rc )
@@ -1447,8 +1450,6 @@ void domain_destroy(struct domain *d)
 {
     BUG_ON(!d->is_dying);
 
-    domain_llc_coloring_free(d);
-
     /* May be already destroyed, or get_domain() can race us. */
     if ( atomic_cmpxchg(&d->refcnt, 0, DOMAIN_DESTROYED) != 0 )
         return;
diff --git a/xen/common/llc-coloring.c b/xen/common/llc-coloring.c
index bd1f634f0bb8..ea3e0ca07017 100644
--- a/xen/common/llc-coloring.c
+++ b/xen/common/llc-coloring.c
@@ -309,8 +309,10 @@ int domain_set_llc_colors(struct domain *d,
 
 void domain_llc_coloring_free(struct domain *d)
 {
+    d->num_llc_colors = 0;
+
     if ( d->llc_colors != default_colors )
-        xfree(d->llc_colors);
+        XFREE(d->llc_colors);
 }
 
 int __init domain_set_llc_colors_from_str(struct domain *d, const char *str)
-- 
2.39.5

On 24.07.2025 18:23, Andrew Cooper wrote:
> domain_destroy() is the wrong position to be freeing colouring information.
> 
> The comment in context identifies how domain_destroy() can be called multiple
> times on the same domain, leading to a double free of d->llc_colors as it's
> the wrong side of the atomic_cmpxchg() to be made safe.
> 
> Furthermore, by freeing d->llc_colors but leaving d->num_llc_colors nonzero,
> alloc_color_heap_page() can in principle use after free.
> 
> Make domain_llc_coloring_free() idempotent, zeroing d->num_llc_colors and
> NULL-ing d->llc_colors after freeing, and call it from domain_teardown().

And this pulling earlier is safe because e.g. free_color_heap_page() ->
page_to_llc_color() -> mfn_to_color() don't use the domain's colors array.
That's not a given (and could be different if e.g. per-domain stats were
maintained), so may want mentioning.

> Fixes: 6985aa5e0c3c ("xen: extend domctl interface for cache coloring")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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