From 51e09058d589914ee46b8e996bd6e5d322c5cf32 Mon Sep 17 00:00:00 2001 From: Jan Beulich Date: Tue, 28 Apr 2026 13:41:28 +0100 Subject: [PATCH] gnttab: split gnttab_map_frame() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If a domain tries to map status frames in parallel to switching grant table version from 2 to 1, the mapping operation may put in place P2M entries referencing MFNs which gnttab_unpopulate_status_frames() is in the process of freeing. Ideally we would refcount pages when entered into P2M tables, but that's a significant change. Extend the grant-table-locked region instead in xenmem_add_to_physmap_one() (being the sole caller of gnttab_map_frame()), such that a race with gnttab_unpopulate_status_frames() is no longer possible. This is XSA-486 / CVE-2026-23558. Fixes: 5ce8fafa947c ("Dynamic grant-table sizing") Fixes: a98dc13703e0 ("Introduce a grant_entry_v2 structure") Reported-by: Rafal Wojtczuk Signed-off-by: Jan Beulich Reviewed-by: Roger Pau Monné (cherry picked from commit 02708d3b51b04aeeaa4b127637d7fec8d8c5b1d8) --- xen/arch/arm/mm.c | 19 +++++++++++++++---- xen/arch/x86/mm/p2m.c | 25 ++++++++++++++++--------- xen/common/grant_table.c | 13 +++++++++++-- xen/include/xen/grant_table.h | 15 +++++++++++---- 4 files changed, 53 insertions(+), 19 deletions(-) diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index a56e20ba2b..4b391ac612 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -114,12 +114,10 @@ int xenmem_add_to_physmap_one( switch ( space ) { case XENMAPSPACE_grant_table: - rc = gnttab_map_frame(d, idx, gfn, &mfn); + rc = gnttab_map_frame_begin(d, idx, gfn, &mfn); if ( rc ) return rc; - /* Need to take care of the reference obtained in gnttab_map_frame(). */ - page = mfn_to_page(mfn); t = p2m_ram_rw; break; @@ -221,10 +219,23 @@ int xenmem_add_to_physmap_one( * to drop the reference we took earlier. In all other cases we need to * drop any reference we took earlier (perhaps indirectly). */ - if ( space == XENMAPSPACE_gmfn_foreign ? rc : page != NULL ) + switch ( space ) { + default: + if ( page ) + put_page(page); + break; + + case XENMAPSPACE_grant_table: + gnttab_map_frame_end(d, mfn); + break; + + case XENMAPSPACE_gmfn_foreign: + if ( !rc ) + break; ASSERT(page != NULL); put_page(page); + break; } return rc; diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index a317f67ec0..29f5c7c279 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -1984,11 +1984,9 @@ int xenmem_add_to_physmap_one( break; case XENMAPSPACE_grant_table: - rc = gnttab_map_frame(d, idx, gfn, &mfn); + rc = gnttab_map_frame_begin(d, idx, gfn, &mfn); if ( rc ) return rc; - /* Need to take care of the reference obtained in gnttab_map_frame(). */ - page = mfn_to_page(mfn); break; case XENMAPSPACE_gmfn: @@ -2070,19 +2068,28 @@ int xenmem_add_to_physmap_one( put_gfn(d, gfn_x(gfn)); put_both: - /* - * In the XENMAPSPACE_gmfn case, we took a ref of the gfn at the top. - * We also may need to transfer ownership of the page reference to our - * caller. - */ - if ( space == XENMAPSPACE_gmfn ) + switch ( space ) { + case XENMAPSPACE_gmfn: + /* + * We took a ref of the gfn at the top. We also may need to transfer + * ownership of the page reference to our caller. + */ put_gfn(d, gmfn); if ( !rc && extra.ppage ) { *extra.ppage = page; page = NULL; } + break; + + case XENMAPSPACE_grant_table: + /* + * We (gnttab_map_frame_begin()) acquired a lock and took a ref of the + * page underlying the MFN at the top. + */ + gnttab_map_frame_end(d, mfn); + break; } if ( page ) diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index 6c77867f8c..e0f306f268 100644 --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -4242,7 +4242,8 @@ int gnttab_acquire_resource( return rc; } -int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn, mfn_t *mfn) +int gnttab_map_frame_begin( + struct domain *d, unsigned long idx, gfn_t gfn, mfn_t *mfn) { int rc = 0; struct grant_table *gt = d->grant_table; @@ -4280,11 +4281,19 @@ int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn, mfn_t *mfn) put_page(pg); } - grant_write_unlock(gt); + if ( rc ) + grant_write_unlock(d->grant_table); return rc; } +void gnttab_map_frame_end(struct domain *d, mfn_t mfn) +{ + put_page(mfn_to_page(mfn)); + + grant_write_unlock(d->grant_table); +} + static void gnttab_usage_print(struct domain *rd) { int first = 1; diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h index 50edfecfb6..a1db21e3fe 100644 --- a/xen/include/xen/grant_table.h +++ b/xen/include/xen/grant_table.h @@ -56,8 +56,13 @@ int gnttab_release_mappings(struct domain *d); int mem_sharing_gref_to_gfn(struct grant_table *gt, grant_ref_t ref, gfn_t *gfn, uint16_t *status); -int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn, - mfn_t *mfn); +/* + * These need to be used as a pair, as the first (in the success case) returns + * with a lock and page reference held which the second needs to drop. + */ +int gnttab_map_frame_begin(struct domain *d, unsigned long idx, gfn_t gfn, + mfn_t *mfn); +void gnttab_map_frame_end(struct domain *d, mfn_t mfn); unsigned int gnttab_resource_max_frames(const struct domain *d, unsigned int id); @@ -96,12 +101,14 @@ static inline int mem_sharing_gref_to_gfn(struct grant_table *gt, return -EINVAL; } -static inline int gnttab_map_frame(struct domain *d, unsigned long idx, - gfn_t gfn, mfn_t *mfn) +static inline int gnttab_map_frame_begin(struct domain *d, unsigned long idx, + gfn_t gfn, mfn_t *mfn) { return -EINVAL; } +static inline void gnttab_map_frame_end(struct domain *d, mfn_t mfn) {} + static inline unsigned int gnttab_resource_max_frames( const struct domain *d, unsigned int id) {