[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [xen staging] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame()
commit 1b78d4c63687fbcd0baacb13a7bd97605ffe3b88 Author: Jan Beulich <jbeulich@xxxxxxxx> AuthorDate: Fri Dec 3 13:54:28 2021 +0100 Commit: Jan Beulich <jbeulich@xxxxxxxx> CommitDate: Fri Dec 3 13:54:28 2021 +0100 gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame() Without holding appropriate locks, attempting to remove a prior mapping of the underlying page is pointless, as the same (or another) mapping could be re-established by a parallel request on another vCPU. Move the code to Arm's gnttab_set_frame_gfn(); it cannot be dropped there since xenmem_add_to_physmap_one() doesn't call it either (unlike on x86). Of course this new placement doesn't improve things in any way as far as the security of grant status frame mappings goes (see XSA-379). Proper locking would be needed here to allow status frames to be mapped securely. In turn this then requires replacing the other use in gnttab_unpopulate_status_frames(), which yet in turn requires adjusting x86's gnttab_set_frame_gfn(). Note that with proper locking inside guest_physmap_remove_page() combined with checking the GFN's mapping there against the passed in MFN, there then is no issue with the involved multiple gnttab_set_frame_gfn()-s potentially returning varying values (due to a racing XENMAPSPACE_grant_table request). This, as a side effect, does away with gnttab_map_frame() having a local variable "gfn" which shadows a function parameter of the same name. Together with XSA-379 this points out that XSA-255's addition to gnttab_map_frame() was really useless. Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx> Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx> Acked-by: Julien Grall <jgrall@xxxxxxxxxx> --- xen/common/grant_table.c | 26 +++++++++++--------------- xen/include/asm-arm/grant_table.h | 16 +++++++++++----- xen/include/asm-x86/grant_table.h | 7 ++++++- 3 files changed, 28 insertions(+), 21 deletions(-) diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index 012a74455b..0262f2c48a 100644 --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -93,7 +93,7 @@ struct grant_table { struct radix_tree_root maptrack_tree; /* Domain to which this struct grant_table belongs. */ - const struct domain *domain; + struct domain *domain; struct grant_table_arch arch; }; @@ -1796,8 +1796,8 @@ gnttab_unpopulate_status_frames(struct domain *d, struct grant_table *gt) { int rc = gfn_eq(gfn, INVALID_GFN) ? 0 - : guest_physmap_remove_page(d, gfn, - page_to_mfn(pg), 0); + : gnttab_set_frame_gfn(gt, true, i, INVALID_GFN, + page_to_mfn(pg)); if ( rc ) { @@ -1807,7 +1807,6 @@ gnttab_unpopulate_status_frames(struct domain *d, struct grant_table *gt) domain_crash(d); return rc; } - gnttab_set_frame_gfn(gt, true, i, INVALID_GFN); } BUG_ON(page_get_owner(pg) != d); @@ -4150,6 +4149,9 @@ int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn, mfn_t *mfn) struct grant_table *gt = d->grant_table; bool status = false; + if ( gfn_eq(gfn, INVALID_GFN) ) + return -EINVAL; + grant_write_lock(gt); if ( evaluate_nospec(gt->gt_version == 2) && (idx & XENMAPIDX_grant_table_status) ) @@ -4162,24 +4164,18 @@ int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn, mfn_t *mfn) else rc = gnttab_get_shared_frame_mfn(d, idx, mfn); - if ( !rc && paging_mode_translate(d) ) - { - gfn_t gfn = gnttab_get_frame_gfn(gt, status, idx); - - if ( !gfn_eq(gfn, INVALID_GFN) ) - rc = guest_physmap_remove_page(d, gfn, *mfn, 0); - } - if ( !rc ) { + struct page_info *pg = mfn_to_page(*mfn); + /* * 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 + if ( !get_page(pg, d) ) rc = -EBUSY; + else if ( (rc = gnttab_set_frame_gfn(gt, status, idx, gfn, *mfn)) ) + put_page(pg); } grant_write_unlock(gt); diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h index 0ce77f9a1c..d31a4d6805 100644 --- a/xen/include/asm-arm/grant_table.h +++ b/xen/include/asm-arm/grant_table.h @@ -71,11 +71,17 @@ int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn, XFREE((gt)->arch.status_gfn); \ } while ( 0 ) -#define gnttab_set_frame_gfn(gt, st, idx, gfn) \ - do { \ - ((st) ? (gt)->arch.status_gfn : (gt)->arch.shared_gfn)[idx] = \ - (gfn); \ - } while ( 0 ) +#define gnttab_set_frame_gfn(gt, st, idx, gfn, mfn) \ + ({ \ + int rc_ = 0; \ + gfn_t ogfn = gnttab_get_frame_gfn(gt, st, idx); \ + if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) || \ + (rc_ = guest_physmap_remove_page((gt)->domain, ogfn, mfn, \ + 0)) == 0 ) \ + ((st) ? (gt)->arch.status_gfn \ + : (gt)->arch.shared_gfn)[idx] = (gfn); \ + rc_; \ + }) #define gnttab_get_frame_gfn(gt, st, idx) ({ \ (st) ? gnttab_status_gfn(NULL, gt, idx) \ diff --git a/xen/include/asm-x86/grant_table.h b/xen/include/asm-x86/grant_table.h index 84e32960c0..a8a21439a4 100644 --- a/xen/include/asm-x86/grant_table.h +++ b/xen/include/asm-x86/grant_table.h @@ -37,7 +37,12 @@ static inline int replace_grant_host_mapping(uint64_t addr, mfn_t frame, #define gnttab_init_arch(gt) 0 #define gnttab_destroy_arch(gt) do {} while ( 0 ) -#define gnttab_set_frame_gfn(gt, st, idx, gfn) do {} while ( 0 ) +#define gnttab_set_frame_gfn(gt, st, idx, gfn, mfn) \ + (gfn_eq(gfn, INVALID_GFN) \ + ? guest_physmap_remove_page((gt)->domain, \ + gnttab_get_frame_gfn(gt, st, idx), \ + mfn, 0) \ + : 0 /* Handled in add_to_physmap_one(). */) #define gnttab_get_frame_gfn(gt, st, idx) ({ \ mfn_t mfn_ = (st) ? gnttab_status_mfn(gt, idx) \ : gnttab_shared_mfn(gt, idx); \ -- generated by git-patchbot for /home/xen/git/xen.git#staging
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |