diff options
Diffstat (limited to 'system/xen/xsa/xsa218-0002-gnttab-Avoid-potential-double-put-of-maptrack-entry.patch')
| -rw-r--r-- | system/xen/xsa/xsa218-0002-gnttab-Avoid-potential-double-put-of-maptrack-entry.patch | 231 |
1 files changed, 231 insertions, 0 deletions
diff --git a/system/xen/xsa/xsa218-0002-gnttab-Avoid-potential-double-put-of-maptrack-entry.patch b/system/xen/xsa/xsa218-0002-gnttab-Avoid-potential-double-put-of-maptrack-entry.patch new file mode 100644 index 000000000000..b07c978b3c81 --- /dev/null +++ b/system/xen/xsa/xsa218-0002-gnttab-Avoid-potential-double-put-of-maptrack-entry.patch @@ -0,0 +1,231 @@ +From bb6d476b09e635baf5e9fb22540ab7c3530d1d98 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 2/3] 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 | 77 +++++++++++++++++++++++++++++++++--------------- + 1 file changed, 53 insertions(+), 24 deletions(-) + +diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c +index d80bd49..ba10e76 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 put_handle = false; + + 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); + 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 = true; + } + + 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 + |