* Xen Security Advisory 224 (CVE-2017-10920, CVE-2017-10921, CVE-2017-10922) - grant table operations mishandle reference counts
@ 2017-07-07 13:54 Xen.org security team
0 siblings, 0 replies; only message in thread
From: Xen.org security team @ 2017-07-07 13:54 UTC (permalink / raw)
To: xen-announce, xen-devel, xen-users, oss-security; +Cc: Xen.org security team
[-- Attachment #1: Type: text/plain, Size: 7047 bytes --]
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
Xen Security Advisory CVE-2017-10920,CVE-2017-10921,CVE-2017-10922 / XSA-224
version 5
grant table operations mishandle reference counts
UPDATES IN VERSION 5
====================
CVEs assigned.
ISSUE DESCRIPTION
=================
We have discovered a number of bugs in the code mapping and unmapping
grant references.
* If a grant is mapped with both the GNTMAP_device_map and
GNTMAP_host_map flags, but unmapped only with host_map, the device_map
portion remains but the page reference counts are lowered as though it
had been removed. This bug can be leveraged cause a page's reference
counts and type counts to fall to zero while retaining writeable
mappings to the page. (CVE-2017-10920.)
* Under some specific conditions, if a grant is mapped with both the
GNTMAP_device_map and GNTMAP_host_map flags, the operation may not
grab sufficient type counts. When the grant is then unmapped, the
type count will be erroneously reduced. This bug can be leveraged
cause a page's reference counts and type counts to fall to zero while
retaining writeable mappings to the page. (CVE-2017-10921.)
* When a grant reference is given to an MMIO region (as opposed to a
normal guest page), if the grant is mapped with only the
GNTMAP_device_map flag set, a mapping is created at host_addr anyway.
This does *not* cause reference counts to change, but there will be no
record of this mapping, so it will not be considered when reporting
whether the grant is still in use. (CVE-2017-10922.)
IMPACT
======
For the worst issue, a PV guest could gain a writeable mapping of its
own pagetable, allowing it to escalate its privileges to that of the
host.
VULNERABLE SYSTEMS
==================
All versions of Xen are vulnerable.
Only x86 systems are vulnerable.
Any system running untrusted PV guests is vulnerable.
Systems with untrusted HVM guests are only vulnerable if those guests
are served by a trusted PV backend which is vulnerable: Namely, one
which calls grant_map() with both the GNTMAP_device_map and
GNTMAP_host_map flags. The security team is not aware of any backends
which are vulnerable.
MITIGATION
==========
Running only HVM guests will avoid this vulnerability.
CREDITS
=======
This issue was discovered by Jan Beulich of SUSE.
RESOLUTION
==========
Applying the appropriate set of attached patched resolves this issue.
Note that these patches are assumed to be applied on top of the XSA-218
ones; not doing so may cause at least mechanical problems of applying
the ones here.
xsa224-unstable/*.patch xen-unstable
xsa224-4.8/*.patch Xen 4.8.x
xsa224-4.7/*.patch Xen 4.7.x
xsa224-4.6/*.patch Xen 4.6.x
xsa224-4.5/*.patch Xen 4.5.x
$ sha256sum xsa224*/*
db39535185c1879775b62873fbed1e6285300ec1e1bd5d09ac2d96a98ac6443c xsa224-unstable/0001-gnttab-Fix-handling-of-dev_bus_addr-during-unmap.patch
1588257f5b0c7113cd478475014f56fbeb6e79de7acbe67cf6d7a265e2b3fa15 xsa224-unstable/0002-gnttab-never-create-host-mapping-unless-asked-to.patch
a7517ca0e253fb9fb5b1ea1e56d04167f32ef87be145462a15241af26e4e0d65 xsa224-unstable/0003-gnttab-correct-logic-to-get-page-references-during-m.patch
951217a88f9c945eb9f7933cd66615aef955206fab955020334ac54da05663fa xsa224-unstable/0004-gnttab-__gnttab_unmap_common_complete-is-all-or-noth.patch
190470fbd77fca58aab89a9bd034732525ce8f7ce7c417a0ca5d25b366639baa xsa224-4.5/0001-gnttab-Fix-handling-of-dev_bus_addr-during-unmap.patch
9374e4dd6666a63fb32e6cfbdc95071b0cc153ff7cb2d2efdd98468e0e079605 xsa224-4.5/0002-gnttab-never-create-host-mapping-unless-asked-to.patch
d825e6fa5827e28e3755c92b274044666cc91b6a8cbc16e2081f43e0371991d4 xsa224-4.5/0003-gnttab-correct-logic-to-get-page-references-during-m.patch
d3aaffaf487a84e43fe10f7dec5af72b64d1b2315440c36335a0ed8ec1439ca1 xsa224-4.5/0004-gnttab-__gnttab_unmap_common_complete-is-all-or-noth.patch
c6cd6b82ef774bec5eaad5f32e767c917bc7ad2a73ee81d3f7eef67aaf1a1330 xsa224-4.6/0001-gnttab-Fix-handling-of-dev_bus_addr-during-unmap.patch
db32d15757c9d147c7e89eebd10a16324e59141fbb5ce3feb87fc9bf01864a6a xsa224-4.6/0002-gnttab-never-create-host-mapping-unless-asked-to.patch
6bc9bbcf320d673822bd41545a014bd998294d06c5b38d79a6badf1a154ed0d6 xsa224-4.6/0003-gnttab-correct-logic-to-get-page-references-during-m.patch
088064fec3192928f205b34b808ca40fd685a8ba5037bb665ed0a4f87d6d4035 xsa224-4.6/0004-gnttab-__gnttab_unmap_common_complete-is-all-or-noth.patch
cdd93fb950b823cf96fe52685f6394c1b5e0a1e3d7d3c961a5e781da83551a9f xsa224-4.7/0001-gnttab-Fix-handling-of-dev_bus_addr-during-unmap.patch
0583da31891084b2557a9623bc2b11a480e296004a8716b91c79fe28a824a6e0 xsa224-4.7/0002-gnttab-never-create-host-mapping-unless-asked-to.patch
2323bf581a835f152285b98ed2e4b5b503b0f67bd8e3449d33e8fe03b14ce064 xsa224-4.7/0003-gnttab-correct-logic-to-get-page-references-during-m.patch
b4f4adb1ea850e0174e51f76da7e97769211977c71809bd62102d33d90444b09 xsa224-4.7/0004-gnttab-__gnttab_unmap_common_complete-is-all-or-noth.patch
88b20e6765f0bfffe7598215f3a8e25c0931dbe3c7223cb3c08f998842cfc14b xsa224-4.8/0001-gnttab-Fix-handling-of-dev_bus_addr-during-unmap.patch
ce62c97f470d6fbf557f50be8936051e91592a6330527515b7cdb187a0d633b2 xsa224-4.8/0002-gnttab-never-create-host-mapping-unless-asked-to.patch
5fd8cd67737c6a038d6c47fcf3c5bd2d238f4ac361538d650292ee185bda8000 xsa224-4.8/0003-gnttab-correct-logic-to-get-page-references-during-m.patch
f9c65c7f04063872602c609d2fc3caffc44716b3d378569969a7884abe881a19 xsa224-4.8/0004-gnttab-__gnttab_unmap_common_complete-is-all-or-noth.patch
$
DEPLOYMENT DURING EMBARGO
=========================
Deployment of the patches and/or mitigations described above (or
others which are substantially similar) is permitted during the
embargo, even on public-facing systems with untrusted guest users and
administrators.
But: Distribution of updated software is prohibited (except to other
members of the predisclosure list).
Predisclosure list members who wish to deploy significantly different
patches and/or mitigations, please contact the Xen Project Security
Team.
(Note: this during-embargo deployment notice is retained in
post-embargo publicly released Xen Project advisories, even though it
is then no longer applicable. This is to enable the community to have
oversight of the Xen Project Security Team's decisionmaking.)
For more information about permissible uses of embargoed information,
consult the Xen Project community's agreed Security Policy:
http://www.xenproject.org/security-policy.html
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
iQEcBAEBCAAGBQJZX5I3AAoJEIP+FMlX6CvZ2vcH/3JCnqxUvVBcIAQ0UK87wjzZ
GQnrro45f1mfO2JyBzhbm9sKDmS0NgcOVZRZ6hteQ54ykq4bQOTDjoXd/bPSwvlk
yvtKsQa7k2n6NDw5XSPJeo8Yl1H44XqHLEzBEMtbjhveKA2zE2p7HrTjaIjblHB5
Xm37DDGHix0T57MSCAWE3BKTbIxpe6FL+Isdgigo8dxYgf8GP5BjN77MZM3rLBmf
2C+5uuEIqrn2ObQGA19mg5flfHe2vluP0VxZfoA5/1EZRnSiNelQtlD2A5b50dKh
ruAs//sIW+tOYfwQqYlb4kvt/Q4rml6jGdgxOfqoRoFRfENHxZ+vb/azENwiGxU=
=fWMp
-----END PGP SIGNATURE-----
[-- Attachment #2: xsa224-unstable/0001-gnttab-Fix-handling-of-dev_bus_addr-during-unmap.patch --]
[-- Type: application/octet-stream, Size: 3952 bytes --]
From 5a67915261681a1609c05dfe561d20be2669b94a Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Thu, 15 Jun 2017 16:24:02 +0100
Subject: [PATCH 1/4] gnttab: Fix handling of dev_bus_addr during unmap
If a grant has been mapped with the GNTTAB_device_map flag, calling
grant_unmap_ref() with dev_bus_addr set to zero should cause the
GNTTAB_device_map part of the mapping to be left alone.
Unfortunately, at the moment, op->dev_bus_addr is implicitly checked
before clearing the map and adjusting the pin count, but only the bits
above 12; and it is not checked at all before dropping page
references. This means a guest can repeatedly make such a call to
cause the reference count to drop to zero, causing the page to be
freed and re-used, even though it's still mapped in its pagetables.
To fix this, always check op->dev_bus_addr explicitly for being
non-zero, as well as op->flag & GNTMAP_device_map, before doing
operations on the device_map.
While we're here, make the logic a bit cleaner:
* Always initialize op->frame to zero and set it from act->frame, to reduce the
chance of untrusted input being used
* Explicitly check the full dev_bus_addr against act->frame <<
PAGE_SHIFT, rather than ignoring the lower 12 bits
This is part of XSA-224.
Reported-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
xen/common/grant_table.c | 23 +++++++++++------------
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 927fd2b..156eae0 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1089,8 +1089,6 @@ __gnttab_unmap_common(
ld = current->domain;
lgt = ld->grant_table;
- op->frame = (unsigned long)(op->dev_bus_addr >> PAGE_SHIFT);
-
if ( unlikely(op->handle >= lgt->maptrack_limit) )
{
gdprintk(XENLOG_INFO, "Bad handle %#x\n", op->handle);
@@ -1174,16 +1172,14 @@ __gnttab_unmap_common(
goto act_release_out;
}
- if ( op->frame == 0 )
- {
- op->frame = act->frame;
- }
- else
+ op->frame = act->frame;
+
+ if ( op->dev_bus_addr )
{
- if ( unlikely(op->frame != act->frame) )
+ if ( unlikely(op->dev_bus_addr != pfn_to_paddr(act->frame)) )
PIN_FAIL(act_release_out, GNTST_general_error,
- "Bad frame number doesn't match gntref. (%lx != %lx)\n",
- op->frame, act->frame);
+ "Bus address doesn't match gntref (%"PRIx64" != %"PRIpaddr")\n",
+ op->dev_bus_addr, pfn_to_paddr(act->frame));
map->flags &= ~GNTMAP_device_map;
}
@@ -1276,7 +1272,8 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op)
else
status = &status_entry(rgt, op->ref);
- if ( unlikely(op->frame != act->frame) )
+ if ( op->dev_bus_addr &&
+ unlikely(op->dev_bus_addr != pfn_to_paddr(act->frame)) )
{
/*
* Suggests that __gntab_unmap_common failed early and so
@@ -1287,7 +1284,7 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op)
pg = mfn_to_page(op->frame);
- if ( op->flags & GNTMAP_device_map )
+ if ( op->dev_bus_addr && (op->flags & GNTMAP_device_map) )
{
if ( !is_iomem_page(_mfn(act->frame)) )
{
@@ -1358,6 +1355,7 @@ __gnttab_unmap_grant_ref(
/* Intialise these in case common contains old state */
common->new_addr = 0;
common->rd = NULL;
+ common->frame = 0;
__gnttab_unmap_common(common);
op->status = common->status;
@@ -1422,6 +1420,7 @@ __gnttab_unmap_and_replace(
/* Intialise these in case common contains old state */
common->dev_bus_addr = 0;
common->rd = NULL;
+ common->frame = 0;
__gnttab_unmap_common(common);
op->status = common->status;
--
2.1.4
[-- Attachment #3: xsa224-unstable/0002-gnttab-never-create-host-mapping-unless-asked-to.patch --]
[-- Type: application/octet-stream, Size: 1376 bytes --]
From 05a4454d13b98beda2bf76acd5aa4044010d7521 Mon Sep 17 00:00:00 2001
From: Jan Beulich <jbeulich@suse.com>
Date: Fri, 2 Jun 2017 15:21:27 +0100
Subject: [PATCH 2/4] gnttab: never create host mapping unless asked to
We shouldn't create a host mapping unless asked to even in the case of
mapping a granted MMIO page. In particular the mapping wouldn't be torn
down when processing the matching unmap request.
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 | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 156eae0..95b5368 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -911,10 +911,13 @@ __gnttab_map_grant_ref(
goto undo_out;
}
- rc = create_grant_host_mapping(
- op->host_addr, frame, op->flags, cache_flags);
- if ( rc != GNTST_okay )
- goto undo_out;
+ if ( op->flags & GNTMAP_host_map )
+ {
+ rc = create_grant_host_mapping(op->host_addr, frame, op->flags,
+ cache_flags);
+ if ( rc != GNTST_okay )
+ goto undo_out;
+ }
}
else if ( owner == rd || owner == dom_cow )
{
--
2.1.4
[-- Attachment #4: xsa224-unstable/0003-gnttab-correct-logic-to-get-page-references-during-m.patch --]
[-- Type: application/octet-stream, Size: 6124 bytes --]
From 686310db422808656a1e0a98d8f1d0d7c0746201 Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Fri, 2 Jun 2017 15:21:27 +0100
Subject: [PATCH 3/4] gnttab: correct logic to get page references during map
requests
The rules for reference counting are somewhat complicated:
* Each of GNTTAB_host_map and GNTTAB_device_map need their own
reference count
* If the mapping is writeable:
- GNTTAB_host_map needs a type count under only some conditions
- GNTTAB_device_map always needs a type count
If the mapping succeeds, we need to keep all of these; if the mapping
fails, we need to release whatever references we have acquired so far.
Additionally, the code that does a lot of this calculation "inherits"
a reference as part of the process of finding out who the owner is.
Finally, if the grant is mapped as writeable (without the
GNTMAP_readonly flag), but the hypervisor cannot grab a
PGT_writeable_page type, the entire operation should fail.
Unfortunately, the current code has several logic holes:
* If a grant is mapped only GNTTAB_device_map, and with a writeable
mapping, but in conditions where a *host* type count is not
necessary, the code will fail to grab the necessary type count.
* If a grant is mapped both GNTTAB_device_map and GNTTAB_host_map,
with a writeable mapping, in conditions where the host type count is
not necessary, *and* where the page cannot be changed to type
PGT_writeable, the condition will not be detected.
In both cases, this means that on success, the type count will be
erroneously reduced when the grant is unmapped. In the second case,
the type count will be erroneously reduced on the failure path as
well. (In the first case the failure path logic has the same hole
as the reference grabbing logic.)
Additionally, the return value of get_page() is not checked; but this
may fail even if the first get_page() succeeded due to a reference
counting overflow.
First of all, simplify the restoration logic by explicitly counting
the reference and type references acquired.
Consider each mapping type separately, explicitly marking the
'incoming' reference as used so we know when we need to grab a second
one.
Finally, always check the return value of get_page[_type]() and go to
the failure path if appropriate.
This is part of XSA-224.
Reported-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
xen/common/grant_table.c | 58 +++++++++++++++++++++++++++---------------------
1 file changed, 33 insertions(+), 25 deletions(-)
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 95b5368..937f9b8 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -758,12 +758,12 @@ __gnttab_map_grant_ref(
struct grant_table *lgt, *rgt;
struct vcpu *led;
int handle;
- unsigned long frame = 0, nr_gets = 0;
+ unsigned long frame = 0;
struct page_info *pg = NULL;
int rc = GNTST_okay;
u32 old_pin;
u32 act_pin;
- unsigned int cache_flags;
+ unsigned int cache_flags, refcnt = 0, typecnt = 0;
struct active_grant_entry *act = NULL;
struct grant_mapping *mt;
grant_entry_header_t *shah;
@@ -889,11 +889,17 @@ __gnttab_map_grant_ref(
else
owner = page_get_owner(pg);
+ if ( owner )
+ refcnt++;
+
if ( !pg || (owner == dom_io) )
{
/* Only needed the reference to confirm dom_io ownership. */
if ( pg )
+ {
put_page(pg);
+ refcnt--;
+ }
if ( paging_mode_external(ld) )
{
@@ -921,27 +927,38 @@ __gnttab_map_grant_ref(
}
else if ( owner == rd || owner == dom_cow )
{
- if ( gnttab_host_mapping_get_page_type(op, ld, rd) )
+ if ( (op->flags & GNTMAP_device_map) && !(op->flags & GNTMAP_readonly) )
{
if ( (owner == dom_cow) ||
!get_page_type(pg, PGT_writable_page) )
goto could_not_pin;
+ typecnt++;
}
- nr_gets++;
if ( op->flags & GNTMAP_host_map )
{
- rc = create_grant_host_mapping(op->host_addr, frame, op->flags, 0);
- if ( rc != GNTST_okay )
- goto undo_out;
-
+ /*
+ * Only need to grab another reference if device_map claimed
+ * the other one.
+ */
if ( op->flags & GNTMAP_device_map )
{
- nr_gets++;
- (void)get_page(pg, rd);
- if ( !(op->flags & GNTMAP_readonly) )
- get_page_type(pg, PGT_writable_page);
+ if ( !get_page(pg, rd) )
+ goto could_not_pin;
+ refcnt++;
+ }
+
+ if ( gnttab_host_mapping_get_page_type(op, ld, rd) )
+ {
+ if ( (owner == dom_cow) ||
+ !get_page_type(pg, PGT_writable_page) )
+ goto could_not_pin;
+ typecnt++;
}
+
+ rc = create_grant_host_mapping(op->host_addr, frame, op->flags, 0);
+ if ( rc != GNTST_okay )
+ goto undo_out;
}
}
else
@@ -950,8 +967,6 @@ __gnttab_map_grant_ref(
if ( !rd->is_dying )
gdprintk(XENLOG_WARNING, "Could not pin grant frame %lx\n",
frame);
- if ( owner != NULL )
- put_page(pg);
rc = GNTST_general_error;
goto undo_out;
}
@@ -1014,18 +1029,11 @@ __gnttab_map_grant_ref(
return;
undo_out:
- if ( nr_gets > 1 )
- {
- if ( !(op->flags & GNTMAP_readonly) )
- put_page_type(pg);
- put_page(pg);
- }
- if ( nr_gets > 0 )
- {
- if ( gnttab_host_mapping_get_page_type(op, ld, rd) )
- put_page_type(pg);
+ while ( typecnt-- )
+ put_page_type(pg);
+
+ while ( refcnt-- )
put_page(pg);
- }
grant_read_lock(rgt);
--
2.1.4
[-- Attachment #5: xsa224-unstable/0004-gnttab-__gnttab_unmap_common_complete-is-all-or-noth.patch --]
[-- Type: application/octet-stream, Size: 11573 bytes --]
From 748aa3060753f876f221222994119dbf029ced60 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 | 106 ++++++++++++++++++--------------------
xen/include/asm-arm/grant_table.h | 2 +-
xen/include/asm-x86/grant_table.h | 5 +-
3 files changed, 54 insertions(+), 59 deletions(-)
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 937f9b8..a9067c0 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;
@@ -948,7 +948,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) )
@@ -1095,6 +1096,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;
@@ -1145,6 +1147,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;
@@ -1160,6 +1176,7 @@ __gnttab_unmap_common(
{
gdprintk(XENLOG_WARNING, "Unstable handle %#x\n", op->handle);
rc = GNTST_bad_handle;
+ flags = 0;
goto unlock_out;
}
@@ -1173,9 +1190,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);
@@ -1185,24 +1202,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)) )
@@ -1239,7 +1259,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;
@@ -1256,13 +1276,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;
}
@@ -1272,8 +1288,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);
@@ -1283,72 +1297,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(_mfn(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 )
- {
- /*
- * 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(_mfn(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);
@@ -1364,6 +1356,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;
@@ -1429,6 +1422,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;
@@ -3389,7 +3383,9 @@ gnttab_release_mappings(
if ( gnttab_release_host_mappings(d) &&
!is_iomem_page(_mfn(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 e1b3391..32d0a86 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
[-- Attachment #6: xsa224-4.5/0001-gnttab-Fix-handling-of-dev_bus_addr-during-unmap.patch --]
[-- Type: application/octet-stream, Size: 3966 bytes --]
From 0aa6bc3bba0aeec067feed2a7378d285d7529684 Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Thu, 15 Jun 2017 16:24:02 +0100
Subject: [PATCH 1/4] gnttab: Fix handling of dev_bus_addr during unmap
If a grant has been mapped with the GNTTAB_device_map flag, calling
grant_unmap_ref() with dev_bus_addr set to zero should cause the
GNTTAB_device_map part of the mapping to be left alone.
Unfortunately, at the moment, op->dev_bus_addr is implicitly checked
before clearing the map and adjusting the pin count, but only the bits
above 12; and it is not checked at all before dropping page
references. This means a guest can repeatedly make such a call to
cause the reference count to drop to zero, causing the page to be
freed and re-used, even though it's still mapped in its pagetables.
To fix this, always check op->dev_bus_addr explicitly for being
non-zero, as well as op->flag & GNTMAP_device_map, before doing
operations on the device_map.
While we're here, make the logic a bit cleaner:
* Always initialize op->frame to zero and set it from act->frame, to reduce the
chance of untrusted input being used
* Explicitly check the full dev_bus_addr against act->frame <<
PAGE_SHIFT, rather than ignoring the lower 12 bits
This is part of XSA-224.
Reported-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
xen/common/grant_table.c | 23 +++++++++++------------
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index ac98aef..2679073 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -899,8 +899,6 @@ __gnttab_unmap_common(
ld = current->domain;
lgt = ld->grant_table;
- op->frame = (unsigned long)(op->dev_bus_addr >> PAGE_SHIFT);
-
if ( unlikely(op->handle >= lgt->maptrack_limit) )
{
gdprintk(XENLOG_INFO, "Bad handle (%d).\n", op->handle);
@@ -957,16 +955,14 @@ __gnttab_unmap_common(
op->ref = map->ref;
act = &active_entry(rgt, map->ref);
- if ( op->frame == 0 )
- {
- op->frame = act->frame;
- }
- else
+ op->frame = act->frame;
+
+ if ( op->dev_bus_addr )
{
- if ( unlikely(op->frame != act->frame) )
+ if ( unlikely(op->dev_bus_addr != pfn_to_paddr(act->frame)) )
PIN_FAIL(unmap_out, GNTST_general_error,
- "Bad frame number doesn't match gntref. (%lx != %lx)\n",
- op->frame, act->frame);
+ "Bus address doesn't match gntref (%"PRIx64" != %"PRIpaddr")\n",
+ op->dev_bus_addr, pfn_to_paddr(act->frame));
map->flags &= ~GNTMAP_device_map;
}
@@ -1057,7 +1053,8 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op)
else
status = &status_entry(rgt, op->ref);
- if ( unlikely(op->frame != act->frame) )
+ if ( op->dev_bus_addr &&
+ unlikely(op->dev_bus_addr != pfn_to_paddr(act->frame)) )
{
/*
* Suggests that __gntab_unmap_common failed early and so
@@ -1068,7 +1065,7 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op)
pg = mfn_to_page(op->frame);
- if ( op->flags & GNTMAP_device_map )
+ if ( op->dev_bus_addr && (op->flags & GNTMAP_device_map) )
{
if ( !is_iomem_page(act->frame) )
{
@@ -1136,6 +1133,7 @@ __gnttab_unmap_grant_ref(
/* Intialise these in case common contains old state */
common->new_addr = 0;
common->rd = NULL;
+ common->frame = 0;
__gnttab_unmap_common(common);
op->status = common->status;
@@ -1200,6 +1198,7 @@ __gnttab_unmap_and_replace(
/* Intialise these in case common contains old state */
common->dev_bus_addr = 0;
common->rd = NULL;
+ common->frame = 0;
__gnttab_unmap_common(common);
op->status = common->status;
--
2.1.4
[-- Attachment #7: xsa224-4.5/0002-gnttab-never-create-host-mapping-unless-asked-to.patch --]
[-- Type: application/octet-stream, Size: 1376 bytes --]
From 52078b99abbc534a5bda6f7d8ab2b403711a9bcf Mon Sep 17 00:00:00 2001
From: Jan Beulich <jbeulich@suse.com>
Date: Fri, 2 Jun 2017 15:21:27 +0100
Subject: [PATCH 2/4] gnttab: never create host mapping unless asked to
We shouldn't create a host mapping unless asked to even in the case of
mapping a granted MMIO page. In particular the mapping wouldn't be torn
down when processing the matching unmap request.
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 | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 2679073..c40073d 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -736,10 +736,13 @@ __gnttab_map_grant_ref(
goto undo_out;
}
- rc = create_grant_host_mapping(
- op->host_addr, frame, op->flags, cache_flags);
- if ( rc != GNTST_okay )
- goto undo_out;
+ if ( op->flags & GNTMAP_host_map )
+ {
+ rc = create_grant_host_mapping(op->host_addr, frame, op->flags,
+ cache_flags);
+ if ( rc != GNTST_okay )
+ goto undo_out;
+ }
}
else if ( owner == rd || owner == dom_cow )
{
--
2.1.4
[-- Attachment #8: xsa224-4.5/0003-gnttab-correct-logic-to-get-page-references-during-m.patch --]
[-- Type: application/octet-stream, Size: 6119 bytes --]
From 5fd4726857a913e90de95623dc385f8856465839 Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Fri, 2 Jun 2017 15:21:27 +0100
Subject: [PATCH 3/4] gnttab: correct logic to get page references during map
requests
The rules for reference counting are somewhat complicated:
* Each of GNTTAB_host_map and GNTTAB_device_map need their own
reference count
* If the mapping is writeable:
- GNTTAB_host_map needs a type count under only some conditions
- GNTTAB_device_map always needs a type count
If the mapping succeeds, we need to keep all of these; if the mapping
fails, we need to release whatever references we have acquired so far.
Additionally, the code that does a lot of this calculation "inherits"
a reference as part of the process of finding out who the owner is.
Finally, if the grant is mapped as writeable (without the
GNTMAP_readonly flag), but the hypervisor cannot grab a
PGT_writeable_page type, the entire operation should fail.
Unfortunately, the current code has several logic holes:
* If a grant is mapped only GNTTAB_device_map, and with a writeable
mapping, but in conditions where a *host* type count is not
necessary, the code will fail to grab the necessary type count.
* If a grant is mapped both GNTTAB_device_map and GNTTAB_host_map,
with a writeable mapping, in conditions where the host type count is
not necessary, *and* where the page cannot be changed to type
PGT_writeable, the condition will not be detected.
In both cases, this means that on success, the type count will be
erroneously reduced when the grant is unmapped. In the second case,
the type count will be erroneously reduced on the failure path as
well. (In the first case the failure path logic has the same hole
as the reference grabbing logic.)
Additionally, the return value of get_page() is not checked; but this
may fail even if the first get_page() succeeded due to a reference
counting overflow.
First of all, simplify the restoration logic by explicitly counting
the reference and type references acquired.
Consider each mapping type separately, explicitly marking the
'incoming' reference as used so we know when we need to grab a second
one.
Finally, always check the return value of get_page[_type]() and go to
the failure path if appropriate.
This is part of XSA-224.
Reported-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
xen/common/grant_table.c | 58 +++++++++++++++++++++++++++---------------------
1 file changed, 33 insertions(+), 25 deletions(-)
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index c40073d..9f4fc37 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -572,12 +572,12 @@ __gnttab_map_grant_ref(
struct grant_table *lgt, *rgt;
struct vcpu *led;
int handle;
- unsigned long frame = 0, nr_gets = 0;
+ unsigned long frame = 0;
struct page_info *pg = NULL;
int rc = GNTST_okay;
u32 old_pin;
u32 act_pin;
- unsigned int cache_flags;
+ unsigned int cache_flags, refcnt = 0, typecnt = 0;
struct active_grant_entry *act = NULL;
struct grant_mapping *mt;
grant_entry_v1_t *sha1;
@@ -714,11 +714,17 @@ __gnttab_map_grant_ref(
else
owner = page_get_owner(pg);
+ if ( owner )
+ refcnt++;
+
if ( !pg || (owner == dom_io) )
{
/* Only needed the reference to confirm dom_io ownership. */
if ( pg )
+ {
put_page(pg);
+ refcnt--;
+ }
if ( paging_mode_external(ld) )
{
@@ -746,27 +752,38 @@ __gnttab_map_grant_ref(
}
else if ( owner == rd || owner == dom_cow )
{
- if ( gnttab_host_mapping_get_page_type(op, ld, rd) )
+ if ( (op->flags & GNTMAP_device_map) && !(op->flags & GNTMAP_readonly) )
{
if ( (owner == dom_cow) ||
!get_page_type(pg, PGT_writable_page) )
goto could_not_pin;
+ typecnt++;
}
- nr_gets++;
if ( op->flags & GNTMAP_host_map )
{
- rc = create_grant_host_mapping(op->host_addr, frame, op->flags, 0);
- if ( rc != GNTST_okay )
- goto undo_out;
-
+ /*
+ * Only need to grab another reference if device_map claimed
+ * the other one.
+ */
if ( op->flags & GNTMAP_device_map )
{
- nr_gets++;
- (void)get_page(pg, rd);
- if ( !(op->flags & GNTMAP_readonly) )
- get_page_type(pg, PGT_writable_page);
+ if ( !get_page(pg, rd) )
+ goto could_not_pin;
+ refcnt++;
+ }
+
+ if ( gnttab_host_mapping_get_page_type(op, ld, rd) )
+ {
+ if ( (owner == dom_cow) ||
+ !get_page_type(pg, PGT_writable_page) )
+ goto could_not_pin;
+ typecnt++;
}
+
+ rc = create_grant_host_mapping(op->host_addr, frame, op->flags, 0);
+ if ( rc != GNTST_okay )
+ goto undo_out;
}
}
else
@@ -775,8 +792,6 @@ __gnttab_map_grant_ref(
if ( !rd->is_dying )
gdprintk(XENLOG_WARNING, "Could not pin grant frame %lx\n",
frame);
- if ( owner != NULL )
- put_page(pg);
rc = GNTST_general_error;
goto undo_out;
}
@@ -827,18 +842,11 @@ __gnttab_map_grant_ref(
return;
undo_out:
- if ( nr_gets > 1 )
- {
- if ( !(op->flags & GNTMAP_readonly) )
- put_page_type(pg);
- put_page(pg);
- }
- if ( nr_gets > 0 )
- {
- if ( gnttab_host_mapping_get_page_type(op, ld, rd) )
- put_page_type(pg);
+ while ( typecnt-- )
+ put_page_type(pg);
+
+ while ( refcnt-- )
put_page(pg);
- }
spin_lock(&rgt->lock);
--
2.1.4
[-- Attachment #9: xsa224-4.5/0004-gnttab-__gnttab_unmap_common_complete-is-all-or-noth.patch --]
[-- Type: application/octet-stream, Size: 11051 bytes --]
From 414d97feb2e28ed131da33546ea81a919c30e285 Mon Sep 17 00:00:00 2001
From: Jan Beulich <jbeulich@suse.com>
Date: Fri, 2 Jun 2017 15:51:58 +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 | 105 ++++++++++++++++++--------------------
xen/include/asm-arm/grant_table.h | 2 +-
xen/include/asm-x86/grant_table.h | 5 +-
3 files changed, 53 insertions(+), 59 deletions(-)
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 9f4fc37..32c8b41 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;
@@ -773,7 +773,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) )
@@ -905,6 +906,7 @@ __gnttab_unmap_common(
struct active_grant_entry *act;
s16 rc = 0;
struct grant_mapping *map;
+ unsigned int flags;
bool_t put_handle = 0;
ld = current->domain;
@@ -954,8 +956,22 @@ __gnttab_unmap_common(
rgt = rd->grant_table;
double_gt_lock(lgt, rgt);
- op->flags = map->flags;
- if ( unlikely(!op->flags) || unlikely(map->domid != dom) )
+ 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 unmap_out;
+ }
+
+ flags = map->flags;
+ if ( unlikely(!flags) || unlikely(map->domid != dom) )
{
gdprintk(XENLOG_WARNING, "Unstable handle %u\n", op->handle);
rc = GNTST_bad_handle;
@@ -968,24 +984,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(unmap_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(unmap_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 unmap_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)) )
@@ -1020,7 +1039,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;
@@ -1037,13 +1056,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;
}
@@ -1053,9 +1068,6 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op)
rgt = rd->grant_table;
spin_lock(&rgt->lock);
- if ( rgt->gt_version == 0 )
- goto unmap_out;
-
act = &active_entry(rgt, op->ref);
sha = shared_entry_header(rgt, op->ref);
@@ -1064,70 +1076,49 @@ __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 unmap_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 )
- {
- /*
- * 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 unmap_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);
- unmap_out:
spin_unlock(&rgt->lock);
rcu_unlock_domain(rd);
}
@@ -1142,6 +1133,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;
@@ -1207,6 +1199,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;
@@ -2980,7 +2973,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 0edad67..c6c5456 100644
--- a/xen/include/asm-arm/grant_table.h
+++ b/xen/include/asm-arm/grant_table.h
@@ -10,7 +10,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
[-- Attachment #10: xsa224-4.6/0001-gnttab-Fix-handling-of-dev_bus_addr-during-unmap.patch --]
[-- Type: application/octet-stream, Size: 3948 bytes --]
From 62e73c9a3e11c6bffa18e20a97329ef7fb694635 Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Thu, 15 Jun 2017 16:24:02 +0100
Subject: [PATCH 1/4] gnttab: Fix handling of dev_bus_addr during unmap
If a grant has been mapped with the GNTTAB_device_map flag, calling
grant_unmap_ref() with dev_bus_addr set to zero should cause the
GNTTAB_device_map part of the mapping to be left alone.
Unfortunately, at the moment, op->dev_bus_addr is implicitly checked
before clearing the map and adjusting the pin count, but only the bits
above 12; and it is not checked at all before dropping page
references. This means a guest can repeatedly make such a call to
cause the reference count to drop to zero, causing the page to be
freed and re-used, even though it's still mapped in its pagetables.
To fix this, always check op->dev_bus_addr explicitly for being
non-zero, as well as op->flag & GNTMAP_device_map, before doing
operations on the device_map.
While we're here, make the logic a bit cleaner:
* Always initialize op->frame to zero and set it from act->frame, to reduce the
chance of untrusted input being used
* Explicitly check the full dev_bus_addr against act->frame <<
PAGE_SHIFT, rather than ignoring the lower 12 bits
This is part of XSA-224.
Reported-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
xen/common/grant_table.c | 23 +++++++++++------------
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index a642763..c35aea9 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1076,8 +1076,6 @@ __gnttab_unmap_common(
ld = current->domain;
lgt = ld->grant_table;
- op->frame = (unsigned long)(op->dev_bus_addr >> PAGE_SHIFT);
-
if ( unlikely(op->handle >= lgt->maptrack_limit) )
{
gdprintk(XENLOG_INFO, "Bad handle (%d).\n", op->handle);
@@ -1161,16 +1159,14 @@ __gnttab_unmap_common(
goto act_release_out;
}
- if ( op->frame == 0 )
- {
- op->frame = act->frame;
- }
- else
+ op->frame = act->frame;
+
+ if ( op->dev_bus_addr )
{
- if ( unlikely(op->frame != act->frame) )
+ if ( unlikely(op->dev_bus_addr != pfn_to_paddr(act->frame)) )
PIN_FAIL(act_release_out, GNTST_general_error,
- "Bad frame number doesn't match gntref. (%lx != %lx)\n",
- op->frame, act->frame);
+ "Bus address doesn't match gntref (%"PRIx64" != %"PRIpaddr")\n",
+ op->dev_bus_addr, pfn_to_paddr(act->frame));
map->flags &= ~GNTMAP_device_map;
}
@@ -1263,7 +1259,8 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op)
else
status = &status_entry(rgt, op->ref);
- if ( unlikely(op->frame != act->frame) )
+ if ( op->dev_bus_addr &&
+ unlikely(op->dev_bus_addr != pfn_to_paddr(act->frame)) )
{
/*
* Suggests that __gntab_unmap_common failed early and so
@@ -1274,7 +1271,7 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op)
pg = mfn_to_page(op->frame);
- if ( op->flags & GNTMAP_device_map )
+ if ( op->dev_bus_addr && (op->flags & GNTMAP_device_map) )
{
if ( !is_iomem_page(act->frame) )
{
@@ -1345,6 +1342,7 @@ __gnttab_unmap_grant_ref(
/* Intialise these in case common contains old state */
common->new_addr = 0;
common->rd = NULL;
+ common->frame = 0;
__gnttab_unmap_common(common);
op->status = common->status;
@@ -1409,6 +1407,7 @@ __gnttab_unmap_and_replace(
/* Intialise these in case common contains old state */
common->dev_bus_addr = 0;
common->rd = NULL;
+ common->frame = 0;
__gnttab_unmap_common(common);
op->status = common->status;
--
2.1.4
[-- Attachment #11: xsa224-4.6/0002-gnttab-never-create-host-mapping-unless-asked-to.patch --]
[-- Type: application/octet-stream, Size: 1376 bytes --]
From 82080ffca8cc95799b54e778923eaa20619ff961 Mon Sep 17 00:00:00 2001
From: Jan Beulich <jbeulich@suse.com>
Date: Fri, 2 Jun 2017 15:21:27 +0100
Subject: [PATCH 2/4] gnttab: never create host mapping unless asked to
We shouldn't create a host mapping unless asked to even in the case of
mapping a granted MMIO page. In particular the mapping wouldn't be torn
down when processing the matching unmap request.
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 | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index c35aea9..c0b4c05 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -898,10 +898,13 @@ __gnttab_map_grant_ref(
goto undo_out;
}
- rc = create_grant_host_mapping(
- op->host_addr, frame, op->flags, cache_flags);
- if ( rc != GNTST_okay )
- goto undo_out;
+ if ( op->flags & GNTMAP_host_map )
+ {
+ rc = create_grant_host_mapping(op->host_addr, frame, op->flags,
+ cache_flags);
+ if ( rc != GNTST_okay )
+ goto undo_out;
+ }
}
else if ( owner == rd || owner == dom_cow )
{
--
2.1.4
[-- Attachment #12: xsa224-4.6/0003-gnttab-correct-logic-to-get-page-references-during-m.patch --]
[-- Type: application/octet-stream, Size: 6125 bytes --]
From 35b4cf7019e9c7631fbb462e5d907e5a3026a9c5 Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Fri, 2 Jun 2017 15:21:27 +0100
Subject: [PATCH 3/4] gnttab: correct logic to get page references during map
requests
The rules for reference counting are somewhat complicated:
* Each of GNTTAB_host_map and GNTTAB_device_map need their own
reference count
* If the mapping is writeable:
- GNTTAB_host_map needs a type count under only some conditions
- GNTTAB_device_map always needs a type count
If the mapping succeeds, we need to keep all of these; if the mapping
fails, we need to release whatever references we have acquired so far.
Additionally, the code that does a lot of this calculation "inherits"
a reference as part of the process of finding out who the owner is.
Finally, if the grant is mapped as writeable (without the
GNTMAP_readonly flag), but the hypervisor cannot grab a
PGT_writeable_page type, the entire operation should fail.
Unfortunately, the current code has several logic holes:
* If a grant is mapped only GNTTAB_device_map, and with a writeable
mapping, but in conditions where a *host* type count is not
necessary, the code will fail to grab the necessary type count.
* If a grant is mapped both GNTTAB_device_map and GNTTAB_host_map,
with a writeable mapping, in conditions where the host type count is
not necessary, *and* where the page cannot be changed to type
PGT_writeable, the condition will not be detected.
In both cases, this means that on success, the type count will be
erroneously reduced when the grant is unmapped. In the second case,
the type count will be erroneously reduced on the failure path as
well. (In the first case the failure path logic has the same hole
as the reference grabbing logic.)
Additionally, the return value of get_page() is not checked; but this
may fail even if the first get_page() succeeded due to a reference
counting overflow.
First of all, simplify the restoration logic by explicitly counting
the reference and type references acquired.
Consider each mapping type separately, explicitly marking the
'incoming' reference as used so we know when we need to grab a second
one.
Finally, always check the return value of get_page[_type]() and go to
the failure path if appropriate.
This is part of XSA-224.
Reported-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
xen/common/grant_table.c | 58 +++++++++++++++++++++++++++---------------------
1 file changed, 33 insertions(+), 25 deletions(-)
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index c0b4c05..c0f5acd 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -744,12 +744,12 @@ __gnttab_map_grant_ref(
struct grant_table *lgt, *rgt;
struct vcpu *led;
int handle;
- unsigned long frame = 0, nr_gets = 0;
+ unsigned long frame = 0;
struct page_info *pg = NULL;
int rc = GNTST_okay;
u32 old_pin;
u32 act_pin;
- unsigned int cache_flags;
+ unsigned int cache_flags, refcnt = 0, typecnt = 0;
struct active_grant_entry *act = NULL;
struct grant_mapping *mt;
grant_entry_header_t *shah;
@@ -876,11 +876,17 @@ __gnttab_map_grant_ref(
else
owner = page_get_owner(pg);
+ if ( owner )
+ refcnt++;
+
if ( !pg || (owner == dom_io) )
{
/* Only needed the reference to confirm dom_io ownership. */
if ( pg )
+ {
put_page(pg);
+ refcnt--;
+ }
if ( paging_mode_external(ld) )
{
@@ -908,27 +914,38 @@ __gnttab_map_grant_ref(
}
else if ( owner == rd || owner == dom_cow )
{
- if ( gnttab_host_mapping_get_page_type(op, ld, rd) )
+ if ( (op->flags & GNTMAP_device_map) && !(op->flags & GNTMAP_readonly) )
{
if ( (owner == dom_cow) ||
!get_page_type(pg, PGT_writable_page) )
goto could_not_pin;
+ typecnt++;
}
- nr_gets++;
if ( op->flags & GNTMAP_host_map )
{
- rc = create_grant_host_mapping(op->host_addr, frame, op->flags, 0);
- if ( rc != GNTST_okay )
- goto undo_out;
-
+ /*
+ * Only need to grab another reference if device_map claimed
+ * the other one.
+ */
if ( op->flags & GNTMAP_device_map )
{
- nr_gets++;
- (void)get_page(pg, rd);
- if ( !(op->flags & GNTMAP_readonly) )
- get_page_type(pg, PGT_writable_page);
+ if ( !get_page(pg, rd) )
+ goto could_not_pin;
+ refcnt++;
+ }
+
+ if ( gnttab_host_mapping_get_page_type(op, ld, rd) )
+ {
+ if ( (owner == dom_cow) ||
+ !get_page_type(pg, PGT_writable_page) )
+ goto could_not_pin;
+ typecnt++;
}
+
+ rc = create_grant_host_mapping(op->host_addr, frame, op->flags, 0);
+ if ( rc != GNTST_okay )
+ goto undo_out;
}
}
else
@@ -937,8 +954,6 @@ __gnttab_map_grant_ref(
if ( !rd->is_dying )
gdprintk(XENLOG_WARNING, "Could not pin grant frame %lx\n",
frame);
- if ( owner != NULL )
- put_page(pg);
rc = GNTST_general_error;
goto undo_out;
}
@@ -1001,18 +1016,11 @@ __gnttab_map_grant_ref(
return;
undo_out:
- if ( nr_gets > 1 )
- {
- if ( !(op->flags & GNTMAP_readonly) )
- put_page_type(pg);
- put_page(pg);
- }
- if ( nr_gets > 0 )
- {
- if ( gnttab_host_mapping_get_page_type(op, ld, rd) )
- put_page_type(pg);
+ while ( typecnt-- )
+ put_page_type(pg);
+
+ while ( refcnt-- )
put_page(pg);
- }
read_lock(&rgt->lock);
--
2.1.4
[-- Attachment #13: xsa224-4.6/0004-gnttab-__gnttab_unmap_common_complete-is-all-or-noth.patch --]
[-- Type: application/octet-stream, Size: 11599 bytes --]
From 804036f102d6933ce05a4457fd15ecffea43d33b 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 c0f5acd..0e8317c 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;
@@ -935,7 +935,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) )
@@ -1082,6 +1083,7 @@ __gnttab_unmap_common(
struct active_grant_entry *act;
s16 rc = 0;
struct grant_mapping *map;
+ unsigned int flags;
bool_t put_handle = 0;
ld = current->domain;
@@ -1132,6 +1134,20 @@ __gnttab_unmap_common(
read_lock(&rgt->lock);
+ 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;
@@ -1147,6 +1163,7 @@ __gnttab_unmap_common(
{
gdprintk(XENLOG_WARNING, "Unstable handle %#x\n", op->handle);
rc = GNTST_bad_handle;
+ flags = 0;
goto unlock_out;
}
@@ -1160,9 +1177,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);
@@ -1172,24 +1189,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)) )
@@ -1226,7 +1246,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;
@@ -1243,13 +1263,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;
}
@@ -1259,8 +1275,6 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op)
rgt = rd->grant_table;
read_lock(&rgt->lock);
- if ( rgt->gt_version == 0 )
- goto unlock_out;
act = active_entry_acquire(rgt, op->ref);
sha = shared_entry_header(rgt, op->ref);
@@ -1270,72 +1284,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:
read_unlock(&rgt->lock);
rcu_unlock_domain(rd);
@@ -1351,6 +1343,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;
@@ -1416,6 +1409,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;
@@ -3376,7 +3370,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 5e076cc..d76c7c7 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
[-- Attachment #14: xsa224-4.7/0001-gnttab-Fix-handling-of-dev_bus_addr-during-unmap.patch --]
[-- Type: application/octet-stream, Size: 3948 bytes --]
From fd97f5f5ba9375163c8d8771fe551bb4a6423b36 Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Thu, 15 Jun 2017 16:24:02 +0100
Subject: [PATCH 1/4] gnttab: Fix handling of dev_bus_addr during unmap
If a grant has been mapped with the GNTTAB_device_map flag, calling
grant_unmap_ref() with dev_bus_addr set to zero should cause the
GNTTAB_device_map part of the mapping to be left alone.
Unfortunately, at the moment, op->dev_bus_addr is implicitly checked
before clearing the map and adjusting the pin count, but only the bits
above 12; and it is not checked at all before dropping page
references. This means a guest can repeatedly make such a call to
cause the reference count to drop to zero, causing the page to be
freed and re-used, even though it's still mapped in its pagetables.
To fix this, always check op->dev_bus_addr explicitly for being
non-zero, as well as op->flag & GNTMAP_device_map, before doing
operations on the device_map.
While we're here, make the logic a bit cleaner:
* Always initialize op->frame to zero and set it from act->frame, to reduce the
chance of untrusted input being used
* Explicitly check the full dev_bus_addr against act->frame <<
PAGE_SHIFT, rather than ignoring the lower 12 bits
This is part of XSA-224.
Reported-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
xen/common/grant_table.c | 23 +++++++++++------------
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index c4d73af..69cbdb6 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1089,8 +1089,6 @@ __gnttab_unmap_common(
ld = current->domain;
lgt = ld->grant_table;
- op->frame = (unsigned long)(op->dev_bus_addr >> PAGE_SHIFT);
-
if ( unlikely(op->handle >= lgt->maptrack_limit) )
{
gdprintk(XENLOG_INFO, "Bad handle (%d).\n", op->handle);
@@ -1174,16 +1172,14 @@ __gnttab_unmap_common(
goto act_release_out;
}
- if ( op->frame == 0 )
- {
- op->frame = act->frame;
- }
- else
+ op->frame = act->frame;
+
+ if ( op->dev_bus_addr )
{
- if ( unlikely(op->frame != act->frame) )
+ if ( unlikely(op->dev_bus_addr != pfn_to_paddr(act->frame)) )
PIN_FAIL(act_release_out, GNTST_general_error,
- "Bad frame number doesn't match gntref. (%lx != %lx)\n",
- op->frame, act->frame);
+ "Bus address doesn't match gntref (%"PRIx64" != %"PRIpaddr")\n",
+ op->dev_bus_addr, pfn_to_paddr(act->frame));
map->flags &= ~GNTMAP_device_map;
}
@@ -1276,7 +1272,8 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op)
else
status = &status_entry(rgt, op->ref);
- if ( unlikely(op->frame != act->frame) )
+ if ( op->dev_bus_addr &&
+ unlikely(op->dev_bus_addr != pfn_to_paddr(act->frame)) )
{
/*
* Suggests that __gntab_unmap_common failed early and so
@@ -1287,7 +1284,7 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op)
pg = mfn_to_page(op->frame);
- if ( op->flags & GNTMAP_device_map )
+ if ( op->dev_bus_addr && (op->flags & GNTMAP_device_map) )
{
if ( !is_iomem_page(act->frame) )
{
@@ -1358,6 +1355,7 @@ __gnttab_unmap_grant_ref(
/* Intialise these in case common contains old state */
common->new_addr = 0;
common->rd = NULL;
+ common->frame = 0;
__gnttab_unmap_common(common);
op->status = common->status;
@@ -1422,6 +1420,7 @@ __gnttab_unmap_and_replace(
/* Intialise these in case common contains old state */
common->dev_bus_addr = 0;
common->rd = NULL;
+ common->frame = 0;
__gnttab_unmap_common(common);
op->status = common->status;
--
2.1.4
[-- Attachment #15: xsa224-4.7/0002-gnttab-never-create-host-mapping-unless-asked-to.patch --]
[-- Type: application/octet-stream, Size: 1376 bytes --]
From 8894a0c20d920aada305aade0591c1e77167b1db Mon Sep 17 00:00:00 2001
From: Jan Beulich <jbeulich@suse.com>
Date: Fri, 2 Jun 2017 15:21:27 +0100
Subject: [PATCH 2/4] gnttab: never create host mapping unless asked to
We shouldn't create a host mapping unless asked to even in the case of
mapping a granted MMIO page. In particular the mapping wouldn't be torn
down when processing the matching unmap request.
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 | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 69cbdb6..452538e 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -911,10 +911,13 @@ __gnttab_map_grant_ref(
goto undo_out;
}
- rc = create_grant_host_mapping(
- op->host_addr, frame, op->flags, cache_flags);
- if ( rc != GNTST_okay )
- goto undo_out;
+ if ( op->flags & GNTMAP_host_map )
+ {
+ rc = create_grant_host_mapping(op->host_addr, frame, op->flags,
+ cache_flags);
+ if ( rc != GNTST_okay )
+ goto undo_out;
+ }
}
else if ( owner == rd || owner == dom_cow )
{
--
2.1.4
[-- Attachment #16: xsa224-4.7/0003-gnttab-correct-logic-to-get-page-references-during-m.patch --]
[-- Type: application/octet-stream, Size: 6124 bytes --]
From 5d491e3cf32ff03552db9d66e842964fec06dcd4 Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Fri, 2 Jun 2017 15:21:27 +0100
Subject: [PATCH 3/4] gnttab: correct logic to get page references during map
requests
The rules for reference counting are somewhat complicated:
* Each of GNTTAB_host_map and GNTTAB_device_map need their own
reference count
* If the mapping is writeable:
- GNTTAB_host_map needs a type count under only some conditions
- GNTTAB_device_map always needs a type count
If the mapping succeeds, we need to keep all of these; if the mapping
fails, we need to release whatever references we have acquired so far.
Additionally, the code that does a lot of this calculation "inherits"
a reference as part of the process of finding out who the owner is.
Finally, if the grant is mapped as writeable (without the
GNTMAP_readonly flag), but the hypervisor cannot grab a
PGT_writeable_page type, the entire operation should fail.
Unfortunately, the current code has several logic holes:
* If a grant is mapped only GNTTAB_device_map, and with a writeable
mapping, but in conditions where a *host* type count is not
necessary, the code will fail to grab the necessary type count.
* If a grant is mapped both GNTTAB_device_map and GNTTAB_host_map,
with a writeable mapping, in conditions where the host type count is
not necessary, *and* where the page cannot be changed to type
PGT_writeable, the condition will not be detected.
In both cases, this means that on success, the type count will be
erroneously reduced when the grant is unmapped. In the second case,
the type count will be erroneously reduced on the failure path as
well. (In the first case the failure path logic has the same hole
as the reference grabbing logic.)
Additionally, the return value of get_page() is not checked; but this
may fail even if the first get_page() succeeded due to a reference
counting overflow.
First of all, simplify the restoration logic by explicitly counting
the reference and type references acquired.
Consider each mapping type separately, explicitly marking the
'incoming' reference as used so we know when we need to grab a second
one.
Finally, always check the return value of get_page[_type]() and go to
the failure path if appropriate.
This is part of XSA-224.
Reported-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
xen/common/grant_table.c | 58 +++++++++++++++++++++++++++---------------------
1 file changed, 33 insertions(+), 25 deletions(-)
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 452538e..5e92e2c 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -758,12 +758,12 @@ __gnttab_map_grant_ref(
struct grant_table *lgt, *rgt;
struct vcpu *led;
int handle;
- unsigned long frame = 0, nr_gets = 0;
+ unsigned long frame = 0;
struct page_info *pg = NULL;
int rc = GNTST_okay;
u32 old_pin;
u32 act_pin;
- unsigned int cache_flags;
+ unsigned int cache_flags, refcnt = 0, typecnt = 0;
struct active_grant_entry *act = NULL;
struct grant_mapping *mt;
grant_entry_header_t *shah;
@@ -889,11 +889,17 @@ __gnttab_map_grant_ref(
else
owner = page_get_owner(pg);
+ if ( owner )
+ refcnt++;
+
if ( !pg || (owner == dom_io) )
{
/* Only needed the reference to confirm dom_io ownership. */
if ( pg )
+ {
put_page(pg);
+ refcnt--;
+ }
if ( paging_mode_external(ld) )
{
@@ -921,27 +927,38 @@ __gnttab_map_grant_ref(
}
else if ( owner == rd || owner == dom_cow )
{
- if ( gnttab_host_mapping_get_page_type(op, ld, rd) )
+ if ( (op->flags & GNTMAP_device_map) && !(op->flags & GNTMAP_readonly) )
{
if ( (owner == dom_cow) ||
!get_page_type(pg, PGT_writable_page) )
goto could_not_pin;
+ typecnt++;
}
- nr_gets++;
if ( op->flags & GNTMAP_host_map )
{
- rc = create_grant_host_mapping(op->host_addr, frame, op->flags, 0);
- if ( rc != GNTST_okay )
- goto undo_out;
-
+ /*
+ * Only need to grab another reference if device_map claimed
+ * the other one.
+ */
if ( op->flags & GNTMAP_device_map )
{
- nr_gets++;
- (void)get_page(pg, rd);
- if ( !(op->flags & GNTMAP_readonly) )
- get_page_type(pg, PGT_writable_page);
+ if ( !get_page(pg, rd) )
+ goto could_not_pin;
+ refcnt++;
+ }
+
+ if ( gnttab_host_mapping_get_page_type(op, ld, rd) )
+ {
+ if ( (owner == dom_cow) ||
+ !get_page_type(pg, PGT_writable_page) )
+ goto could_not_pin;
+ typecnt++;
}
+
+ rc = create_grant_host_mapping(op->host_addr, frame, op->flags, 0);
+ if ( rc != GNTST_okay )
+ goto undo_out;
}
}
else
@@ -950,8 +967,6 @@ __gnttab_map_grant_ref(
if ( !rd->is_dying )
gdprintk(XENLOG_WARNING, "Could not pin grant frame %lx\n",
frame);
- if ( owner != NULL )
- put_page(pg);
rc = GNTST_general_error;
goto undo_out;
}
@@ -1014,18 +1029,11 @@ __gnttab_map_grant_ref(
return;
undo_out:
- if ( nr_gets > 1 )
- {
- if ( !(op->flags & GNTMAP_readonly) )
- put_page_type(pg);
- put_page(pg);
- }
- if ( nr_gets > 0 )
- {
- if ( gnttab_host_mapping_get_page_type(op, ld, rd) )
- put_page_type(pg);
+ while ( typecnt-- )
+ put_page_type(pg);
+
+ while ( refcnt-- )
put_page(pg);
- }
grant_read_lock(rgt);
--
2.1.4
[-- Attachment #17: xsa224-4.7/0004-gnttab-__gnttab_unmap_common_complete-is-all-or-noth.patch --]
[-- Type: application/octet-stream, Size: 11596 bytes --]
From 3ad26b95cd9bacedad5ba503515cf6e618162be1 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 5e92e2c..025aad0 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;
@@ -948,7 +948,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) )
@@ -1095,6 +1096,7 @@ __gnttab_unmap_common(
struct active_grant_entry *act;
s16 rc = 0;
struct grant_mapping *map;
+ unsigned int flags;
bool_t put_handle = 0;
ld = current->domain;
@@ -1145,6 +1147,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;
@@ -1160,6 +1176,7 @@ __gnttab_unmap_common(
{
gdprintk(XENLOG_WARNING, "Unstable handle %#x\n", op->handle);
rc = GNTST_bad_handle;
+ flags = 0;
goto unlock_out;
}
@@ -1173,9 +1190,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);
@@ -1185,24 +1202,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)) )
@@ -1239,7 +1259,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;
@@ -1256,13 +1276,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;
}
@@ -1272,8 +1288,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);
@@ -1283,72 +1297,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);
@@ -1364,6 +1356,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;
@@ -1429,6 +1422,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;
@@ -3389,7 +3383,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 5e076cc..d76c7c7 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
[-- Attachment #18: xsa224-4.8/0001-gnttab-Fix-handling-of-dev_bus_addr-during-unmap.patch --]
[-- Type: application/octet-stream, Size: 3948 bytes --]
From 9808ed0b1ebc3a5d2aa08a9ff91fcf3ecb42bc9f Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Thu, 15 Jun 2017 16:24:02 +0100
Subject: [PATCH 1/4] gnttab: Fix handling of dev_bus_addr during unmap
If a grant has been mapped with the GNTTAB_device_map flag, calling
grant_unmap_ref() with dev_bus_addr set to zero should cause the
GNTTAB_device_map part of the mapping to be left alone.
Unfortunately, at the moment, op->dev_bus_addr is implicitly checked
before clearing the map and adjusting the pin count, but only the bits
above 12; and it is not checked at all before dropping page
references. This means a guest can repeatedly make such a call to
cause the reference count to drop to zero, causing the page to be
freed and re-used, even though it's still mapped in its pagetables.
To fix this, always check op->dev_bus_addr explicitly for being
non-zero, as well as op->flag & GNTMAP_device_map, before doing
operations on the device_map.
While we're here, make the logic a bit cleaner:
* Always initialize op->frame to zero and set it from act->frame, to reduce the
chance of untrusted input being used
* Explicitly check the full dev_bus_addr against act->frame <<
PAGE_SHIFT, rather than ignoring the lower 12 bits
This is part of XSA-224.
Reported-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
xen/common/grant_table.c | 23 +++++++++++------------
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index ba10e76..2671761 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1085,8 +1085,6 @@ __gnttab_unmap_common(
ld = current->domain;
lgt = ld->grant_table;
- op->frame = (unsigned long)(op->dev_bus_addr >> PAGE_SHIFT);
-
if ( unlikely(op->handle >= lgt->maptrack_limit) )
{
gdprintk(XENLOG_INFO, "Bad handle (%d).\n", op->handle);
@@ -1169,16 +1167,14 @@ __gnttab_unmap_common(
goto act_release_out;
}
- if ( op->frame == 0 )
- {
- op->frame = act->frame;
- }
- else
+ op->frame = act->frame;
+
+ if ( op->dev_bus_addr )
{
- if ( unlikely(op->frame != act->frame) )
+ if ( unlikely(op->dev_bus_addr != pfn_to_paddr(act->frame)) )
PIN_FAIL(act_release_out, GNTST_general_error,
- "Bad frame number doesn't match gntref. (%lx != %lx)\n",
- op->frame, act->frame);
+ "Bus address doesn't match gntref (%"PRIx64" != %"PRIpaddr")\n",
+ op->dev_bus_addr, pfn_to_paddr(act->frame));
map->flags &= ~GNTMAP_device_map;
}
@@ -1271,7 +1267,8 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op)
else
status = &status_entry(rgt, op->ref);
- if ( unlikely(op->frame != act->frame) )
+ if ( op->dev_bus_addr &&
+ unlikely(op->dev_bus_addr != pfn_to_paddr(act->frame)) )
{
/*
* Suggests that __gntab_unmap_common failed early and so
@@ -1282,7 +1279,7 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common *op)
pg = mfn_to_page(op->frame);
- if ( op->flags & GNTMAP_device_map )
+ if ( op->dev_bus_addr && (op->flags & GNTMAP_device_map) )
{
if ( !is_iomem_page(act->frame) )
{
@@ -1353,6 +1350,7 @@ __gnttab_unmap_grant_ref(
/* Intialise these in case common contains old state */
common->new_addr = 0;
common->rd = NULL;
+ common->frame = 0;
__gnttab_unmap_common(common);
op->status = common->status;
@@ -1417,6 +1415,7 @@ __gnttab_unmap_and_replace(
/* Intialise these in case common contains old state */
common->dev_bus_addr = 0;
common->rd = NULL;
+ common->frame = 0;
__gnttab_unmap_common(common);
op->status = common->status;
--
2.1.4
[-- Attachment #19: xsa224-4.8/0002-gnttab-never-create-host-mapping-unless-asked-to.patch --]
[-- Type: application/octet-stream, Size: 1376 bytes --]
From 2d6357522946bd5a105066db8079e5dd46cb3047 Mon Sep 17 00:00:00 2001
From: Jan Beulich <jbeulich@suse.com>
Date: Fri, 2 Jun 2017 15:21:27 +0100
Subject: [PATCH 2/4] gnttab: never create host mapping unless asked to
We shouldn't create a host mapping unless asked to even in the case of
mapping a granted MMIO page. In particular the mapping wouldn't be torn
down when processing the matching unmap request.
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 | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 2671761..5baae24 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -907,10 +907,13 @@ __gnttab_map_grant_ref(
goto undo_out;
}
- rc = create_grant_host_mapping(
- op->host_addr, frame, op->flags, cache_flags);
- if ( rc != GNTST_okay )
- goto undo_out;
+ if ( op->flags & GNTMAP_host_map )
+ {
+ rc = create_grant_host_mapping(op->host_addr, frame, op->flags,
+ cache_flags);
+ if ( rc != GNTST_okay )
+ goto undo_out;
+ }
}
else if ( owner == rd || owner == dom_cow )
{
--
2.1.4
[-- Attachment #20: xsa224-4.8/0003-gnttab-correct-logic-to-get-page-references-during-m.patch --]
[-- Type: application/octet-stream, Size: 6124 bytes --]
From 4e718be6f59526927d5cd31ecd80c5c758dca3f5 Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Fri, 2 Jun 2017 15:21:27 +0100
Subject: [PATCH 3/4] gnttab: correct logic to get page references during map
requests
The rules for reference counting are somewhat complicated:
* Each of GNTTAB_host_map and GNTTAB_device_map need their own
reference count
* If the mapping is writeable:
- GNTTAB_host_map needs a type count under only some conditions
- GNTTAB_device_map always needs a type count
If the mapping succeeds, we need to keep all of these; if the mapping
fails, we need to release whatever references we have acquired so far.
Additionally, the code that does a lot of this calculation "inherits"
a reference as part of the process of finding out who the owner is.
Finally, if the grant is mapped as writeable (without the
GNTMAP_readonly flag), but the hypervisor cannot grab a
PGT_writeable_page type, the entire operation should fail.
Unfortunately, the current code has several logic holes:
* If a grant is mapped only GNTTAB_device_map, and with a writeable
mapping, but in conditions where a *host* type count is not
necessary, the code will fail to grab the necessary type count.
* If a grant is mapped both GNTTAB_device_map and GNTTAB_host_map,
with a writeable mapping, in conditions where the host type count is
not necessary, *and* where the page cannot be changed to type
PGT_writeable, the condition will not be detected.
In both cases, this means that on success, the type count will be
erroneously reduced when the grant is unmapped. In the second case,
the type count will be erroneously reduced on the failure path as
well. (In the first case the failure path logic has the same hole
as the reference grabbing logic.)
Additionally, the return value of get_page() is not checked; but this
may fail even if the first get_page() succeeded due to a reference
counting overflow.
First of all, simplify the restoration logic by explicitly counting
the reference and type references acquired.
Consider each mapping type separately, explicitly marking the
'incoming' reference as used so we know when we need to grab a second
one.
Finally, always check the return value of get_page[_type]() and go to
the failure path if appropriate.
This is part of XSA-224.
Reported-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
xen/common/grant_table.c | 58 +++++++++++++++++++++++++++---------------------
1 file changed, 33 insertions(+), 25 deletions(-)
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 5baae24..d07b931 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -754,12 +754,12 @@ __gnttab_map_grant_ref(
struct grant_table *lgt, *rgt;
struct vcpu *led;
int handle;
- unsigned long frame = 0, nr_gets = 0;
+ unsigned long frame = 0;
struct page_info *pg = NULL;
int rc = GNTST_okay;
u32 old_pin;
u32 act_pin;
- unsigned int cache_flags;
+ unsigned int cache_flags, refcnt = 0, typecnt = 0;
struct active_grant_entry *act = NULL;
struct grant_mapping *mt;
grant_entry_header_t *shah;
@@ -885,11 +885,17 @@ __gnttab_map_grant_ref(
else
owner = page_get_owner(pg);
+ if ( owner )
+ refcnt++;
+
if ( !pg || (owner == dom_io) )
{
/* Only needed the reference to confirm dom_io ownership. */
if ( pg )
+ {
put_page(pg);
+ refcnt--;
+ }
if ( paging_mode_external(ld) )
{
@@ -917,27 +923,38 @@ __gnttab_map_grant_ref(
}
else if ( owner == rd || owner == dom_cow )
{
- if ( gnttab_host_mapping_get_page_type(op, ld, rd) )
+ if ( (op->flags & GNTMAP_device_map) && !(op->flags & GNTMAP_readonly) )
{
if ( (owner == dom_cow) ||
!get_page_type(pg, PGT_writable_page) )
goto could_not_pin;
+ typecnt++;
}
- nr_gets++;
if ( op->flags & GNTMAP_host_map )
{
- rc = create_grant_host_mapping(op->host_addr, frame, op->flags, 0);
- if ( rc != GNTST_okay )
- goto undo_out;
-
+ /*
+ * Only need to grab another reference if device_map claimed
+ * the other one.
+ */
if ( op->flags & GNTMAP_device_map )
{
- nr_gets++;
- (void)get_page(pg, rd);
- if ( !(op->flags & GNTMAP_readonly) )
- get_page_type(pg, PGT_writable_page);
+ if ( !get_page(pg, rd) )
+ goto could_not_pin;
+ refcnt++;
+ }
+
+ if ( gnttab_host_mapping_get_page_type(op, ld, rd) )
+ {
+ if ( (owner == dom_cow) ||
+ !get_page_type(pg, PGT_writable_page) )
+ goto could_not_pin;
+ typecnt++;
}
+
+ rc = create_grant_host_mapping(op->host_addr, frame, op->flags, 0);
+ if ( rc != GNTST_okay )
+ goto undo_out;
}
}
else
@@ -946,8 +963,6 @@ __gnttab_map_grant_ref(
if ( !rd->is_dying )
gdprintk(XENLOG_WARNING, "Could not pin grant frame %lx\n",
frame);
- if ( owner != NULL )
- put_page(pg);
rc = GNTST_general_error;
goto undo_out;
}
@@ -1010,18 +1025,11 @@ __gnttab_map_grant_ref(
return;
undo_out:
- if ( nr_gets > 1 )
- {
- if ( !(op->flags & GNTMAP_readonly) )
- put_page_type(pg);
- put_page(pg);
- }
- if ( nr_gets > 0 )
- {
- if ( gnttab_host_mapping_get_page_type(op, ld, rd) )
- put_page_type(pg);
+ while ( typecnt-- )
+ put_page_type(pg);
+
+ while ( refcnt-- )
put_page(pg);
- }
grant_read_lock(rgt);
--
2.1.4
[-- Attachment #21: xsa224-4.8/0004-gnttab-__gnttab_unmap_common_complete-is-all-or-noth.patch --]
[-- Type: application/octet-stream, Size: 11597 bytes --]
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
[-- Attachment #22: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] only message in thread
only message in thread, other threads:[~2017-07-07 13:54 UTC | newest]
Thread overview: (only message) (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-07-07 13:54 Xen Security Advisory 224 (CVE-2017-10920, CVE-2017-10921, CVE-2017-10922) - grant table operations mishandle reference counts Xen.org security team
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).