From d27237abe37e45a1f245e23484062b09ff3477ed Mon Sep 17 00:00:00 2001
From: Jan Beulich <jbeulich@suse.com>
Date: Thu, 15 Jun 2017 16:25:27 +0100
Subject: [PATCH 4/4] gnttab: __gnttab_unmap_common_complete() is
all-or-nothing
All failures have to be detected in __gnttab_unmap_common(), the
completion function must not skip part of its processing. In particular
the GNTMAP_device_map related putting of page references and adjustment
of pin count must not occur if __gnttab_unmap_common() signaled an
error. Furthermore the function must not make adjustments to global
state (here: clearing GNTTAB_device_map) before all possibly failing
operations have been performed.
There's one exception for IOMMU related failures: As IOMMU manipulation
occurs after GNTMAP_*_map have been cleared already, the related page
reference and pin count adjustments need to be done nevertheless. A
fundamental requirement for the correctness of this is that
iommu_{,un}map_page() crash any affected DomU in case of failure.
The version check appears to be pointless (or could perhaps be a
BUG_ON() or ASSERT()), but for the moment also move it.
This is part of XSA-224.
Reported-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
xen/common/grant_table.c | 108 ++++++++++++++++++--------------------
xen/include/asm-arm/grant_table.h | 2 +-
xen/include/asm-x86/grant_table.h | 5 +-
3 files changed, 55 insertions(+), 60 deletions(-)
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index d07b931..7ea68b1 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -96,7 +96,7 @@ struct gnttab_unmap_common {
int16_t status;
/* Shared state beteen *_unmap and *_unmap_complete */
- u16 flags;
+ u16 done;
unsigned long frame;
struct domain *rd;
grant_ref_t ref;
@@ -944,7 +944,8 @@ __gnttab_map_grant_ref(
refcnt++;
}
- if ( gnttab_host_mapping_get_page_type(op, ld, rd) )
+ if ( gnttab_host_mapping_get_page_type(op->flags & GNTMAP_readonly,
+ ld, rd) )
{
if ( (owner == dom_cow) ||
!get_page_type(pg, PGT_writable_page) )
@@ -1091,6 +1092,7 @@ __gnttab_unmap_common(
struct active_grant_entry *act;
s16 rc = 0;
struct grant_mapping *map;
+ unsigned int flags;
bool put_handle = false;
ld = current->domain;
@@ -1140,6 +1142,20 @@ __gnttab_unmap_common(
grant_read_lock(rgt);
+ if ( rgt->gt_version == 0 )
+ {
+ /*
+ * This ought to be impossible, as such a mapping should not have
+ * been established (see the nr_grant_entries(rgt) bounds check in
+ * __gnttab_map_grant_ref()). Doing this check only in
+ * __gnttab_unmap_common_complete() - as it used to be done - would,
+ * however, be too late.
+ */
+ rc = GNTST_bad_gntref;
+ flags = 0;
+ goto unlock_out;
+ }
+
op->rd = rd;
op->ref = map->ref;
@@ -1155,6 +1171,7 @@ __gnttab_unmap_common(
{
gdprintk(XENLOG_WARNING, "Unstable handle %u\n", op->handle);
rc = GNTST_bad_handle;
+ flags = 0;
goto unlock_out;
}
@@ -1168,9 +1185,9 @@ __gnttab_unmap_common(
* hold anyway; see docs/misc/grant-tables.txt's "Locking" section.
*/
- op->flags = read_atomic(&map->flags);
+ flags = read_atomic(&map->flags);
smp_rmb();
- if ( unlikely(!op->flags) || unlikely(map->domid != dom) ||
+ if ( unlikely(!flags) || unlikely(map->domid != dom) ||
unlikely(map->ref != op->ref) )
{
gdprintk(XENLOG_WARNING, "Unstable handle %#x\n", op->handle);
@@ -1180,24 +1197,27 @@ __gnttab_unmap_common(
op->frame = act->frame;
- if ( op->dev_bus_addr )
- {
- if ( unlikely(op->dev_bus_addr != pfn_to_paddr(act->frame)) )
- PIN_FAIL(act_release_out, GNTST_general_error,
- "Bus address doesn't match gntref (%"PRIx64" != %"PRIpaddr")\n",
- op->dev_bus_addr, pfn_to_paddr(act->frame));
-
- map->flags &= ~GNTMAP_device_map;
- }
+ if ( op->dev_bus_addr &&
+ unlikely(op->dev_bus_addr != pfn_to_paddr(act->frame)) )
+ PIN_FAIL(act_release_out, GNTST_general_error,
+ "Bus address doesn't match gntref (%"PRIx64" != %"PRIpaddr")\n",
+ op->dev_bus_addr, pfn_to_paddr(act->frame));
- if ( (op->host_addr != 0) && (op->flags & GNTMAP_host_map) )
+ if ( op->host_addr && (flags & GNTMAP_host_map) )
{
if ( (rc = replace_grant_host_mapping(op->host_addr,
op->frame, op->new_addr,
- op->flags)) < 0 )
+ flags)) < 0 )
goto act_release_out;
map->flags &= ~GNTMAP_host_map;
+ op->done |= GNTMAP_host_map | (flags & GNTMAP_readonly);
+ }
+
+ if ( op->dev_bus_addr && (flags & GNTMAP_device_map) )
+ {
+ map->flags &= ~GNTMAP_device_map;
+ op->done |= GNTMAP_device_map | (flags & GNTMAP_readonly);
}
if ( !(map->flags & (GNTMAP_device_map|GNTMAP_host_map)) )
@@ -1234,7 +1254,7 @@ __gnttab_unmap_common(
}
/* If just unmapped a writable mapping, mark as dirtied */
- if ( rc == GNTST_okay && !(op->flags & GNTMAP_readonly) )
+ if ( rc == GNTST_okay && !(flags & GNTMAP_readonly) )
gnttab_mark_dirty(rd, op->frame);
op->status = rc;
@@ -1251,13 +1271,9 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op)
struct page_info *pg;
uint16_t *status;
- if ( rd == NULL )
+ if ( !op->done )
{
- /*
- * Suggests that __gntab_unmap_common failed in
- * rcu_lock_domain_by_id() or earlier, and so we have nothing
- * to complete
- */
+ /* __gntab_unmap_common() didn't do anything - nothing to complete. */
return;
}
@@ -1267,8 +1283,6 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op)
rgt = rd->grant_table;
grant_read_lock(rgt);
- if ( rgt->gt_version == 0 )
- goto unlock_out;
act = active_entry_acquire(rgt, op->ref);
sha = shared_entry_header(rgt, op->ref);
@@ -1278,72 +1292,50 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op)
else
status = &status_entry(rgt, op->ref);
- if ( op->dev_bus_addr &&
- unlikely(op->dev_bus_addr != pfn_to_paddr(act->frame)) )
- {
- /*
- * Suggests that __gntab_unmap_common failed early and so
- * nothing further to do
- */
- goto act_release_out;
- }
-
pg = mfn_to_page(op->frame);
- if ( op->dev_bus_addr && (op->flags & GNTMAP_device_map) )
+ if ( op->done & GNTMAP_device_map )
{
if ( !is_iomem_page(act->frame) )
{
- if ( op->flags & GNTMAP_readonly )
+ if ( op->done & GNTMAP_readonly )
put_page(pg);
else
put_page_and_type(pg);
}
ASSERT(act->pin & (GNTPIN_devw_mask | GNTPIN_devr_mask));
- if ( op->flags & GNTMAP_readonly )
+ if ( op->done & GNTMAP_readonly )
act->pin -= GNTPIN_devr_inc;
else
act->pin -= GNTPIN_devw_inc;
}
- if ( (op->host_addr != 0) && (op->flags & GNTMAP_host_map) )
+ if ( op->done & GNTMAP_host_map )
{
- if ( op->status != 0 )
+ if ( !is_iomem_page(op->frame) )
{
- /*
- * Suggests that __gntab_unmap_common failed in
- * replace_grant_host_mapping() or IOMMU handling, so nothing
- * further to do (short of re-establishing the mapping in the
- * latter case).
- */
- goto act_release_out;
- }
-
- if ( !is_iomem_page(op->frame) )
- {
- if ( gnttab_host_mapping_get_page_type(op, ld, rd) )
+ if ( gnttab_host_mapping_get_page_type(op->done & GNTMAP_readonly,
+ ld, rd) )
put_page_type(pg);
put_page(pg);
}
ASSERT(act->pin & (GNTPIN_hstw_mask | GNTPIN_hstr_mask));
- if ( op->flags & GNTMAP_readonly )
+ if ( op->done & GNTMAP_readonly )
act->pin -= GNTPIN_hstr_inc;
else
act->pin -= GNTPIN_hstw_inc;
}
if ( ((act->pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask)) == 0) &&
- !(op->flags & GNTMAP_readonly) )
+ !(op->done & GNTMAP_readonly) )
gnttab_clear_flag(_GTF_writing, status);
if ( act->pin == 0 )
gnttab_clear_flag(_GTF_reading, status);
- act_release_out:
active_entry_release(act);
- unlock_out:
grant_read_unlock(rgt);
rcu_unlock_domain(rd);
@@ -1359,6 +1351,7 @@ __gnttab_unmap_grant_ref(
common->handle = op->handle;
/* Intialise these in case common contains old state */
+ common->done = 0;
common->new_addr = 0;
common->rd = NULL;
common->frame = 0;
@@ -1424,6 +1417,7 @@ __gnttab_unmap_and_replace(
common->handle = op->handle;
/* Intialise these in case common contains old state */
+ common->done = 0;
common->dev_bus_addr = 0;
common->rd = NULL;
common->frame = 0;
@@ -3385,7 +3379,9 @@ gnttab_release_mappings(
if ( gnttab_release_host_mappings(d) &&
!is_iomem_page(act->frame) )
{
- if ( gnttab_host_mapping_get_page_type(map, d, rd) )
+ if ( gnttab_host_mapping_get_page_type((map->flags &
+ GNTMAP_readonly),
+ d, rd) )
put_page_type(pg);
put_page(pg);
}
diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h
index eb02423..bc4d61a 100644
--- a/xen/include/asm-arm/grant_table.h
+++ b/xen/include/asm-arm/grant_table.h
@@ -9,7 +9,7 @@ void gnttab_clear_flag(unsigned long nr, uint16_t *addr);
int create_grant_host_mapping(unsigned long gpaddr,
unsigned long mfn, unsigned int flags, unsigned int
cache_flags);
-#define gnttab_host_mapping_get_page_type(op, d, rd) (0)
+#define gnttab_host_mapping_get_page_type(ro, ld, rd) (0)
int replace_grant_host_mapping(unsigned long gpaddr, unsigned long mfn,
unsigned long new_gpaddr, unsigned int flags);
void gnttab_mark_dirty(struct domain *d, unsigned long l);
diff --git a/xen/include/asm-x86/grant_table.h b/xen/include/asm-x86/grant_table.h
index 8c9bbcf..9ca631c 100644
--- a/xen/include/asm-x86/grant_table.h
+++ b/xen/include/asm-x86/grant_table.h
@@ -58,9 +58,8 @@ static inline void gnttab_clear_flag(unsigned int nr, uint16_t *st)
}
/* Foreign mappings of HHVM-guest pages do not modify the type count. */
-#define gnttab_host_mapping_get_page_type(op, ld, rd) \
- (!((op)->flags & GNTMAP_readonly) && \
- (((ld) == (rd)) || !paging_mode_external(rd)))
+#define gnttab_host_mapping_get_page_type(ro, ld, rd) \
+ (!(ro) && (((ld) == (rd)) || !paging_mode_external(rd)))
/* Done implicitly when page tables are destroyed. */
#define gnttab_release_host_mappings(domain) ( paging_mode_external(domain) )
--
2.1.4