Skip to content

Commit

Permalink
gnttab: add preemption check to gnttab_release_mappings()
Browse files Browse the repository at this point in the history
A guest may die with many grant mappings still in place, or simply with
a large maptrack table. Iterating through this may take more time than
is reasonable without intermediate preemption (to run softirqs and
perhaps the scheduler).

Move the invocation of the function to the section where other
restartable functions get invoked, and have the function itself check
for preemption every once in a while. Have it iterate the table
backwards, such that decreasing the maptrack limit is all it takes to
convey restart information.

In domain_teardown() introduce PROG_none such that inserting at the
front will be easier going forward.

This is part of CVE-2021-28698 / XSA-380.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
  • Loading branch information
jbeulich committed Aug 25, 2021
1 parent f147422 commit b1ee10b
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 14 deletions.
12 changes: 9 additions & 3 deletions xen/common/domain.c
Original file line number Diff line number Diff line change
Expand Up @@ -412,11 +412,18 @@ static int domain_teardown(struct domain *d)
v = d->teardown.vcpu

enum {
PROG_vcpu_teardown = 1,
PROG_none,
PROG_gnttab_mappings,
PROG_vcpu_teardown,
PROG_done,
};

case 0:
case PROG_none:
rc = gnttab_release_mappings(d);
if ( rc )
return rc;

PROGRESS(gnttab_mappings):
for_each_vcpu ( d, v )
{
PROGRESS_VCPU(teardown);
Expand Down Expand Up @@ -908,7 +915,6 @@ int domain_kill(struct domain *d)
return domain_kill(d);
d->is_dying = DOMDYING_dying;
argo_destroy(d);
gnttab_release_mappings(d);
vnuma_destroy(d->vnuma);
domain_set_outstanding_pages(d, 0);
/* fallthrough */
Expand Down
46 changes: 39 additions & 7 deletions xen/common/grant_table.c
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,13 @@ struct grant_table {
unsigned int nr_grant_frames;
/* Number of grant status frames shared with guest (for version 2) */
unsigned int nr_status_frames;
/* Number of available maptrack entries. */
/*
* Number of available maptrack entries. For cleanup purposes it is
* important to realize that this field and @maptrack further down will
* only ever be accessed by the local domain. Thus it is okay to clean
* up early, and to shrink the limit for the purpose of tracking cleanup
* progress.
*/
unsigned int maptrack_limit;
/* Shared grant table (see include/public/grant_table.h). */
union {
Expand Down Expand Up @@ -3679,9 +3685,7 @@ do_grant_table_op(
#include "compat/grant_table.c"
#endif

void
gnttab_release_mappings(
struct domain *d)
int gnttab_release_mappings(struct domain *d)
{
struct grant_table *gt = d->grant_table, *rgt;
struct grant_mapping *map;
Expand All @@ -3695,8 +3699,32 @@ gnttab_release_mappings(

BUG_ON(!d->is_dying);

for ( handle = 0; handle < gt->maptrack_limit; handle++ )
if ( !gt || !gt->maptrack )
return 0;

for ( handle = gt->maptrack_limit; handle; )
{
/*
* Deal with full pages such that their freeing (in the body of the
* if()) remains simple.
*/
if ( handle < gt->maptrack_limit && !(handle % MAPTRACK_PER_PAGE) )
{
/*
* Changing maptrack_limit alters nr_maptrack_frames()'es return
* value. Free the then excess trailing page right here, rather
* than leaving it to grant_table_destroy() (and in turn requiring
* to leave gt->maptrack_limit unaltered).
*/
gt->maptrack_limit = handle;
FREE_XENHEAP_PAGE(gt->maptrack[nr_maptrack_frames(gt)]);

if ( hypercall_preempt_check() )
return -ERESTART;
}

--handle;

map = &maptrack_entry(gt, handle);
if ( !(map->flags & (GNTMAP_device_map|GNTMAP_host_map)) )
continue;
Expand Down Expand Up @@ -3780,6 +3808,11 @@ gnttab_release_mappings(

map->flags = 0;
}

gt->maptrack_limit = 0;
FREE_XENHEAP_PAGE(gt->maptrack[0]);

return 0;
}

void grant_table_warn_active_grants(struct domain *d)
Expand Down Expand Up @@ -3843,8 +3876,7 @@ grant_table_destroy(
free_xenheap_page(t->shared_raw[i]);
xfree(t->shared_raw);

for ( i = 0; i < nr_maptrack_frames(t); i++ )
free_xenheap_page(t->maptrack[i]);
ASSERT(!t->maptrack_limit);
vfree(t->maptrack);

for ( i = 0; i < nr_active_grant_frames(t); i++ )
Expand Down
6 changes: 2 additions & 4 deletions xen/include/xen/grant_table.h
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,7 @@ void grant_table_init_vcpu(struct vcpu *v);
void grant_table_warn_active_grants(struct domain *d);

/* Domain death release of granted mappings of other domains' memory. */
void
gnttab_release_mappings(
struct domain *d);
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);
Expand Down Expand Up @@ -80,7 +78,7 @@ static inline void grant_table_init_vcpu(struct vcpu *v) {}

static inline void grant_table_warn_active_grants(struct domain *d) {}

static inline void gnttab_release_mappings(struct domain *d) {}
static inline int gnttab_release_mappings(struct domain *d) { return 0; }

static inline int mem_sharing_gref_to_gfn(struct grant_table *gt,
grant_ref_t ref,
Expand Down

0 comments on commit b1ee10b

Please sign in to comment.