From ef6455a3707cf1c7c61ad8af12558811eeee4ba8 Mon Sep 17 00:00:00 2001 From: Jan Beulich Date: Wed, 8 Sep 2021 14:53:04 +0200 Subject: [PATCH] gnttab: deal with status frame mapping race Once gnttab_map_frame() drops the grant table lock, the MFN it reports back to its caller is free to other manipulation. In particular gnttab_unpopulate_status_frames() might free it, by a racing request on another CPU, thus resulting in a reference to a deallocated page getting added to a domain's P2M. Obtain a page reference in gnttab_map_frame() to prevent freeing of the page until xenmem_add_to_physmap_one() has actually completed its acting on the page. Do so uniformly, even if only strictly required for v2 status pages, to avoid extra conditionals (which then would all need to be kept in sync going forward). This is CVE-2021-28701 / XSA-384. Reported-by: Julien Grall Signed-off-by: Jan Beulich Reviewed-by: Julien Grall master commit: eb6bbf7b30da5bae87932514d54d0e3c68b23757 master date: 2021-09-08 14:37:45 +0200 --- xen/arch/arm/mm.c | 11 ++++++++--- xen/arch/x86/mm.c | 2 ++ xen/common/grant_table.c | 11 ++++++++++- 3 files changed, 20 insertions(+), 4 deletions(-) diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index 38200e2533..e6c0a0d353 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -1423,6 +1423,8 @@ int xenmem_add_to_physmap_one( 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; @@ -1490,9 +1492,12 @@ int xenmem_add_to_physmap_one( /* Map at new location. */ rc = guest_physmap_add_entry(d, gfn, mfn, 0, t); - /* If we fail to add the mapping, we need to drop the reference we - * took earlier on foreign pages */ - if ( rc && space == XENMAPSPACE_gmfn_foreign ) + /* + * For XENMAPSPACE_gmfn_foreign if we failed to add the mapping, we need + * 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 ) { ASSERT(page != NULL); put_page(page); diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 56bad5b3ea..689a430d3b 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -4620,6 +4620,8 @@ int xenmem_add_to_physmap_one( rc = gnttab_map_frame(d, idx, gpfn, &mfn); if ( rc ) return rc; + /* Need to take care of the ref obtained in gnttab_map_frame(). */ + page = mfn_to_page(mfn); break; case XENMAPSPACE_gmfn: { diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index 86e298dfc0..f8486c3de7 100644 --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -4105,7 +4105,16 @@ int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn, mfn_t *mfn) } if ( !rc ) - gnttab_set_frame_gfn(gt, status, idx, gfn); + { + /* + * Make sure gnttab_unpopulate_status_frames() won't (successfully) + * free the page until our caller has completed its operation. + */ + if ( get_page(mfn_to_page(*mfn), d) ) + gnttab_set_frame_gfn(gt, status, idx, gfn); + else + rc = -EBUSY; + } grant_write_unlock(gt); -- 2.30.2