On Mon, Sep 13, 2021 at 08:41:47AM +0200, Jan Beulich wrote:
> 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(). 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 <jbeul...@suse.com>
>
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -92,7 +92,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;
> };
> @@ -1795,8 +1795,8 @@ gnttab_unpopulate_status_frames(struct d
> {
> 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 )
> {
> @@ -1806,7 +1806,6 @@ gnttab_unpopulate_status_frames(struct d
> domain_crash(d);
> return rc;
> }
> - gnttab_set_frame_gfn(gt, true, i, INVALID_GFN);
> }
>
> BUG_ON(page_get_owner(pg) != d);
> @@ -4132,6 +4131,9 @@ int gnttab_map_frame(struct domain *d, u
> 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) )
> @@ -4144,24 +4146,18 @@ int gnttab_map_frame(struct domain *d, u
> 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);
> --- 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
> 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); \
Newline maybe? Not sure whether we decided that macros should also
follow coding style regarding variable definition followed by newline.
> + if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) || \
I'm slightly confused by this checks, don't you need to check for
gfn_eq(gfn, INVALID_GFN) (not ogfn) in order to call
guest_physmap_remove_page?
Or assuming that ogfn is not invalid can be used to imply a removal?
Also the check for ogfn == gfn is only used on Arm, while I would
assume a similar one would be needed on x86 to guarantee the same
behavior?
Thanks, Roger.
