Differential D16416 Diff 46059 head/emulators/xen-kernel47/files/0003-gnttab-Avoid-potential-double-put-of-maptrack-entry.patch
Changeset View
Changeset View
Standalone View
Standalone View
head/emulators/xen-kernel47/files/0003-gnttab-Avoid-potential-double-put-of-maptrack-entry.patch
Property | Old Value | New Value |
---|---|---|
fbsd:nokeywords | null | yes \ No newline at end of property |
svn:eol-style | null | native \ No newline at end of property |
svn:mime-type | null | text/plain \ No newline at end of property |
From 39b704785a8d330c02e8e2d2368c80dbaf679bc0 Mon Sep 17 00:00:00 2001 | |||||
From: George Dunlap <george.dunlap@citrix.com> | |||||
Date: Thu, 15 Jun 2017 12:05:14 +0100 | |||||
Subject: [PATCH 3/4] gnttab: Avoid potential double-put of maptrack entry | |||||
Each grant mapping for a particular domain is tracked by an in-Xen | |||||
"maptrack" entry. This entry is is referenced by a "handle", which is | |||||
given to the guest when it calls gnttab_map_grant_ref(). | |||||
There are two types of mapping a particular handle can refer to: | |||||
GNTMAP_host_map and GNTMAP_device_map. A given | |||||
gnttab_unmap_grant_ref() call can remove either only one or both of | |||||
these entries. When a particular handle has no entries left, it must | |||||
be freed. | |||||
gnttab_unmap_grant_ref() loops through its grant unmap request list | |||||
twice. It first removes entries from any host pagetables and (if | |||||
appropraite) iommus; then it does a single domain TLB flush; then it | |||||
does the clean-up, including telling the granter that entries are no | |||||
longer being used (if appropriate). | |||||
At the moment, it's during the first pass that the maptrack flags are | |||||
cleared, but the second pass that the maptrack entry is freed. | |||||
Unfortunately this allows the following race, which results in a | |||||
double-free: | |||||
A: (pass 1) clear host_map | |||||
B: (pass 1) clear device_map | |||||
A: (pass 2) See that maptrack entry has no mappings, free it | |||||
B: (pass 2) See that maptrack entry has no mappings, free it # | |||||
Unfortunately, unlike the active entry pinning update, we can't simply | |||||
move the maptrack flag changes to the second half, because the | |||||
maptrack flags are used to determine if iommu entries need to be | |||||
added: a domain's iommu must never have fewer permissions than the | |||||
maptrack flags indicate, or a subsequent map_grant_ref() might fail to | |||||
add the necessary iommu entries. | |||||
Instead, free the maptrack entry in the first pass if there are no | |||||
further mappings. | |||||
This is part of XSA-218. | |||||
Reported-by: Jan Beulich <jbeulich.com> | |||||
Signed-off-by: George Dunlap <george.dunlap@citrix.com> | |||||
Signed-off-by: Jan Beulich <jbeulich@suse.com> | |||||
--- | |||||
xen/common/grant_table.c | 79 +++++++++++++++++++++++++++++++++--------------- | |||||
1 file changed, 54 insertions(+), 25 deletions(-) | |||||
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c | |||||
index cfc483f..81a1a8b 100644 | |||||
--- a/xen/common/grant_table.c | |||||
+++ b/xen/common/grant_table.c | |||||
@@ -98,8 +98,8 @@ struct gnttab_unmap_common { | |||||
/* Shared state beteen *_unmap and *_unmap_complete */ | |||||
u16 flags; | |||||
unsigned long frame; | |||||
- struct grant_mapping *map; | |||||
struct domain *rd; | |||||
+ grant_ref_t ref; | |||||
}; | |||||
/* Number of unmap operations that are done between each tlb flush */ | |||||
@@ -1079,6 +1079,8 @@ __gnttab_unmap_common( | |||||
struct grant_table *lgt, *rgt; | |||||
struct active_grant_entry *act; | |||||
s16 rc = 0; | |||||
+ struct grant_mapping *map; | |||||
+ bool_t put_handle = 0; | |||||
ld = current->domain; | |||||
lgt = ld->grant_table; | |||||
@@ -1092,11 +1094,11 @@ __gnttab_unmap_common( | |||||
return; | |||||
} | |||||
- op->map = &maptrack_entry(lgt, op->handle); | |||||
+ map = &maptrack_entry(lgt, op->handle); | |||||
grant_read_lock(lgt); | |||||
- if ( unlikely(!read_atomic(&op->map->flags)) ) | |||||
+ if ( unlikely(!read_atomic(&map->flags)) ) | |||||
{ | |||||
grant_read_unlock(lgt); | |||||
gdprintk(XENLOG_INFO, "Zero flags for handle (%d).\n", op->handle); | |||||
@@ -1104,7 +1106,7 @@ __gnttab_unmap_common( | |||||
return; | |||||
} | |||||
- dom = op->map->domid; | |||||
+ dom = map->domid; | |||||
grant_read_unlock(lgt); | |||||
if ( unlikely((rd = rcu_lock_domain_by_id(dom)) == NULL) ) | |||||
@@ -1129,16 +1131,43 @@ __gnttab_unmap_common( | |||||
grant_read_lock(rgt); | |||||
- op->flags = read_atomic(&op->map->flags); | |||||
- if ( unlikely(!op->flags) || unlikely(op->map->domid != dom) ) | |||||
+ op->rd = rd; | |||||
+ op->ref = map->ref; | |||||
+ | |||||
+ /* | |||||
+ * We can't assume there was no racing unmap for this maptrack entry, | |||||
+ * and hence we can't assume map->ref is valid for rd. While the checks | |||||
+ * below (with the active entry lock held) will reject any such racing | |||||
+ * requests, we still need to make sure we don't attempt to acquire an | |||||
+ * invalid lock. | |||||
+ */ | |||||
+ smp_rmb(); | |||||
+ if ( unlikely(op->ref >= nr_grant_entries(rgt)) ) | |||||
{ | |||||
- gdprintk(XENLOG_WARNING, "Unstable handle %u\n", op->handle); | |||||
+ gdprintk(XENLOG_WARNING, "Unstable handle %#x\n", op->handle); | |||||
rc = GNTST_bad_handle; | |||||
- goto unmap_out; | |||||
+ goto unlock_out; | |||||
} | |||||
- op->rd = rd; | |||||
- act = active_entry_acquire(rgt, op->map->ref); | |||||
+ act = active_entry_acquire(rgt, op->ref); | |||||
+ | |||||
+ /* | |||||
+ * Note that we (ab)use the active entry lock here to protect against | |||||
+ * multiple unmaps of the same mapping here. We don't want to hold lgt's | |||||
+ * lock, and we only hold rgt's lock for reading (but the latter wouldn't | |||||
+ * be the right one anyway). Hence the easiest is to rely on a lock we | |||||
+ * hold anyway; see docs/misc/grant-tables.txt's "Locking" section. | |||||
+ */ | |||||
+ | |||||
+ op->flags = read_atomic(&map->flags); | |||||
+ smp_rmb(); | |||||
+ if ( unlikely(!op->flags) || unlikely(map->domid != dom) || | |||||
+ unlikely(map->ref != op->ref) ) | |||||
+ { | |||||
+ gdprintk(XENLOG_WARNING, "Unstable handle %#x\n", op->handle); | |||||
+ rc = GNTST_bad_handle; | |||||
+ goto act_release_out; | |||||
+ } | |||||
if ( op->frame == 0 ) | |||||
{ | |||||
@@ -1151,7 +1180,7 @@ __gnttab_unmap_common( | |||||
"Bad frame number doesn't match gntref. (%lx != %lx)\n", | |||||
op->frame, act->frame); | |||||
- op->map->flags &= ~GNTMAP_device_map; | |||||
+ map->flags &= ~GNTMAP_device_map; | |||||
} | |||||
if ( (op->host_addr != 0) && (op->flags & GNTMAP_host_map) ) | |||||
@@ -1161,14 +1190,23 @@ __gnttab_unmap_common( | |||||
op->flags)) < 0 ) | |||||
goto act_release_out; | |||||
- op->map->flags &= ~GNTMAP_host_map; | |||||
+ map->flags &= ~GNTMAP_host_map; | |||||
+ } | |||||
+ | |||||
+ if ( !(map->flags & (GNTMAP_device_map|GNTMAP_host_map)) ) | |||||
+ { | |||||
+ map->flags = 0; | |||||
+ put_handle = 1; | |||||
} | |||||
act_release_out: | |||||
active_entry_release(act); | |||||
- unmap_out: | |||||
+ unlock_out: | |||||
grant_read_unlock(rgt); | |||||
+ if ( put_handle ) | |||||
+ put_maptrack_handle(lgt, op->handle); | |||||
+ | |||||
if ( rc == GNTST_okay && gnttab_need_iommu_mapping(ld) ) | |||||
{ | |||||
unsigned int kind; | |||||
@@ -1205,7 +1243,6 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op) | |||||
grant_entry_header_t *sha; | |||||
struct page_info *pg; | |||||
uint16_t *status; | |||||
- bool_t put_handle = 0; | |||||
if ( rd == NULL ) | |||||
{ | |||||
@@ -1226,13 +1263,13 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op) | |||||
if ( rgt->gt_version == 0 ) | |||||
goto unlock_out; | |||||
- act = active_entry_acquire(rgt, op->map->ref); | |||||
- sha = shared_entry_header(rgt, op->map->ref); | |||||
+ act = active_entry_acquire(rgt, op->ref); | |||||
+ sha = shared_entry_header(rgt, op->ref); | |||||
if ( rgt->gt_version == 1 ) | |||||
status = &sha->flags; | |||||
else | |||||
- status = &status_entry(rgt, op->map->ref); | |||||
+ status = &status_entry(rgt, op->ref); | |||||
if ( unlikely(op->frame != act->frame) ) | |||||
{ | |||||
@@ -1289,9 +1326,6 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op) | |||||
act->pin -= GNTPIN_hstw_inc; | |||||
} | |||||
- if ( (op->map->flags & (GNTMAP_device_map|GNTMAP_host_map)) == 0 ) | |||||
- put_handle = 1; | |||||
- | |||||
if ( ((act->pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask)) == 0) && | |||||
!(op->flags & GNTMAP_readonly) ) | |||||
gnttab_clear_flag(_GTF_writing, status); | |||||
@@ -1304,11 +1338,6 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op) | |||||
unlock_out: | |||||
grant_read_unlock(rgt); | |||||
- if ( put_handle ) | |||||
- { | |||||
- op->map->flags = 0; | |||||
- put_maptrack_handle(ld->grant_table, op->handle); | |||||
- } | |||||
rcu_unlock_domain(rd); | |||||
} | |||||
-- | |||||
2.1.4 | |||||