* Xen Security Advisory 234 (CVE-2017-14319) - insufficient grant unmapping checks for x86 PV guests
@ 2017-09-12 12:03 Xen.org security team
0 siblings, 0 replies; only message in thread
From: Xen.org security team @ 2017-09-12 12:03 UTC (permalink / raw)
To: xen-announce, xen-devel, xen-users, oss-security; +Cc: Xen.org security team
[-- Attachment #1: Type: text/plain, Size: 3944 bytes --]
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
Xen Security Advisory CVE-2017-14319 / XSA-234
version 3
insufficient grant unmapping checks for x86 PV guests
UPDATES IN VERSION 3
====================
Added metadata file
Public release.
ISSUE DESCRIPTION
=================
When removing or replacing a grant mapping, the x86 PV specific path
needs to make sure page table entries remain in sync with other
accounting done. Although the identity of the page frame was
validated correctly, neither the presence of the mapping nor page
writability were taken into account.
IMPACT
======
A malicious or buggy x86 PV guest could escalate its privileges or
crash the hypervisor.
VULNERABLE SYSTEMS
==================
All Xen versions are affected.
Only x86 PV guests can leverage the vulnerability. x86 HVM guests as
well as ARM guests cannot leverage the vulnerability.
MITIGATION
==========
Running only HVM guests will avoid this vulnerability. However, the
vulnerability is exposed to PV stub qemu serving as the device model
for HVM guests. Our default assumption is that an HVM guest has
compromised its PV stub qemu. By extension, it is likely that the
vulnerability is exposed to HVM guests which are served by a PV stub
qemu.
For PV guests, the vulnerability can be avoided if the guest kernel is
controlled by the host rather than guest administrator, provided that
further steps are taken to prevent the guest administrator from loading
code into the kernel (e.g. by disabling loadable modules etc) or from
using other mechanisms which allow them to run code at kernel privilege.
CREDITS
=======
This issue was discovered by Andrew Cooper of Citrix.
RESOLUTION
==========
Applying the appropriate attached patch resolves this issue.
xsa234.patch xen-unstable
xsa234-4.9.patch Xen 4.9.x
xsa234-4.8.patch Xen 4.8.x, Xen 4.7.x
xsa234-4.6.patch Xen 4.6.x
xsa234-4.5.patch Xen 4.5.x
$ sha256sum xsa234*
efbcc7eac0f010281c5651d191076ac08cc7dd22a1945e88e92ba8a03ae8cc40 xsa234.meta
08ffa79e5c2a77db0b91b3bfcf9fa5c50f174fe842b7418e2e1549d47e0aec4d xsa234.patch
4b74f3c85a98bc6f40c6a448b068bf45e71f7cce887b7cb1481aca0e8746d990 xsa234-4.5.patch
3df4ce173196111c1ff849039ea4927c0b4bd632b08a501fb26f64e31b951fba xsa234-4.6.patch
169e4e0eaa6b27e58ff0f4ce50e8fcc3f81b1e0a10210decf22d1b4cac7501fb xsa234-4.8.patch
213f9d81a4ab785db67b9f579c9e88c9c8586c46b93f466a309060750df2df32 xsa234-4.9.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
iQEcBAEBCAAGBQJZt80HAAoJEIP+FMlX6CvZBCsH/1ghPnUr7fpKSgd7huB5gtGC
+QsoqJlmI8U+eWqmS8RlAZ0f5A2Umy7GyYDWqFbvJR2o60AMf7DI9d1QVHQYRSfD
JFw+M4ohZ/gZoHykof929QYY15Fhrnt5PoMJ6ztt3ZsBXYkXTJfyvHwVjCD43Nvt
fANPcYOpm8NneV9mAviVEjR3u08ultjcfq0Gdks22L5zWKzG38j/rbBtA75mx5eT
v/eYXEqrSgXEfI2zJOP/j53D2CwMJnmbbsxgQTvAalSLq1zqNrXFSHEkfyqi+Aix
QReMmubpNVbIv1ybtZsE1tRMgBY7VJBJEbT5/PrOUErb9XMoL0wtMwP+kHuVD2w=
=qFgP
-----END PGP SIGNATURE-----
[-- Attachment #2: xsa234.meta --]
[-- Type: application/octet-stream, Size: 1951 bytes --]
{
"XSA": 234,
"SupportedVersions": [
"master",
"4.9",
"4.8",
"4.7",
"4.6",
"4.5"
],
"Trees": [
"xen"
],
"Recipes": {
"4.5": {
"XenVersion": "4.5",
"Recipes": {
"xen": {
"StableRef": "3217129eb65c0d4995ed08fb8919e3c334cad548",
"Prereqs": [
231,
232,
233
],
"Patches": [ "xsa234-4.5.patch" ]
}
}
},
"4.6": {
"XenVersion": "4.6",
"Recipes": {
"xen": {
"StableRef": "b4660b4d4a35edac715c003c84326de2b0fa4f47",
"Prereqs": [
231,
232,
233
],
"Patches": [ "xsa234-4.6.patch" ]
}
}
},
"4.7": {
"XenVersion": "4.7",
"Recipes": {
"xen": {
"StableRef": "5151257626155d6e331cc9e66d896c84db1611e1",
"Prereqs": [
231,
232,
233
],
"Patches": [ "xsa234-4.8.patch" ]
}
}
},
"4.8": {
"XenVersion": "4.8",
"Recipes": {
"xen": {
"StableRef": "f5211ce75821e0f2cc55effd28dfbe908226970f",
"Prereqs": [
231,
232,
233
],
"Patches": [ "xsa234-4.8.patch" ]
}
}
},
"4.9": {
"XenVersion": "4.9",
"Recipes": {
"xen": {
"StableRef": "9bf14bbf990843bfec16a5d69d36cf46c7593d88",
"Prereqs": [
231,
232,
233
],
"Patches": [ "xsa234-4.9.patch" ]
}
}
},
"master": {
"XenVersion": "master",
"Recipes": {
"xen": {
"StableRef": "9053a74c08fd6abf43bb45ff932b4386de7e8510",
"Prereqs": [
231,
232,
233
],
"Patches": [ "xsa234.patch" ]
}
}
}
}
}
[-- Attachment #3: xsa234.patch --]
[-- Type: application/octet-stream, Size: 7395 bytes --]
From: Jan Beulich <jbeulich@suse.com>
Subject: gnttab: also validate PTE permissions upon destroy/replace
In order for PTE handling to match up with the reference counting done
by common code, presence and writability of grant mapping PTEs must
also be taken into account; validating just the frame number is not
enough. This is in particular relevant if a guest fiddles with grant
PTEs via non-grant hypercalls.
Note that the flags being passed to replace_grant_host_mapping()
already happen to be those of the existing mapping, so no new function
parameter is needed.
This is XSA-234.
Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
v5: Invert the mask used for the "PTE flags don't match" checks.
v4: Slightly relax the "PTE flags don't match" checks to cope with
replace handling also honoring _PAGE_AVAIL and PAGE_CACHE_ATTRS
eventually.
v3: Revert to v1.
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -3855,7 +3855,8 @@ static int create_grant_pte_mapping(
}
static int destroy_grant_pte_mapping(
- uint64_t addr, unsigned long frame, struct domain *d)
+ uint64_t addr, unsigned long frame, unsigned int grant_pte_flags,
+ struct domain *d)
{
int rc = GNTST_okay;
void *va;
@@ -3901,17 +3902,29 @@ static int destroy_grant_pte_mapping(
ol1e = *(l1_pgentry_t *)va;
- /* Check that the virtual address supplied is actually mapped to frame. */
- if ( unlikely(l1e_get_pfn(ol1e) != frame) )
+ /*
+ * Check that the PTE supplied actually maps frame (with appropriate
+ * permissions).
+ */
+ if ( unlikely(l1e_get_pfn(ol1e) != frame) ||
+ unlikely((l1e_get_flags(ol1e) ^ grant_pte_flags) &
+ (_PAGE_PRESENT | _PAGE_RW)) )
{
page_unlock(page);
- gdprintk(XENLOG_WARNING,
- "PTE entry %"PRIpte" for address %"PRIx64" doesn't match frame %lx\n",
- l1e_get_intpte(ol1e), addr, frame);
+ gdprintk(XENLOG_ERR,
+ "PTE %"PRIpte" at %"PRIx64" doesn't match grant (%"PRIpte")\n",
+ l1e_get_intpte(ol1e), addr,
+ l1e_get_intpte(l1e_from_pfn(frame, grant_pte_flags)));
rc = GNTST_general_error;
goto failed;
}
+ if ( unlikely((l1e_get_flags(ol1e) ^ grant_pte_flags) &
+ ~(_PAGE_AVAIL | PAGE_CACHE_ATTRS)) )
+ gdprintk(XENLOG_WARNING,
+ "PTE flags %x at %"PRIx64" don't match grant (%x)\n",
+ l1e_get_flags(ol1e), addr, grant_pte_flags);
+
/* Delete pagetable entry. */
if ( unlikely(!UPDATE_ENTRY(l1,
(l1_pgentry_t *)va, ol1e, l1e_empty(), mfn,
@@ -3919,7 +3932,8 @@ static int destroy_grant_pte_mapping(
0)) )
{
page_unlock(page);
- gdprintk(XENLOG_WARNING, "Cannot delete PTE entry at %p\n", va);
+ gdprintk(XENLOG_WARNING, "Cannot delete PTE entry at %"PRIx64"\n",
+ addr);
rc = GNTST_general_error;
goto failed;
}
@@ -3987,7 +4001,8 @@ static int create_grant_va_mapping(
}
static int replace_grant_va_mapping(
- unsigned long addr, unsigned long frame, l1_pgentry_t nl1e, struct vcpu *v)
+ unsigned long addr, unsigned long frame, unsigned int grant_pte_flags,
+ l1_pgentry_t nl1e, struct vcpu *v)
{
l1_pgentry_t *pl1e, ol1e;
unsigned long gl1mfn;
@@ -4023,20 +4038,33 @@ static int replace_grant_va_mapping(
ol1e = *pl1e;
- /* Check that the virtual address supplied is actually mapped to frame. */
- if ( unlikely(l1e_get_pfn(ol1e) != frame) )
- {
- gdprintk(XENLOG_WARNING,
- "PTE entry %lx for address %lx doesn't match frame %lx\n",
- l1e_get_pfn(ol1e), addr, frame);
+ /*
+ * Check that the virtual address supplied is actually mapped to frame
+ * (with appropriate permissions).
+ */
+ if ( unlikely(l1e_get_pfn(ol1e) != frame) ||
+ unlikely((l1e_get_flags(ol1e) ^ grant_pte_flags) &
+ (_PAGE_PRESENT | _PAGE_RW)) )
+ {
+ gdprintk(XENLOG_ERR,
+ "PTE %"PRIpte" for %lx doesn't match grant (%"PRIpte")\n",
+ l1e_get_intpte(ol1e), addr,
+ l1e_get_intpte(l1e_from_pfn(frame, grant_pte_flags)));
rc = GNTST_general_error;
goto unlock_and_out;
}
+ if ( unlikely((l1e_get_flags(ol1e) ^ grant_pte_flags) &
+ ~(_PAGE_AVAIL | PAGE_CACHE_ATTRS)) )
+ gdprintk(XENLOG_WARNING,
+ "PTE flags %x for %"PRIx64" don't match grant (%x)\n",
+ l1e_get_flags(ol1e), addr, grant_pte_flags);
+
/* Delete pagetable entry. */
if ( unlikely(!UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, gl1mfn, v, 0)) )
{
- gdprintk(XENLOG_WARNING, "Cannot delete PTE entry at %p\n", pl1e);
+ gdprintk(XENLOG_WARNING, "Cannot delete PTE entry for %"PRIx64"\n",
+ addr);
rc = GNTST_general_error;
goto unlock_and_out;
}
@@ -4050,9 +4078,11 @@ static int replace_grant_va_mapping(
}
static int destroy_grant_va_mapping(
- unsigned long addr, unsigned long frame, struct vcpu *v)
+ unsigned long addr, unsigned long frame, unsigned int grant_pte_flags,
+ struct vcpu *v)
{
- return replace_grant_va_mapping(addr, frame, l1e_empty(), v);
+ return replace_grant_va_mapping(addr, frame, grant_pte_flags,
+ l1e_empty(), v);
}
int create_grant_pv_mapping(uint64_t addr, unsigned long frame,
@@ -4091,17 +4121,36 @@ int replace_grant_pv_mapping(
unsigned long gl1mfn;
struct page_info *l1pg;
int rc;
+ unsigned int grant_pte_flags;
+ grant_pte_flags =
+ _PAGE_PRESENT | _PAGE_ACCESSED | _PAGE_DIRTY | _PAGE_GNTTAB | _PAGE_NX;
+
+ if ( flags & GNTMAP_application_map )
+ grant_pte_flags |= _PAGE_USER;
+ if ( !(flags & GNTMAP_readonly) )
+ grant_pte_flags |= _PAGE_RW;
+ /*
+ * On top of the explicit settings done by create_grant_host_mapping()
+ * also open-code relevant parts of adjust_guest_l1e(). Don't mirror
+ * available and cachability flags, though.
+ */
+ if ( !is_pv_32bit_domain(curr->domain) )
+ grant_pte_flags |= (grant_pte_flags & _PAGE_USER)
+ ? _PAGE_GLOBAL
+ : _PAGE_GUEST_KERNEL | _PAGE_USER;
+
if ( flags & GNTMAP_contains_pte )
{
if ( !new_addr )
- return destroy_grant_pte_mapping(addr, frame, curr->domain);
+ return destroy_grant_pte_mapping(addr, frame, grant_pte_flags,
+ curr->domain);
return GNTST_general_error;
}
if ( !new_addr )
- return destroy_grant_va_mapping(addr, frame, curr);
+ return destroy_grant_va_mapping(addr, frame, grant_pte_flags, curr);
pl1e = guest_map_l1e(new_addr, &gl1mfn);
if ( !pl1e )
@@ -4149,7 +4198,7 @@ int replace_grant_host_mapping(
put_page(l1pg);
guest_unmap_l1e(pl1e);
- rc = replace_grant_va_mapping(addr, frame, ol1e, curr);
+ rc = replace_grant_va_mapping(addr, frame, grant_pte_flags, ol1e, curr);
if ( rc )
put_page_from_l1e(ol1e, curr->domain);
[-- Attachment #4: xsa234-4.5.patch --]
[-- Type: application/octet-stream, Size: 7090 bytes --]
From: Jan Beulich <jbeulich@suse.com>
Subject: gnttab: also validate PTE permissions upon destroy/replace
In order for PTE handling to match up with the reference counting done
by common code, presence and writability of grant mapping PTEs must
also be taken into account; validating just the frame number is not
enough. This is in particular relevant if a guest fiddles with grant
PTEs via non-grant hypercalls.
Note that the flags being passed to replace_grant_host_mapping()
already happen to be those of the existing mapping, so no new function
parameter is needed.
This is XSA-234.
Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -3833,7 +3833,8 @@ static int create_grant_pte_mapping(
}
static int destroy_grant_pte_mapping(
- uint64_t addr, unsigned long frame, struct domain *d)
+ uint64_t addr, unsigned long frame, unsigned int grant_pte_flags,
+ struct domain *d)
{
int rc = GNTST_okay;
void *va;
@@ -3879,16 +3880,27 @@ static int destroy_grant_pte_mapping(
ol1e = *(l1_pgentry_t *)va;
- /* Check that the virtual address supplied is actually mapped to frame. */
- if ( unlikely(l1e_get_pfn(ol1e) != frame) )
+ /*
+ * Check that the PTE supplied actually maps frame (with appropriate
+ * permissions).
+ */
+ if ( unlikely(l1e_get_pfn(ol1e) != frame) ||
+ unlikely((l1e_get_flags(ol1e) ^ grant_pte_flags) &
+ (_PAGE_PRESENT | _PAGE_RW)) )
{
page_unlock(page);
- MEM_LOG("PTE entry %lx for address %"PRIx64" doesn't match frame %lx",
- (unsigned long)l1e_get_intpte(ol1e), addr, frame);
+ MEM_LOG("PTE %"PRIpte" at %"PRIx64" doesn't match grant (%"PRIpte")",
+ l1e_get_intpte(ol1e), addr,
+ l1e_get_intpte(l1e_from_pfn(frame, grant_pte_flags)));
rc = GNTST_general_error;
goto failed;
}
+ if ( unlikely((l1e_get_flags(ol1e) ^ grant_pte_flags) &
+ ~(_PAGE_AVAIL | PAGE_CACHE_ATTRS)) )
+ MEM_LOG("PTE flags %x at %"PRIx64" don't match grant (%x)\n",
+ l1e_get_flags(ol1e), addr, grant_pte_flags);
+
/* Delete pagetable entry. */
if ( unlikely(!UPDATE_ENTRY
(l1,
@@ -3897,7 +3909,7 @@ static int destroy_grant_pte_mapping(
0)) )
{
page_unlock(page);
- MEM_LOG("Cannot delete PTE entry at %p", va);
+ MEM_LOG("Cannot delete PTE entry at %"PRIx64, addr);
rc = GNTST_general_error;
goto failed;
}
@@ -3965,7 +3977,8 @@ static int create_grant_va_mapping(
}
static int replace_grant_va_mapping(
- unsigned long addr, unsigned long frame, l1_pgentry_t nl1e, struct vcpu *v)
+ unsigned long addr, unsigned long frame, unsigned int grant_pte_flags,
+ l1_pgentry_t nl1e, struct vcpu *v)
{
l1_pgentry_t *pl1e, ol1e;
unsigned long gl1mfn;
@@ -4001,19 +4014,30 @@ static int replace_grant_va_mapping(
ol1e = *pl1e;
- /* Check that the virtual address supplied is actually mapped to frame. */
- if ( unlikely(l1e_get_pfn(ol1e) != frame) )
- {
- MEM_LOG("PTE entry %lx for address %lx doesn't match frame %lx",
- l1e_get_pfn(ol1e), addr, frame);
+ /*
+ * Check that the virtual address supplied is actually mapped to frame
+ * (with appropriate permissions).
+ */
+ if ( unlikely(l1e_get_pfn(ol1e) != frame) ||
+ unlikely((l1e_get_flags(ol1e) ^ grant_pte_flags) &
+ (_PAGE_PRESENT | _PAGE_RW)) )
+ {
+ MEM_LOG("PTE %"PRIpte" for %lx doesn't match grant (%"PRIpte")",
+ l1e_get_intpte(ol1e), addr,
+ l1e_get_intpte(l1e_from_pfn(frame, grant_pte_flags)));
rc = GNTST_general_error;
goto unlock_and_out;
}
+ if ( unlikely((l1e_get_flags(ol1e) ^ grant_pte_flags) &
+ ~(_PAGE_AVAIL | PAGE_CACHE_ATTRS)) )
+ MEM_LOG("PTE flags %x for %"PRIx64" don't match grant (%x)",
+ l1e_get_flags(ol1e), addr, grant_pte_flags);
+
/* Delete pagetable entry. */
if ( unlikely(!UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, gl1mfn, v, 0)) )
{
- MEM_LOG("Cannot delete PTE entry at %p", (unsigned long *)pl1e);
+ MEM_LOG("Cannot delete PTE entry for %"PRIx64, addr);
rc = GNTST_general_error;
goto unlock_and_out;
}
@@ -4027,9 +4051,11 @@ static int replace_grant_va_mapping(
}
static int destroy_grant_va_mapping(
- unsigned long addr, unsigned long frame, struct vcpu *v)
+ unsigned long addr, unsigned long frame, unsigned int grant_pte_flags,
+ struct vcpu *v)
{
- return replace_grant_va_mapping(addr, frame, l1e_empty(), v);
+ return replace_grant_va_mapping(addr, frame, grant_pte_flags,
+ l1e_empty(), v);
}
static int create_grant_p2m_mapping(uint64_t addr, unsigned long frame,
@@ -4123,21 +4149,42 @@ int replace_grant_host_mapping(
unsigned long gl1mfn;
struct page_info *l1pg;
int rc;
+ unsigned int grant_pte_flags;
if ( paging_mode_external(current->domain) )
return replace_grant_p2m_mapping(addr, frame, new_addr, flags);
+ grant_pte_flags =
+ _PAGE_PRESENT | _PAGE_ACCESSED | _PAGE_DIRTY | _PAGE_GNTTAB;
+ if ( cpu_has_nx )
+ grant_pte_flags |= _PAGE_NX_BIT;
+
+ if ( flags & GNTMAP_application_map )
+ grant_pte_flags |= _PAGE_USER;
+ if ( !(flags & GNTMAP_readonly) )
+ grant_pte_flags |= _PAGE_RW;
+ /*
+ * On top of the explicit settings done by create_grant_host_mapping()
+ * also open-code relevant parts of adjust_guest_l1e(). Don't mirror
+ * available and cachability flags, though.
+ */
+ if ( !is_pv_32bit_domain(curr->domain) )
+ grant_pte_flags |= (grant_pte_flags & _PAGE_USER)
+ ? _PAGE_GLOBAL
+ : _PAGE_GUEST_KERNEL | _PAGE_USER;
+
if ( flags & GNTMAP_contains_pte )
{
if ( !new_addr )
- return destroy_grant_pte_mapping(addr, frame, curr->domain);
+ return destroy_grant_pte_mapping(addr, frame, grant_pte_flags,
+ curr->domain);
MEM_LOG("Unsupported grant table operation");
return GNTST_general_error;
}
if ( !new_addr )
- return destroy_grant_va_mapping(addr, frame, curr);
+ return destroy_grant_va_mapping(addr, frame, grant_pte_flags, curr);
pl1e = guest_map_l1e(curr, new_addr, &gl1mfn);
if ( !pl1e )
@@ -4185,7 +4232,7 @@ int replace_grant_host_mapping(
put_page(l1pg);
guest_unmap_l1e(curr, pl1e);
- rc = replace_grant_va_mapping(addr, frame, ol1e, curr);
+ rc = replace_grant_va_mapping(addr, frame, grant_pte_flags, ol1e, curr);
if ( rc && !paging_mode_refcounts(curr->domain) )
put_page_from_l1e(ol1e, curr->domain);
[-- Attachment #5: xsa234-4.6.patch --]
[-- Type: application/octet-stream, Size: 7036 bytes --]
From: Jan Beulich <jbeulich@suse.com>
Subject: gnttab: also validate PTE permissions upon destroy/replace
In order for PTE handling to match up with the reference counting done
by common code, presence and writability of grant mapping PTEs must
also be taken into account; validating just the frame number is not
enough. This is in particular relevant if a guest fiddles with grant
PTEs via non-grant hypercalls.
Note that the flags being passed to replace_grant_host_mapping()
already happen to be those of the existing mapping, so no new function
parameter is needed.
This is XSA-234.
Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -3930,7 +3930,8 @@ static int create_grant_pte_mapping(
}
static int destroy_grant_pte_mapping(
- uint64_t addr, unsigned long frame, struct domain *d)
+ uint64_t addr, unsigned long frame, unsigned int grant_pte_flags,
+ struct domain *d)
{
int rc = GNTST_okay;
void *va;
@@ -3976,16 +3977,27 @@ static int destroy_grant_pte_mapping(
ol1e = *(l1_pgentry_t *)va;
- /* Check that the virtual address supplied is actually mapped to frame. */
- if ( unlikely(l1e_get_pfn(ol1e) != frame) )
+ /*
+ * Check that the PTE supplied actually maps frame (with appropriate
+ * permissions).
+ */
+ if ( unlikely(l1e_get_pfn(ol1e) != frame) ||
+ unlikely((l1e_get_flags(ol1e) ^ grant_pte_flags) &
+ (_PAGE_PRESENT | _PAGE_RW)) )
{
page_unlock(page);
- MEM_LOG("PTE entry %lx for address %"PRIx64" doesn't match frame %lx",
- (unsigned long)l1e_get_intpte(ol1e), addr, frame);
+ MEM_LOG("PTE %"PRIpte" at %"PRIx64" doesn't match grant (%"PRIpte")",
+ l1e_get_intpte(ol1e), addr,
+ l1e_get_intpte(l1e_from_pfn(frame, grant_pte_flags)));
rc = GNTST_general_error;
goto failed;
}
+ if ( unlikely((l1e_get_flags(ol1e) ^ grant_pte_flags) &
+ ~(_PAGE_AVAIL | PAGE_CACHE_ATTRS)) )
+ MEM_LOG("PTE flags %x at %"PRIx64" don't match grant (%x)\n",
+ l1e_get_flags(ol1e), addr, grant_pte_flags);
+
/* Delete pagetable entry. */
if ( unlikely(!UPDATE_ENTRY
(l1,
@@ -3994,7 +4006,7 @@ static int destroy_grant_pte_mapping(
0)) )
{
page_unlock(page);
- MEM_LOG("Cannot delete PTE entry at %p", va);
+ MEM_LOG("Cannot delete PTE entry at %"PRIx64, addr);
rc = GNTST_general_error;
goto failed;
}
@@ -4062,7 +4074,8 @@ static int create_grant_va_mapping(
}
static int replace_grant_va_mapping(
- unsigned long addr, unsigned long frame, l1_pgentry_t nl1e, struct vcpu *v)
+ unsigned long addr, unsigned long frame, unsigned int grant_pte_flags,
+ l1_pgentry_t nl1e, struct vcpu *v)
{
l1_pgentry_t *pl1e, ol1e;
unsigned long gl1mfn;
@@ -4098,19 +4111,30 @@ static int replace_grant_va_mapping(
ol1e = *pl1e;
- /* Check that the virtual address supplied is actually mapped to frame. */
- if ( unlikely(l1e_get_pfn(ol1e) != frame) )
- {
- MEM_LOG("PTE entry %lx for address %lx doesn't match frame %lx",
- l1e_get_pfn(ol1e), addr, frame);
+ /*
+ * Check that the virtual address supplied is actually mapped to frame
+ * (with appropriate permissions).
+ */
+ if ( unlikely(l1e_get_pfn(ol1e) != frame) ||
+ unlikely((l1e_get_flags(ol1e) ^ grant_pte_flags) &
+ (_PAGE_PRESENT | _PAGE_RW)) )
+ {
+ MEM_LOG("PTE %"PRIpte" for %lx doesn't match grant (%"PRIpte")",
+ l1e_get_intpte(ol1e), addr,
+ l1e_get_intpte(l1e_from_pfn(frame, grant_pte_flags)));
rc = GNTST_general_error;
goto unlock_and_out;
}
+ if ( unlikely((l1e_get_flags(ol1e) ^ grant_pte_flags) &
+ ~(_PAGE_AVAIL | PAGE_CACHE_ATTRS)) )
+ MEM_LOG("PTE flags %x for %"PRIx64" don't match grant (%x)",
+ l1e_get_flags(ol1e), addr, grant_pte_flags);
+
/* Delete pagetable entry. */
if ( unlikely(!UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, gl1mfn, v, 0)) )
{
- MEM_LOG("Cannot delete PTE entry at %p", (unsigned long *)pl1e);
+ MEM_LOG("Cannot delete PTE entry for %"PRIx64, addr);
rc = GNTST_general_error;
goto unlock_and_out;
}
@@ -4124,9 +4148,11 @@ static int replace_grant_va_mapping(
}
static int destroy_grant_va_mapping(
- unsigned long addr, unsigned long frame, struct vcpu *v)
+ unsigned long addr, unsigned long frame, unsigned int grant_pte_flags,
+ struct vcpu *v)
{
- return replace_grant_va_mapping(addr, frame, l1e_empty(), v);
+ return replace_grant_va_mapping(addr, frame, grant_pte_flags,
+ l1e_empty(), v);
}
static int create_grant_p2m_mapping(uint64_t addr, unsigned long frame,
@@ -4219,21 +4245,40 @@ int replace_grant_host_mapping(
unsigned long gl1mfn;
struct page_info *l1pg;
int rc;
+ unsigned int grant_pte_flags;
if ( paging_mode_external(current->domain) )
return replace_grant_p2m_mapping(addr, frame, new_addr, flags);
+ grant_pte_flags =
+ _PAGE_PRESENT | _PAGE_ACCESSED | _PAGE_DIRTY | _PAGE_GNTTAB | _PAGE_NX;
+
+ if ( flags & GNTMAP_application_map )
+ grant_pte_flags |= _PAGE_USER;
+ if ( !(flags & GNTMAP_readonly) )
+ grant_pte_flags |= _PAGE_RW;
+ /*
+ * On top of the explicit settings done by create_grant_host_mapping()
+ * also open-code relevant parts of adjust_guest_l1e(). Don't mirror
+ * available and cachability flags, though.
+ */
+ if ( !is_pv_32bit_domain(curr->domain) )
+ grant_pte_flags |= (grant_pte_flags & _PAGE_USER)
+ ? _PAGE_GLOBAL
+ : _PAGE_GUEST_KERNEL | _PAGE_USER;
+
if ( flags & GNTMAP_contains_pte )
{
if ( !new_addr )
- return destroy_grant_pte_mapping(addr, frame, curr->domain);
+ return destroy_grant_pte_mapping(addr, frame, grant_pte_flags,
+ curr->domain);
MEM_LOG("Unsupported grant table operation");
return GNTST_general_error;
}
if ( !new_addr )
- return destroy_grant_va_mapping(addr, frame, curr);
+ return destroy_grant_va_mapping(addr, frame, grant_pte_flags, curr);
pl1e = guest_map_l1e(curr, new_addr, &gl1mfn);
if ( !pl1e )
@@ -4281,7 +4326,7 @@ int replace_grant_host_mapping(
put_page(l1pg);
guest_unmap_l1e(curr, pl1e);
- rc = replace_grant_va_mapping(addr, frame, ol1e, curr);
+ rc = replace_grant_va_mapping(addr, frame, grant_pte_flags, ol1e, curr);
if ( rc && !paging_mode_refcounts(curr->domain) )
put_page_from_l1e(ol1e, curr->domain);
[-- Attachment #6: xsa234-4.8.patch --]
[-- Type: application/octet-stream, Size: 7024 bytes --]
From: Jan Beulich <jbeulich@suse.com>
Subject: gnttab: also validate PTE permissions upon destroy/replace
In order for PTE handling to match up with the reference counting done
by common code, presence and writability of grant mapping PTEs must
also be taken into account; validating just the frame number is not
enough. This is in particular relevant if a guest fiddles with grant
PTEs via non-grant hypercalls.
Note that the flags being passed to replace_grant_host_mapping()
already happen to be those of the existing mapping, so no new function
parameter is needed.
This is XSA-234.
Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4017,7 +4017,8 @@ static int create_grant_pte_mapping(
}
static int destroy_grant_pte_mapping(
- uint64_t addr, unsigned long frame, struct domain *d)
+ uint64_t addr, unsigned long frame, unsigned int grant_pte_flags,
+ struct domain *d)
{
int rc = GNTST_okay;
void *va;
@@ -4063,16 +4064,27 @@ static int destroy_grant_pte_mapping(
ol1e = *(l1_pgentry_t *)va;
- /* Check that the virtual address supplied is actually mapped to frame. */
- if ( unlikely(l1e_get_pfn(ol1e) != frame) )
+ /*
+ * Check that the PTE supplied actually maps frame (with appropriate
+ * permissions).
+ */
+ if ( unlikely(l1e_get_pfn(ol1e) != frame) ||
+ unlikely((l1e_get_flags(ol1e) ^ grant_pte_flags) &
+ (_PAGE_PRESENT | _PAGE_RW)) )
{
page_unlock(page);
- MEM_LOG("PTE entry %lx for address %"PRIx64" doesn't match frame %lx",
- (unsigned long)l1e_get_intpte(ol1e), addr, frame);
+ MEM_LOG("PTE %"PRIpte" at %"PRIx64" doesn't match grant (%"PRIpte")",
+ l1e_get_intpte(ol1e), addr,
+ l1e_get_intpte(l1e_from_pfn(frame, grant_pte_flags)));
rc = GNTST_general_error;
goto failed;
}
+ if ( unlikely((l1e_get_flags(ol1e) ^ grant_pte_flags) &
+ ~(_PAGE_AVAIL | PAGE_CACHE_ATTRS)) )
+ MEM_LOG("PTE flags %x at %"PRIx64" don't match grant (%x)\n",
+ l1e_get_flags(ol1e), addr, grant_pte_flags);
+
/* Delete pagetable entry. */
if ( unlikely(!UPDATE_ENTRY
(l1,
@@ -4081,7 +4093,7 @@ static int destroy_grant_pte_mapping(
0)) )
{
page_unlock(page);
- MEM_LOG("Cannot delete PTE entry at %p", va);
+ MEM_LOG("Cannot delete PTE entry at %"PRIx64, addr);
rc = GNTST_general_error;
goto failed;
}
@@ -4149,7 +4161,8 @@ static int create_grant_va_mapping(
}
static int replace_grant_va_mapping(
- unsigned long addr, unsigned long frame, l1_pgentry_t nl1e, struct vcpu *v)
+ unsigned long addr, unsigned long frame, unsigned int grant_pte_flags,
+ l1_pgentry_t nl1e, struct vcpu *v)
{
l1_pgentry_t *pl1e, ol1e;
unsigned long gl1mfn;
@@ -4185,19 +4198,30 @@ static int replace_grant_va_mapping(
ol1e = *pl1e;
- /* Check that the virtual address supplied is actually mapped to frame. */
- if ( unlikely(l1e_get_pfn(ol1e) != frame) )
- {
- MEM_LOG("PTE entry %lx for address %lx doesn't match frame %lx",
- l1e_get_pfn(ol1e), addr, frame);
+ /*
+ * Check that the virtual address supplied is actually mapped to frame
+ * (with appropriate permissions).
+ */
+ if ( unlikely(l1e_get_pfn(ol1e) != frame) ||
+ unlikely((l1e_get_flags(ol1e) ^ grant_pte_flags) &
+ (_PAGE_PRESENT | _PAGE_RW)) )
+ {
+ MEM_LOG("PTE %"PRIpte" for %lx doesn't match grant (%"PRIpte")",
+ l1e_get_intpte(ol1e), addr,
+ l1e_get_intpte(l1e_from_pfn(frame, grant_pte_flags)));
rc = GNTST_general_error;
goto unlock_and_out;
}
+ if ( unlikely((l1e_get_flags(ol1e) ^ grant_pte_flags) &
+ ~(_PAGE_AVAIL | PAGE_CACHE_ATTRS)) )
+ MEM_LOG("PTE flags %x for %"PRIx64" don't match grant (%x)",
+ l1e_get_flags(ol1e), addr, grant_pte_flags);
+
/* Delete pagetable entry. */
if ( unlikely(!UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, gl1mfn, v, 0)) )
{
- MEM_LOG("Cannot delete PTE entry at %p", (unsigned long *)pl1e);
+ MEM_LOG("Cannot delete PTE entry for %"PRIx64, addr);
rc = GNTST_general_error;
goto unlock_and_out;
}
@@ -4211,9 +4235,11 @@ static int replace_grant_va_mapping(
}
static int destroy_grant_va_mapping(
- unsigned long addr, unsigned long frame, struct vcpu *v)
+ unsigned long addr, unsigned long frame, unsigned int grant_pte_flags,
+ struct vcpu *v)
{
- return replace_grant_va_mapping(addr, frame, l1e_empty(), v);
+ return replace_grant_va_mapping(addr, frame, grant_pte_flags,
+ l1e_empty(), v);
}
static int create_grant_p2m_mapping(uint64_t addr, unsigned long frame,
@@ -4307,21 +4333,40 @@ int replace_grant_host_mapping(
unsigned long gl1mfn;
struct page_info *l1pg;
int rc;
+ unsigned int grant_pte_flags;
if ( paging_mode_external(current->domain) )
return replace_grant_p2m_mapping(addr, frame, new_addr, flags);
+ grant_pte_flags =
+ _PAGE_PRESENT | _PAGE_ACCESSED | _PAGE_DIRTY | _PAGE_GNTTAB | _PAGE_NX;
+
+ if ( flags & GNTMAP_application_map )
+ grant_pte_flags |= _PAGE_USER;
+ if ( !(flags & GNTMAP_readonly) )
+ grant_pte_flags |= _PAGE_RW;
+ /*
+ * On top of the explicit settings done by create_grant_host_mapping()
+ * also open-code relevant parts of adjust_guest_l1e(). Don't mirror
+ * available and cachability flags, though.
+ */
+ if ( !is_pv_32bit_domain(curr->domain) )
+ grant_pte_flags |= (grant_pte_flags & _PAGE_USER)
+ ? _PAGE_GLOBAL
+ : _PAGE_GUEST_KERNEL | _PAGE_USER;
+
if ( flags & GNTMAP_contains_pte )
{
if ( !new_addr )
- return destroy_grant_pte_mapping(addr, frame, curr->domain);
+ return destroy_grant_pte_mapping(addr, frame, grant_pte_flags,
+ curr->domain);
MEM_LOG("Unsupported grant table operation");
return GNTST_general_error;
}
if ( !new_addr )
- return destroy_grant_va_mapping(addr, frame, curr);
+ return destroy_grant_va_mapping(addr, frame, grant_pte_flags, curr);
pl1e = guest_map_l1e(new_addr, &gl1mfn);
if ( !pl1e )
@@ -4369,7 +4414,7 @@ int replace_grant_host_mapping(
put_page(l1pg);
guest_unmap_l1e(pl1e);
- rc = replace_grant_va_mapping(addr, frame, ol1e, curr);
+ rc = replace_grant_va_mapping(addr, frame, grant_pte_flags, ol1e, curr);
if ( rc && !paging_mode_refcounts(curr->domain) )
put_page_from_l1e(ol1e, curr->domain);
[-- Attachment #7: xsa234-4.9.patch --]
[-- Type: application/octet-stream, Size: 7279 bytes --]
From: Jan Beulich <jbeulich@suse.com>
Subject: gnttab: also validate PTE permissions upon destroy/replace
In order for PTE handling to match up with the reference counting done
by common code, presence and writability of grant mapping PTEs must
also be taken into account; validating just the frame number is not
enough. This is in particular relevant if a guest fiddles with grant
PTEs via non-grant hypercalls.
Note that the flags being passed to replace_grant_host_mapping()
already happen to be those of the existing mapping, so no new function
parameter is needed.
This is XSA-234.
Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4058,7 +4058,8 @@ static int create_grant_pte_mapping(
}
static int destroy_grant_pte_mapping(
- uint64_t addr, unsigned long frame, struct domain *d)
+ uint64_t addr, unsigned long frame, unsigned int grant_pte_flags,
+ struct domain *d)
{
int rc = GNTST_okay;
void *va;
@@ -4104,17 +4105,29 @@ static int destroy_grant_pte_mapping(
ol1e = *(l1_pgentry_t *)va;
- /* Check that the virtual address supplied is actually mapped to frame. */
- if ( unlikely(l1e_get_pfn(ol1e) != frame) )
+ /*
+ * Check that the PTE supplied actually maps frame (with appropriate
+ * permissions).
+ */
+ if ( unlikely(l1e_get_pfn(ol1e) != frame) ||
+ unlikely((l1e_get_flags(ol1e) ^ grant_pte_flags) &
+ (_PAGE_PRESENT | _PAGE_RW)) )
{
page_unlock(page);
- gdprintk(XENLOG_WARNING,
- "PTE entry %"PRIpte" for address %"PRIx64" doesn't match frame %lx\n",
- l1e_get_intpte(ol1e), addr, frame);
+ gdprintk(XENLOG_ERR,
+ "PTE %"PRIpte" at %"PRIx64" doesn't match grant (%"PRIpte")\n",
+ l1e_get_intpte(ol1e), addr,
+ l1e_get_intpte(l1e_from_pfn(frame, grant_pte_flags)));
rc = GNTST_general_error;
goto failed;
}
+ if ( unlikely((l1e_get_flags(ol1e) ^ grant_pte_flags) &
+ ~(_PAGE_AVAIL | PAGE_CACHE_ATTRS)) )
+ gdprintk(XENLOG_WARNING,
+ "PTE flags %x at %"PRIx64" don't match grant (%x)\n",
+ l1e_get_flags(ol1e), addr, grant_pte_flags);
+
/* Delete pagetable entry. */
if ( unlikely(!UPDATE_ENTRY
(l1,
@@ -4123,7 +4136,8 @@ static int destroy_grant_pte_mapping(
0)) )
{
page_unlock(page);
- gdprintk(XENLOG_WARNING, "Cannot delete PTE entry at %p\n", va);
+ gdprintk(XENLOG_WARNING, "Cannot delete PTE entry at %"PRIx64"\n",
+ addr);
rc = GNTST_general_error;
goto failed;
}
@@ -4191,7 +4205,8 @@ static int create_grant_va_mapping(
}
static int replace_grant_va_mapping(
- unsigned long addr, unsigned long frame, l1_pgentry_t nl1e, struct vcpu *v)
+ unsigned long addr, unsigned long frame, unsigned int grant_pte_flags,
+ l1_pgentry_t nl1e, struct vcpu *v)
{
l1_pgentry_t *pl1e, ol1e;
unsigned long gl1mfn;
@@ -4227,20 +4242,33 @@ static int replace_grant_va_mapping(
ol1e = *pl1e;
- /* Check that the virtual address supplied is actually mapped to frame. */
- if ( unlikely(l1e_get_pfn(ol1e) != frame) )
- {
- gdprintk(XENLOG_WARNING,
- "PTE entry %lx for address %lx doesn't match frame %lx\n",
- l1e_get_pfn(ol1e), addr, frame);
+ /*
+ * Check that the virtual address supplied is actually mapped to frame
+ * (with appropriate permissions).
+ */
+ if ( unlikely(l1e_get_pfn(ol1e) != frame) ||
+ unlikely((l1e_get_flags(ol1e) ^ grant_pte_flags) &
+ (_PAGE_PRESENT | _PAGE_RW)) )
+ {
+ gdprintk(XENLOG_ERR,
+ "PTE %"PRIpte" for %lx doesn't match grant (%"PRIpte")\n",
+ l1e_get_intpte(ol1e), addr,
+ l1e_get_intpte(l1e_from_pfn(frame, grant_pte_flags)));
rc = GNTST_general_error;
goto unlock_and_out;
}
+ if ( unlikely((l1e_get_flags(ol1e) ^ grant_pte_flags) &
+ ~(_PAGE_AVAIL | PAGE_CACHE_ATTRS)) )
+ gdprintk(XENLOG_WARNING,
+ "PTE flags %x for %"PRIx64" don't match grant (%x)\n",
+ l1e_get_flags(ol1e), addr, grant_pte_flags);
+
/* Delete pagetable entry. */
if ( unlikely(!UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, gl1mfn, v, 0)) )
{
- gdprintk(XENLOG_WARNING, "Cannot delete PTE entry at %p\n", pl1e);
+ gdprintk(XENLOG_WARNING, "Cannot delete PTE entry for %"PRIx64"\n",
+ addr);
rc = GNTST_general_error;
goto unlock_and_out;
}
@@ -4254,9 +4282,11 @@ static int replace_grant_va_mapping(
}
static int destroy_grant_va_mapping(
- unsigned long addr, unsigned long frame, struct vcpu *v)
+ unsigned long addr, unsigned long frame, unsigned int grant_pte_flags,
+ struct vcpu *v)
{
- return replace_grant_va_mapping(addr, frame, l1e_empty(), v);
+ return replace_grant_va_mapping(addr, frame, grant_pte_flags,
+ l1e_empty(), v);
}
static int create_grant_p2m_mapping(uint64_t addr, unsigned long frame,
@@ -4351,20 +4381,39 @@ int replace_grant_host_mapping(
unsigned long gl1mfn;
struct page_info *l1pg;
int rc;
+ unsigned int grant_pte_flags;
if ( paging_mode_external(current->domain) )
return replace_grant_p2m_mapping(addr, frame, new_addr, flags);
+ grant_pte_flags =
+ _PAGE_PRESENT | _PAGE_ACCESSED | _PAGE_DIRTY | _PAGE_GNTTAB | _PAGE_NX;
+
+ if ( flags & GNTMAP_application_map )
+ grant_pte_flags |= _PAGE_USER;
+ if ( !(flags & GNTMAP_readonly) )
+ grant_pte_flags |= _PAGE_RW;
+ /*
+ * On top of the explicit settings done by create_grant_host_mapping()
+ * also open-code relevant parts of adjust_guest_l1e(). Don't mirror
+ * available and cachability flags, though.
+ */
+ if ( !is_pv_32bit_domain(curr->domain) )
+ grant_pte_flags |= (grant_pte_flags & _PAGE_USER)
+ ? _PAGE_GLOBAL
+ : _PAGE_GUEST_KERNEL | _PAGE_USER;
+
if ( flags & GNTMAP_contains_pte )
{
if ( !new_addr )
- return destroy_grant_pte_mapping(addr, frame, curr->domain);
+ return destroy_grant_pte_mapping(addr, frame, grant_pte_flags,
+ curr->domain);
return GNTST_general_error;
}
if ( !new_addr )
- return destroy_grant_va_mapping(addr, frame, curr);
+ return destroy_grant_va_mapping(addr, frame, grant_pte_flags, curr);
pl1e = guest_map_l1e(new_addr, &gl1mfn);
if ( !pl1e )
@@ -4412,7 +4461,7 @@ int replace_grant_host_mapping(
put_page(l1pg);
guest_unmap_l1e(pl1e);
- rc = replace_grant_va_mapping(addr, frame, ol1e, curr);
+ rc = replace_grant_va_mapping(addr, frame, grant_pte_flags, ol1e, curr);
if ( rc && !paging_mode_refcounts(curr->domain) )
put_page_from_l1e(ol1e, curr->domain);
[-- Attachment #8: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] only message in thread
only message in thread, other threads:[~2017-09-12 12:03 UTC | newest]
Thread overview: (only message) (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-09-12 12:03 Xen Security Advisory 234 (CVE-2017-14319) - insufficient grant unmapping checks for x86 PV guests 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).