All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/7] x86/mm: Post XSA-234 cleanup
@ 2017-09-12 12:14 Andrew Cooper
  2017-09-12 12:14 ` [PATCH 1/7] x86/mm: Improvements to PV l1e mapping helpers Andrew Cooper
                   ` (6 more replies)
  0 siblings, 7 replies; 27+ messages in thread
From: Andrew Cooper @ 2017-09-12 12:14 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper

Simplification of the PV grant code

Andrew Cooper (7):
  x86/mm: Improvements to PV l1e mapping helpers
  x86/mm: Factor out the grant flags to pte flags conversion logic
  x86/mm: Misc cleanup to {create,replace}_grant_host_mapping()
  x86/mm: Combine create_grant_{pte,va}_mapping()
  x86/mm: Carve steal_linear_address() out of replace_grant_host_mapping()
  x86/mm: Combine {destroy,replace}_grant_{pte,va}_mapping()
  x86/mm: Prevent 32bit PV guests using out-of-range linear addresses

 xen/arch/x86/mm.c | 549 ++++++++++++++++++++----------------------------------
 1 file changed, 203 insertions(+), 346 deletions(-)

-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH 1/7] x86/mm: Improvements to PV l1e mapping helpers
  2017-09-12 12:14 [PATCH 0/7] x86/mm: Post XSA-234 cleanup Andrew Cooper
@ 2017-09-12 12:14 ` Andrew Cooper
  2017-09-12 12:29   ` Wei Liu
  2017-09-12 12:14 ` [PATCH 2/7] x86/mm: Factor out the grant flags to pte flags conversion logic Andrew Cooper
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 27+ messages in thread
From: Andrew Cooper @ 2017-09-12 12:14 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich

Drop guest_unmap_l1e() and use unmap_domain_page() directly.  This will
simplify future cleanup.  Rename guest_map_l1e() to map_guest_l1e() to closer
match the mapping nomenclature.

Switch map_guest_l1e() to using mfn_t.  Correct the comment to indicate that
it takes a linear address (not a virtual address), and correct the parameter
name.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/mm.c | 85 +++++++++++++++++++++++++++----------------------------
 1 file changed, 41 insertions(+), 44 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index e4fa60e..efb3995 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -534,20 +534,23 @@ void update_cr3(struct vcpu *v)
     make_cr3(v, cr3_mfn);
 }
 
-/* Get a mapping of a PV guest's l1e for this virtual address. */
-static l1_pgentry_t *guest_map_l1e(unsigned long addr, unsigned long *gl1mfn)
+/*
+ * Get a mapping of a PV guest's l1e for this linear address.  The return
+ * pointer should be unmapped using unmap_domain_page().
+ */
+static l1_pgentry_t *map_guest_l1e(unsigned long linear, mfn_t *gl1mfn)
 {
     l2_pgentry_t l2e;
 
     ASSERT(!paging_mode_translate(current->domain));
     ASSERT(!paging_mode_external(current->domain));
 
-    if ( unlikely(!__addr_ok(addr)) )
+    if ( unlikely(!__addr_ok(linear)) )
         return NULL;
 
     /* Find this l1e and its enclosing l1mfn in the linear map. */
     if ( __copy_from_user(&l2e,
-                          &__linear_l2_table[l2_linear_offset(addr)],
+                          &__linear_l2_table[l2_linear_offset(linear)],
                           sizeof(l2_pgentry_t)) )
         return NULL;
 
@@ -555,16 +558,9 @@ static l1_pgentry_t *guest_map_l1e(unsigned long addr, unsigned long *gl1mfn)
     if ( (l2e_get_flags(l2e) & (_PAGE_PRESENT | _PAGE_PSE)) != _PAGE_PRESENT )
         return NULL;
 
-    *gl1mfn = l2e_get_pfn(l2e);
-
-    return (l1_pgentry_t *)map_domain_page(_mfn(*gl1mfn)) +
-           l1_table_offset(addr);
-}
+    *gl1mfn = l2e_get_mfn(l2e);
 
-/* Pull down the mapping we got from guest_map_l1e(). */
-static inline void guest_unmap_l1e(void *p)
-{
-    unmap_domain_page(p);
+    return (l1_pgentry_t *)map_domain_page(*gl1mfn) + l1_table_offset(linear);
 }
 
 /* Read a PV guest's l1e that maps this linear address. */
@@ -3977,30 +3973,30 @@ static int create_grant_va_mapping(
 {
     l1_pgentry_t *pl1e, ol1e;
     struct domain *d = v->domain;
-    unsigned long gl1mfn;
+    mfn_t gl1mfn;
     struct page_info *l1pg;
     int okay;
 
     nl1e = adjust_guest_l1e(nl1e, d);
 
-    pl1e = guest_map_l1e(va, &gl1mfn);
+    pl1e = map_guest_l1e(va, &gl1mfn);
     if ( !pl1e )
     {
         gdprintk(XENLOG_WARNING, "Could not find L1 PTE for address %lx\n", va);
         return GNTST_general_error;
     }
 
-    if ( !get_page_from_mfn(_mfn(gl1mfn), current->domain) )
+    if ( !get_page_from_mfn(gl1mfn, current->domain) )
     {
-        guest_unmap_l1e(pl1e);
+        unmap_domain_page(pl1e);
         return GNTST_general_error;
     }
 
-    l1pg = mfn_to_page(_mfn(gl1mfn));
+    l1pg = mfn_to_page(gl1mfn);
     if ( !page_lock(l1pg) )
     {
         put_page(l1pg);
-        guest_unmap_l1e(pl1e);
+        unmap_domain_page(pl1e);
         return GNTST_general_error;
     }
 
@@ -4008,16 +4004,16 @@ static int create_grant_va_mapping(
     {
         page_unlock(l1pg);
         put_page(l1pg);
-        guest_unmap_l1e(pl1e);
+        unmap_domain_page(pl1e);
         return GNTST_general_error;
     }
 
     ol1e = *pl1e;
-    okay = UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, gl1mfn, v, 0);
+    okay = UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, mfn_x(gl1mfn), v, 0);
 
     page_unlock(l1pg);
     put_page(l1pg);
-    guest_unmap_l1e(pl1e);
+    unmap_domain_page(pl1e);
 
     if ( okay )
         put_page_from_l1e(ol1e, d);
@@ -4030,24 +4026,24 @@ static int replace_grant_va_mapping(
     l1_pgentry_t nl1e, struct vcpu *v)
 {
     l1_pgentry_t *pl1e, ol1e;
-    unsigned long gl1mfn;
+    mfn_t gl1mfn;
     struct page_info *l1pg;
     int rc = 0;
 
-    pl1e = guest_map_l1e(addr, &gl1mfn);
+    pl1e = map_guest_l1e(addr, &gl1mfn);
     if ( !pl1e )
     {
         gdprintk(XENLOG_WARNING, "Could not find L1 PTE for address %lx\n", addr);
         return GNTST_general_error;
     }
 
-    if ( !get_page_from_mfn(_mfn(gl1mfn), current->domain) )
+    if ( !get_page_from_mfn(gl1mfn, current->domain) )
     {
         rc = GNTST_general_error;
         goto out;
     }
 
-    l1pg = mfn_to_page(_mfn(gl1mfn));
+    l1pg = mfn_to_page(gl1mfn);
     if ( !page_lock(l1pg) )
     {
         rc = GNTST_general_error;
@@ -4086,7 +4082,7 @@ static int replace_grant_va_mapping(
                  l1e_get_flags(ol1e), addr, grant_pte_flags);
 
     /* Delete pagetable entry. */
-    if ( unlikely(!UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, gl1mfn, v, 0)) )
+    if ( unlikely(!UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, mfn_x(gl1mfn), v, 0)) )
     {
         gdprintk(XENLOG_WARNING, "Cannot delete PTE entry for %"PRIx64"\n",
                  addr);
@@ -4098,7 +4094,7 @@ static int replace_grant_va_mapping(
     page_unlock(l1pg);
     put_page(l1pg);
  out:
-    guest_unmap_l1e(pl1e);
+    unmap_domain_page(pl1e);
     return rc;
 }
 
@@ -4143,7 +4139,7 @@ int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
 {
     struct vcpu *curr = current;
     l1_pgentry_t *pl1e, ol1e;
-    unsigned long gl1mfn;
+    mfn_t gl1mfn;
     struct page_info *l1pg;
     int rc;
     unsigned int grant_pte_flags;
@@ -4177,7 +4173,7 @@ int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
     if ( !new_addr )
         return destroy_grant_va_mapping(addr, frame, grant_pte_flags, curr);
 
-    pl1e = guest_map_l1e(new_addr, &gl1mfn);
+    pl1e = map_guest_l1e(new_addr, &gl1mfn);
     if ( !pl1e )
     {
         gdprintk(XENLOG_WARNING,
@@ -4185,17 +4181,17 @@ int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
         return GNTST_general_error;
     }
 
-    if ( !get_page_from_mfn(_mfn(gl1mfn), current->domain) )
+    if ( !get_page_from_mfn(gl1mfn, current->domain) )
     {
-        guest_unmap_l1e(pl1e);
+        unmap_domain_page(pl1e);
         return GNTST_general_error;
     }
 
-    l1pg = mfn_to_page(_mfn(gl1mfn));
+    l1pg = mfn_to_page(gl1mfn);
     if ( !page_lock(l1pg) )
     {
         put_page(l1pg);
-        guest_unmap_l1e(pl1e);
+        unmap_domain_page(pl1e);
         return GNTST_general_error;
     }
 
@@ -4203,25 +4199,25 @@ int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
     {
         page_unlock(l1pg);
         put_page(l1pg);
-        guest_unmap_l1e(pl1e);
+        unmap_domain_page(pl1e);
         return GNTST_general_error;
     }
 
     ol1e = *pl1e;
 
     if ( unlikely(!UPDATE_ENTRY(l1, pl1e, ol1e, l1e_empty(),
-                                gl1mfn, curr, 0)) )
+                                mfn_x(gl1mfn), curr, 0)) )
     {
         page_unlock(l1pg);
         put_page(l1pg);
         gdprintk(XENLOG_WARNING, "Cannot delete PTE entry at %p\n", pl1e);
-        guest_unmap_l1e(pl1e);
+        unmap_domain_page(pl1e);
         return GNTST_general_error;
     }
 
     page_unlock(l1pg);
     put_page(l1pg);
-    guest_unmap_l1e(pl1e);
+    unmap_domain_page(pl1e);
 
     rc = replace_grant_va_mapping(addr, frame, grant_pte_flags, ol1e, curr);
     if ( rc )
@@ -4344,7 +4340,8 @@ static int __do_update_va_mapping(
     struct domain *d   = v->domain;
     struct page_info *gl1pg;
     l1_pgentry_t  *pl1e;
-    unsigned long  bmap_ptr, gl1mfn;
+    unsigned long  bmap_ptr;
+    mfn_t          gl1mfn;
     cpumask_t     *mask = NULL;
     int            rc;
 
@@ -4355,11 +4352,11 @@ static int __do_update_va_mapping(
         return rc;
 
     rc = -EINVAL;
-    pl1e = guest_map_l1e(va, &gl1mfn);
-    if ( unlikely(!pl1e || !get_page_from_mfn(_mfn(gl1mfn), d)) )
+    pl1e = map_guest_l1e(va, &gl1mfn);
+    if ( unlikely(!pl1e || !get_page_from_mfn(gl1mfn, d)) )
         goto out;
 
-    gl1pg = mfn_to_page(_mfn(gl1mfn));
+    gl1pg = mfn_to_page(gl1mfn);
     if ( !page_lock(gl1pg) )
     {
         put_page(gl1pg);
@@ -4373,14 +4370,14 @@ static int __do_update_va_mapping(
         goto out;
     }
 
-    rc = mod_l1_entry(pl1e, val, gl1mfn, 0, v, pg_owner);
+    rc = mod_l1_entry(pl1e, val, mfn_x(gl1mfn), 0, v, pg_owner);
 
     page_unlock(gl1pg);
     put_page(gl1pg);
 
  out:
     if ( pl1e )
-        guest_unmap_l1e(pl1e);
+        unmap_domain_page(pl1e);
 
     switch ( flags & UVMF_FLUSHTYPE_MASK )
     {
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH 2/7] x86/mm: Factor out the grant flags to pte flags conversion logic
  2017-09-12 12:14 [PATCH 0/7] x86/mm: Post XSA-234 cleanup Andrew Cooper
  2017-09-12 12:14 ` [PATCH 1/7] x86/mm: Improvements to PV l1e mapping helpers Andrew Cooper
@ 2017-09-12 12:14 ` Andrew Cooper
  2017-09-12 13:28   ` Wei Liu
  2017-09-12 12:14 ` [PATCH 3/7] x86/mm: Misc cleanup to {create, replace}_grant_host_mapping() Andrew Cooper
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 27+ messages in thread
From: Andrew Cooper @ 2017-09-12 12:14 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich

This fixes a bug where the requested AVAIL* flags were not honoured in an
unmap_and_replace operation.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/mm.c | 43 ++++++++++++++++++-------------------------
 1 file changed, 18 insertions(+), 25 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index efb3995..d5aba96 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -3814,6 +3814,22 @@ long do_mmu_update(
     return rc;
 }
 
+static unsigned int grant_to_pte_flags(unsigned int grant_flags,
+                                       unsigned int cache_flags)
+{
+    unsigned int pte_flags =
+        _PAGE_PRESENT | _PAGE_ACCESSED | _PAGE_DIRTY | _PAGE_GNTTAB | _PAGE_NX;
+
+    if ( grant_flags & GNTMAP_application_map )
+        pte_flags |= _PAGE_USER;
+    if ( !(grant_flags & GNTMAP_readonly) )
+        pte_flags |= _PAGE_RW;
+
+    pte_flags |= MASK_INSR((grant_flags >> _GNTMAP_guest_avail0), _PAGE_AVAIL);
+    pte_flags |= cacheattr_to_pte_flags(cache_flags >> 5);
+
+    return pte_flags;
+}
 
 static int create_grant_pte_mapping(
     uint64_t pte_addr, l1_pgentry_t nl1e, struct vcpu *v)
@@ -4110,24 +4126,8 @@ int create_grant_pv_mapping(uint64_t addr, unsigned long frame,
                             unsigned int flags, unsigned int cache_flags)
 {
     l1_pgentry_t pte;
-    uint32_t grant_pte_flags;
 
-    grant_pte_flags =
-        _PAGE_PRESENT | _PAGE_ACCESSED | _PAGE_DIRTY | _PAGE_GNTTAB;
-    if ( cpu_has_nx )
-        grant_pte_flags |= _PAGE_NX_BIT;
-
-    pte = l1e_from_pfn(frame, grant_pte_flags);
-    if ( (flags & GNTMAP_application_map) )
-        l1e_add_flags(pte,_PAGE_USER);
-    if ( !(flags & GNTMAP_readonly) )
-        l1e_add_flags(pte,_PAGE_RW);
-
-    l1e_add_flags(pte,
-                  ((flags >> _GNTMAP_guest_avail0) * _PAGE_AVAIL0)
-                   & _PAGE_AVAIL);
-
-    l1e_add_flags(pte, cacheattr_to_pte_flags(cache_flags >> 5));
+    pte = l1e_from_pfn(frame, grant_to_pte_flags(flags, cache_flags));
 
     if ( flags & GNTMAP_contains_pte )
         return create_grant_pte_mapping(addr, pte, current);
@@ -4142,15 +4142,8 @@ int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
     mfn_t 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;
+    unsigned int grant_pte_flags = grant_to_pte_flags(flags, 0);
 
-    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
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH 3/7] x86/mm: Misc cleanup to {create, replace}_grant_host_mapping()
  2017-09-12 12:14 [PATCH 0/7] x86/mm: Post XSA-234 cleanup Andrew Cooper
  2017-09-12 12:14 ` [PATCH 1/7] x86/mm: Improvements to PV l1e mapping helpers Andrew Cooper
  2017-09-12 12:14 ` [PATCH 2/7] x86/mm: Factor out the grant flags to pte flags conversion logic Andrew Cooper
@ 2017-09-12 12:14 ` Andrew Cooper
  2017-09-12 13:40   ` Wei Liu
  2017-09-12 12:14 ` [PATCH 4/7] x86/mm: Combine create_grant_{pte, va}_mapping() Andrew Cooper
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 27+ messages in thread
From: Andrew Cooper @ 2017-09-12 12:14 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich

The purpose of this patch is solely to simplify the resulting diff of later
changes.

 * Factor out curr and currd at the start of the functions.
 * Rename pte to nl1e.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/mm.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index d5aba96..a848d7d 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4125,19 +4125,21 @@ static int destroy_grant_va_mapping(
 int create_grant_pv_mapping(uint64_t addr, unsigned long frame,
                             unsigned int flags, unsigned int cache_flags)
 {
-    l1_pgentry_t pte;
+    struct vcpu *curr = current;
+    l1_pgentry_t nl1e;
 
-    pte = l1e_from_pfn(frame, grant_to_pte_flags(flags, cache_flags));
+    nl1e = l1e_from_pfn(frame, grant_to_pte_flags(flags, cache_flags));
 
     if ( flags & GNTMAP_contains_pte )
-        return create_grant_pte_mapping(addr, pte, current);
-    return create_grant_va_mapping(addr, pte, current);
+        return create_grant_pte_mapping(addr, nl1e, curr);
+    return create_grant_va_mapping(addr, nl1e, curr);
 }
 
 int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
                              uint64_t new_addr, unsigned int flags)
 {
     struct vcpu *curr = current;
+    struct domain *currd = curr->domain;
     l1_pgentry_t *pl1e, ol1e;
     mfn_t gl1mfn;
     struct page_info *l1pg;
@@ -4149,7 +4151,7 @@ int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
      * also open-code relevant parts of adjust_guest_l1e(). Don't mirror
      * available and cachability flags, though.
      */
-    if ( !is_pv_32bit_domain(curr->domain) )
+    if ( !is_pv_32bit_domain(currd) )
         grant_pte_flags |= (grant_pte_flags & _PAGE_USER)
                            ? _PAGE_GLOBAL
                            : _PAGE_GUEST_KERNEL | _PAGE_USER;
@@ -4158,7 +4160,7 @@ int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
     {
         if ( !new_addr )
             return destroy_grant_pte_mapping(addr, frame, grant_pte_flags,
-                                             curr->domain);
+                                             currd);
 
         return GNTST_general_error;
     }
@@ -4174,7 +4176,7 @@ int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
         return GNTST_general_error;
     }
 
-    if ( !get_page_from_mfn(gl1mfn, current->domain) )
+    if ( !get_page_from_mfn(gl1mfn, currd) )
     {
         unmap_domain_page(pl1e);
         return GNTST_general_error;
@@ -4214,7 +4216,7 @@ int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
 
     rc = replace_grant_va_mapping(addr, frame, grant_pte_flags, ol1e, curr);
     if ( rc )
-        put_page_from_l1e(ol1e, curr->domain);
+        put_page_from_l1e(ol1e, currd);
 
     return rc;
 }
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH 4/7] x86/mm: Combine create_grant_{pte, va}_mapping()
  2017-09-12 12:14 [PATCH 0/7] x86/mm: Post XSA-234 cleanup Andrew Cooper
                   ` (2 preceding siblings ...)
  2017-09-12 12:14 ` [PATCH 3/7] x86/mm: Misc cleanup to {create, replace}_grant_host_mapping() Andrew Cooper
@ 2017-09-12 12:14 ` Andrew Cooper
  2017-09-12 14:04   ` Wei Liu
  2017-09-12 15:31   ` Jan Beulich
  2017-09-12 12:14 ` [PATCH 5/7] x86/mm: Carve steal_linear_address() out of replace_grant_host_mapping() Andrew Cooper
                   ` (2 subsequent siblings)
  6 siblings, 2 replies; 27+ messages in thread
From: Andrew Cooper @ 2017-09-12 12:14 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich

create_grant_{pte,va}_mapping() are nearly identical; all that is really
different between them is how they convert their addr parameter to the pte to
install the grant into.

Reimplement their logic in create_grant_pv_mapping() in a mostly common way.

No (intended) change in behaviour from a guests point of view.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/mm.c | 174 ++++++++++++++++++------------------------------------
 1 file changed, 57 insertions(+), 117 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index a848d7d..e08b4a8 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -3831,66 +3831,6 @@ static unsigned int grant_to_pte_flags(unsigned int grant_flags,
     return pte_flags;
 }
 
-static int create_grant_pte_mapping(
-    uint64_t pte_addr, l1_pgentry_t nl1e, struct vcpu *v)
-{
-    int rc = GNTST_okay;
-    void *va;
-    unsigned long gmfn, mfn;
-    struct page_info *page;
-    l1_pgentry_t ol1e;
-    struct domain *d = v->domain;
-
-    if ( !IS_ALIGNED(pte_addr, sizeof(nl1e)) )
-        return GNTST_general_error;
-
-    nl1e = adjust_guest_l1e(nl1e, d);
-
-    gmfn = pte_addr >> PAGE_SHIFT;
-    page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
-
-    if ( unlikely(!page) )
-    {
-        gdprintk(XENLOG_WARNING, "Could not get page for normal update\n");
-        return GNTST_general_error;
-    }
-
-    mfn = mfn_x(page_to_mfn(page));
-    va = map_domain_page(_mfn(mfn));
-    va = (void *)((unsigned long)va + ((unsigned long)pte_addr & ~PAGE_MASK));
-
-    if ( !page_lock(page) )
-    {
-        rc = GNTST_general_error;
-        goto failed;
-    }
-
-    if ( (page->u.inuse.type_info & PGT_type_mask) != PGT_l1_page_table )
-    {
-        page_unlock(page);
-        rc = GNTST_general_error;
-        goto failed;
-    }
-
-    ol1e = *(l1_pgentry_t *)va;
-    if ( !UPDATE_ENTRY(l1, (l1_pgentry_t *)va, ol1e, nl1e, mfn, v, 0) )
-    {
-        page_unlock(page);
-        rc = GNTST_general_error;
-        goto failed;
-    }
-
-    page_unlock(page);
-
-    put_page_from_l1e(ol1e, d);
-
- failed:
-    unmap_domain_page(va);
-    put_page(page);
-
-    return rc;
-}
-
 static int destroy_grant_pte_mapping(
     uint64_t addr, unsigned long frame, unsigned int grant_pte_flags,
     struct domain *d)
@@ -3983,60 +3923,6 @@ static int destroy_grant_pte_mapping(
     return rc;
 }
 
-
-static int create_grant_va_mapping(
-    unsigned long va, l1_pgentry_t nl1e, struct vcpu *v)
-{
-    l1_pgentry_t *pl1e, ol1e;
-    struct domain *d = v->domain;
-    mfn_t gl1mfn;
-    struct page_info *l1pg;
-    int okay;
-
-    nl1e = adjust_guest_l1e(nl1e, d);
-
-    pl1e = map_guest_l1e(va, &gl1mfn);
-    if ( !pl1e )
-    {
-        gdprintk(XENLOG_WARNING, "Could not find L1 PTE for address %lx\n", va);
-        return GNTST_general_error;
-    }
-
-    if ( !get_page_from_mfn(gl1mfn, current->domain) )
-    {
-        unmap_domain_page(pl1e);
-        return GNTST_general_error;
-    }
-
-    l1pg = mfn_to_page(gl1mfn);
-    if ( !page_lock(l1pg) )
-    {
-        put_page(l1pg);
-        unmap_domain_page(pl1e);
-        return GNTST_general_error;
-    }
-
-    if ( (l1pg->u.inuse.type_info & PGT_type_mask) != PGT_l1_page_table )
-    {
-        page_unlock(l1pg);
-        put_page(l1pg);
-        unmap_domain_page(pl1e);
-        return GNTST_general_error;
-    }
-
-    ol1e = *pl1e;
-    okay = UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, mfn_x(gl1mfn), v, 0);
-
-    page_unlock(l1pg);
-    put_page(l1pg);
-    unmap_domain_page(pl1e);
-
-    if ( okay )
-        put_page_from_l1e(ol1e, d);
-
-    return okay ? GNTST_okay : GNTST_general_error;
-}
-
 static int replace_grant_va_mapping(
     unsigned long addr, unsigned long frame, unsigned int grant_pte_flags,
     l1_pgentry_t nl1e, struct vcpu *v)
@@ -4126,13 +4012,67 @@ int create_grant_pv_mapping(uint64_t addr, unsigned long frame,
                             unsigned int flags, unsigned int cache_flags)
 {
     struct vcpu *curr = current;
-    l1_pgentry_t nl1e;
+    struct domain *currd = curr->domain;
+    l1_pgentry_t nl1e, ol1e, *pl1e;
+    struct page_info *page;
+    mfn_t gl1mfn;
+    int rc = GNTST_general_error;
 
     nl1e = l1e_from_pfn(frame, grant_to_pte_flags(flags, cache_flags));
+    nl1e = adjust_guest_l1e(nl1e, currd);
 
+    /*
+     * The meaning of addr depends on GNTMAP_contains_pte.  It is either a
+     * machine address of an L1e the guest has nominated to be altered, or a
+     * linear address we need to look up the appropriate L1e for.
+     */
     if ( flags & GNTMAP_contains_pte )
-        return create_grant_pte_mapping(addr, nl1e, curr);
-    return create_grant_va_mapping(addr, nl1e, curr);
+    {
+        /* addr must be suitably aligned, or we will corrupt adjacent ptes. */
+        if ( !IS_ALIGNED(addr, sizeof(nl1e)) )
+            goto out;
+
+        gl1mfn = _mfn(addr >> PAGE_SHIFT);
+
+        if ( !get_page_from_mfn(gl1mfn, currd) )
+            goto out;
+
+        pl1e = map_domain_page(gl1mfn) + (addr & ~PAGE_MASK);
+    }
+    else
+    {
+        pl1e = map_guest_l1e(addr, &gl1mfn);
+
+        if ( !pl1e )
+            goto out;
+
+        if ( !get_page_from_mfn(gl1mfn, currd) )
+            goto out_unmap;
+    }
+
+    page = mfn_to_page(gl1mfn);
+    if ( !page_lock(page) )
+        goto out_put;
+
+    if ( (page->u.inuse.type_info & PGT_type_mask) != PGT_l1_page_table )
+        goto out_unlock;
+
+    ol1e = *pl1e;
+    if ( UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, mfn_x(gl1mfn), curr, 0) )
+        rc = GNTST_okay;
+
+ out_unlock:
+    page_unlock(page);
+ out_put:
+    put_page(page);
+ out_unmap:
+    unmap_domain_page(pl1e);
+
+    if ( rc == GNTST_okay )
+        put_page_from_l1e(ol1e, currd);
+
+ out:
+    return rc;
 }
 
 int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH 5/7] x86/mm: Carve steal_linear_address() out of replace_grant_host_mapping()
  2017-09-12 12:14 [PATCH 0/7] x86/mm: Post XSA-234 cleanup Andrew Cooper
                   ` (3 preceding siblings ...)
  2017-09-12 12:14 ` [PATCH 4/7] x86/mm: Combine create_grant_{pte, va}_mapping() Andrew Cooper
@ 2017-09-12 12:14 ` Andrew Cooper
  2017-09-12 14:19   ` Wei Liu
  2017-09-12 12:14 ` [PATCH 6/7] x86/mm: Combine {destroy, replace}_grant_{pte, va}_mapping() Andrew Cooper
  2017-09-12 12:14 ` [PATCH 7/7] x86/mm: Prevent 32bit PV guests using out-of-range linear addresses Andrew Cooper
  6 siblings, 1 reply; 27+ messages in thread
From: Andrew Cooper @ 2017-09-12 12:14 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich

Document its curious semantics.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/mm.c | 105 ++++++++++++++++++++++++++++++------------------------
 1 file changed, 58 insertions(+), 47 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index e08b4a8..afd98d9 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4075,14 +4075,68 @@ int create_grant_pv_mapping(uint64_t addr, unsigned long frame,
     return rc;
 }
 
-int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
-                             uint64_t new_addr, unsigned int flags)
+/*
+ * This exists soley for implementing GNTABOP_unmap_and_replace, the ABI of
+ * which is bizare.  This GNTTABOP isn't used any more, but was used by
+ * classic-xen kernels and PVOps Linux before the M2P_OVERRIDE infrastructure
+ * was replaced with something which actually worked.
+ *
+ * Look up the L1e mapping linear, and zap it.  Return the L1e via *out.
+ * Returns a boolean indicating success.  If success, the caller is
+ * responsible for calling put_page_from_l1e().
+ */
+static bool steal_linear_address(unsigned long linear, l1_pgentry_t *out)
 {
     struct vcpu *curr = current;
     struct domain *currd = curr->domain;
     l1_pgentry_t *pl1e, ol1e;
+    struct page_info *page;
     mfn_t gl1mfn;
-    struct page_info *l1pg;
+    bool okay = false;
+
+    ASSERT(is_pv_domain(currd));
+
+    pl1e = map_guest_l1e(linear, &gl1mfn);
+    if ( !pl1e )
+    {
+        gdprintk(XENLOG_WARNING,
+                 "Could not find L1 PTE for linear %"PRIx64"\n", linear);
+        goto out;
+    }
+
+    if ( !get_page_from_mfn(gl1mfn, currd) )
+        goto out_unmap;
+
+    page = mfn_to_page(gl1mfn);
+    if ( !page_lock(page) )
+        goto out_put;
+
+    if ( (page->u.inuse.type_info & PGT_type_mask) != PGT_l1_page_table )
+        goto out_unlock;
+
+    ol1e = *pl1e;
+    okay = UPDATE_ENTRY(l1, pl1e, ol1e, l1e_empty(), mfn_x(gl1mfn), curr, 0);
+
+ out_unlock:
+    page_unlock(page);
+ out_put:
+    put_page(page);
+ out_unmap:
+    unmap_domain_page(pl1e);
+
+    if ( okay )
+        *out = ol1e;
+
+ out:
+    return okay;
+}
+
+int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
+                             uint64_t new_addr, unsigned int flags)
+{
+    struct vcpu *curr = current;
+    struct domain *currd = curr->domain;
+    l1_pgentry_t ol1e;
     int rc;
     unsigned int grant_pte_flags = grant_to_pte_flags(flags, 0);
 
@@ -4108,51 +4162,8 @@ int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
     if ( !new_addr )
         return destroy_grant_va_mapping(addr, frame, grant_pte_flags, curr);
 
-    pl1e = map_guest_l1e(new_addr, &gl1mfn);
-    if ( !pl1e )
-    {
-        gdprintk(XENLOG_WARNING,
-                 "Could not find L1 PTE for address %"PRIx64"\n", new_addr);
-        return GNTST_general_error;
-    }
-
-    if ( !get_page_from_mfn(gl1mfn, currd) )
-    {
-        unmap_domain_page(pl1e);
-        return GNTST_general_error;
-    }
-
-    l1pg = mfn_to_page(gl1mfn);
-    if ( !page_lock(l1pg) )
-    {
-        put_page(l1pg);
-        unmap_domain_page(pl1e);
-        return GNTST_general_error;
-    }
-
-    if ( (l1pg->u.inuse.type_info & PGT_type_mask) != PGT_l1_page_table )
-    {
-        page_unlock(l1pg);
-        put_page(l1pg);
-        unmap_domain_page(pl1e);
-        return GNTST_general_error;
-    }
-
-    ol1e = *pl1e;
-
-    if ( unlikely(!UPDATE_ENTRY(l1, pl1e, ol1e, l1e_empty(),
-                                mfn_x(gl1mfn), curr, 0)) )
-    {
-        page_unlock(l1pg);
-        put_page(l1pg);
-        gdprintk(XENLOG_WARNING, "Cannot delete PTE entry at %p\n", pl1e);
-        unmap_domain_page(pl1e);
+    if ( !steal_linear_address(new_addr, &ol1e) )
         return GNTST_general_error;
-    }
-
-    page_unlock(l1pg);
-    put_page(l1pg);
-    unmap_domain_page(pl1e);
 
     rc = replace_grant_va_mapping(addr, frame, grant_pte_flags, ol1e, curr);
     if ( rc )
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH 6/7] x86/mm: Combine {destroy, replace}_grant_{pte, va}_mapping()
  2017-09-12 12:14 [PATCH 0/7] x86/mm: Post XSA-234 cleanup Andrew Cooper
                   ` (4 preceding siblings ...)
  2017-09-12 12:14 ` [PATCH 5/7] x86/mm: Carve steal_linear_address() out of replace_grant_host_mapping() Andrew Cooper
@ 2017-09-12 12:14 ` Andrew Cooper
  2017-09-12 14:58   ` Wei Liu
  2017-09-12 15:46   ` Jan Beulich
  2017-09-12 12:14 ` [PATCH 7/7] x86/mm: Prevent 32bit PV guests using out-of-range linear addresses Andrew Cooper
  6 siblings, 2 replies; 27+ messages in thread
From: Andrew Cooper @ 2017-09-12 12:14 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich

As with the create side of things, these are largely identical.  Most cases
are actually destroying the mapping rather than replacing it with a stolen
entry.

Reimplement their logic in replace_grant_pv_mapping() in a mostly common
way.

No (intended) change in behaviour from a guests point of view.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/mm.c | 277 +++++++++++++++++-------------------------------------
 1 file changed, 87 insertions(+), 190 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index afd98d9..1a8ad42 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -3831,183 +3831,6 @@ static unsigned int grant_to_pte_flags(unsigned int grant_flags,
     return pte_flags;
 }
 
-static int destroy_grant_pte_mapping(
-    uint64_t addr, unsigned long frame, unsigned int grant_pte_flags,
-    struct domain *d)
-{
-    int rc = GNTST_okay;
-    void *va;
-    unsigned long gmfn, mfn;
-    struct page_info *page;
-    l1_pgentry_t ol1e;
-
-    /*
-     * addr comes from Xen's active_entry tracking so isn't guest controlled,
-     * but it had still better be PTE-aligned.
-     */
-    if ( !IS_ALIGNED(addr, sizeof(ol1e)) )
-    {
-        ASSERT_UNREACHABLE();
-        return GNTST_general_error;
-    }
-
-    gmfn = addr >> PAGE_SHIFT;
-    page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
-
-    if ( unlikely(!page) )
-    {
-        gdprintk(XENLOG_WARNING, "Could not get page for normal update\n");
-        return GNTST_general_error;
-    }
-
-    mfn = mfn_x(page_to_mfn(page));
-    va = map_domain_page(_mfn(mfn));
-    va = (void *)((unsigned long)va + ((unsigned long)addr & ~PAGE_MASK));
-
-    if ( !page_lock(page) )
-    {
-        rc = GNTST_general_error;
-        goto failed;
-    }
-
-    if ( (page->u.inuse.type_info & PGT_type_mask) != PGT_l1_page_table )
-    {
-        page_unlock(page);
-        rc = GNTST_general_error;
-        goto failed;
-    }
-
-    ol1e = *(l1_pgentry_t *)va;
-
-    /*
-     * 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_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,
-                                d->vcpu[0] /* Change if we go to per-vcpu shadows. */,
-                                0)) )
-    {
-        page_unlock(page);
-        gdprintk(XENLOG_WARNING, "Cannot delete PTE entry at %"PRIx64"\n",
-                 addr);
-        rc = GNTST_general_error;
-        goto failed;
-    }
-
-    page_unlock(page);
-
- failed:
-    unmap_domain_page(va);
-    put_page(page);
-    return rc;
-}
-
-static int replace_grant_va_mapping(
-    unsigned long addr, unsigned long frame, unsigned int grant_pte_flags,
-    l1_pgentry_t nl1e, struct vcpu *v)
-{
-    l1_pgentry_t *pl1e, ol1e;
-    mfn_t gl1mfn;
-    struct page_info *l1pg;
-    int rc = 0;
-
-    pl1e = map_guest_l1e(addr, &gl1mfn);
-    if ( !pl1e )
-    {
-        gdprintk(XENLOG_WARNING, "Could not find L1 PTE for address %lx\n", addr);
-        return GNTST_general_error;
-    }
-
-    if ( !get_page_from_mfn(gl1mfn, current->domain) )
-    {
-        rc = GNTST_general_error;
-        goto out;
-    }
-
-    l1pg = mfn_to_page(gl1mfn);
-    if ( !page_lock(l1pg) )
-    {
-        rc = GNTST_general_error;
-        put_page(l1pg);
-        goto out;
-    }
-
-    if ( (l1pg->u.inuse.type_info & PGT_type_mask) != PGT_l1_page_table )
-    {
-        rc = GNTST_general_error;
-        goto unlock_and_out;
-    }
-
-    ol1e = *pl1e;
-
-    /*
-     * 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, mfn_x(gl1mfn), v, 0)) )
-    {
-        gdprintk(XENLOG_WARNING, "Cannot delete PTE entry for %"PRIx64"\n",
-                 addr);
-        rc = GNTST_general_error;
-        goto unlock_and_out;
-    }
-
- unlock_and_out:
-    page_unlock(l1pg);
-    put_page(l1pg);
- out:
-    unmap_domain_page(pl1e);
-    return rc;
-}
-
-static int destroy_grant_va_mapping(
-    unsigned long addr, unsigned long frame, unsigned int grant_pte_flags,
-    struct vcpu *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,
                             unsigned int flags, unsigned int cache_flags)
 {
@@ -4136,12 +3959,14 @@ int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
 {
     struct vcpu *curr = current;
     struct domain *currd = curr->domain;
-    l1_pgentry_t ol1e;
-    int rc;
+    l1_pgentry_t nl1e = l1e_empty(), ol1e, *pl1e;
+    struct page_info *page;
+    mfn_t gl1mfn;
+    int rc = GNTST_general_error;
     unsigned int grant_pte_flags = grant_to_pte_flags(flags, 0);
 
     /*
-     * On top of the explicit settings done by create_grant_host_mapping()
+     * On top of the explicit settings done by create_pv_host_mapping()
      * also open-code relevant parts of adjust_guest_l1e(). Don't mirror
      * available and cachability flags, though.
      */
@@ -4150,24 +3975,96 @@ int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
                            ? _PAGE_GLOBAL
                            : _PAGE_GUEST_KERNEL | _PAGE_USER;
 
+    /*
+     * addr comes from Xen's active_entry tracking, and was used successfully
+     * to create a grant.
+     *
+     * The meaning of addr depends on GNTMAP_contains_pte.  It is either a
+     * machine address of an L1e the guest has nominated to be altered, or a
+     * linear address we need to look up the appropriate L1e for.
+     *
+     * Passing a new_addr of zero is taken to mean destroy.  Passing a
+     * non-zero new_addr has only ever been available via
+     * GNTABOP_unmap_and_replace and only when using linear addresses.
+     */
     if ( flags & GNTMAP_contains_pte )
     {
-        if ( !new_addr )
-            return destroy_grant_pte_mapping(addr, frame, grant_pte_flags,
-                                             currd);
+        /* Replace not available in this addressing mode. */
+        if ( new_addr )
+            goto out;
+
+        if ( !IS_ALIGNED(addr, sizeof(nl1e)) )
+        {
+            ASSERT_UNREACHABLE();
+            goto out;
+        }
+
+        gl1mfn = _mfn(addr >> PAGE_SHIFT);
+
+        if ( !get_page_from_mfn(gl1mfn, currd) )
+            goto out;
+
+        pl1e = map_domain_page(gl1mfn) + (addr & ~PAGE_MASK);
+    }
+    else
+    {
+        if ( new_addr && !steal_linear_address(new_addr, &nl1e) )
+            goto out;
+
+        pl1e = map_guest_l1e(addr, &gl1mfn);
 
-        return GNTST_general_error;
+        if ( !pl1e )
+            goto out;
+
+        if ( !get_page_from_mfn(gl1mfn, currd) )
+            goto out_unmap;
+    }
+
+    page = mfn_to_page(gl1mfn);
+
+    if ( !page_lock(page) )
+        goto out_put;
+
+    if ( (page->u.inuse.type_info & PGT_type_mask) != PGT_l1_page_table )
+        goto out_unlock;
+
+    ol1e = *pl1e;
+
+    /*
+     * 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)));
+        goto out_unlock;
     }
 
-    if ( !new_addr )
-        return destroy_grant_va_mapping(addr, frame, grant_pte_flags, curr);
+    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);
 
-    if ( !steal_linear_address(new_addr, &ol1e) )
-        return GNTST_general_error;
+    if ( UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, mfn_x(gl1mfn), curr, 0) )
+        rc = GNTST_okay;
 
-    rc = replace_grant_va_mapping(addr, frame, grant_pte_flags, ol1e, curr);
+ out_unlock:
+    page_unlock(page);
+ out_put:
+    put_page(page);
+ out_unmap:
+    unmap_domain_page(pl1e);
+
+ out:
+    /* If there was an error, we are still responsible for the stolen pte. */
     if ( rc )
-        put_page_from_l1e(ol1e, currd);
+        put_page_from_l1e(nl1e, currd);
 
     return rc;
 }
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH 7/7] x86/mm: Prevent 32bit PV guests using out-of-range linear addresses
  2017-09-12 12:14 [PATCH 0/7] x86/mm: Post XSA-234 cleanup Andrew Cooper
                   ` (5 preceding siblings ...)
  2017-09-12 12:14 ` [PATCH 6/7] x86/mm: Combine {destroy, replace}_grant_{pte, va}_mapping() Andrew Cooper
@ 2017-09-12 12:14 ` Andrew Cooper
  2017-09-12 15:50   ` Jan Beulich
  6 siblings, 1 reply; 27+ messages in thread
From: Andrew Cooper @ 2017-09-12 12:14 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich

The grant ABI uses 64 bit values, and allows a PV guest to specify linear
addresses.  There is nothing interesting a 32bit PV guest can reference which
will pass an __addr_ok() check, but it should still get an error for trying.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/mm.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 1a8ad42..edf8fdf 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -3864,6 +3864,10 @@ int create_grant_pv_mapping(uint64_t addr, unsigned long frame,
     }
     else
     {
+        /* Guest trying to pass an out-of-range linear address? */
+        if ( is_pv_32bit_domain(currd) && addr != (uint32_t)addr )
+            goto out;
+
         pl1e = map_guest_l1e(addr, &gl1mfn);
 
         if ( !pl1e )
@@ -4008,6 +4012,19 @@ int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
     }
     else
     {
+        if ( is_pv_32bit_domain(currd) )
+        {
+            if ( addr != (uint32_t)addr )
+            {
+                ASSERT_UNREACHABLE();
+                goto out;
+            }
+
+            /* Guest trying to pass an out-of-range linear address? */
+            if ( new_addr != (uint32_t)new_addr )
+                goto out;
+        }
+
         if ( new_addr && !steal_linear_address(new_addr, &nl1e) )
             goto out;
 
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/7] x86/mm: Improvements to PV l1e mapping helpers
  2017-09-12 12:14 ` [PATCH 1/7] x86/mm: Improvements to PV l1e mapping helpers Andrew Cooper
@ 2017-09-12 12:29   ` Wei Liu
  2017-09-12 15:22     ` Jan Beulich
  0 siblings, 1 reply; 27+ messages in thread
From: Wei Liu @ 2017-09-12 12:29 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Wei Liu, Jan Beulich, Xen-devel

On Tue, Sep 12, 2017 at 01:14:40PM +0100, Andrew Cooper wrote:
> Drop guest_unmap_l1e() and use unmap_domain_page() directly.  This will
> simplify future cleanup.  Rename guest_map_l1e() to map_guest_l1e() to closer
> match the mapping nomenclature.
> 
> Switch map_guest_l1e() to using mfn_t.  Correct the comment to indicate that
> it takes a linear address (not a virtual address), and correct the parameter
> name.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 2/7] x86/mm: Factor out the grant flags to pte flags conversion logic
  2017-09-12 12:14 ` [PATCH 2/7] x86/mm: Factor out the grant flags to pte flags conversion logic Andrew Cooper
@ 2017-09-12 13:28   ` Wei Liu
  2017-09-12 15:25     ` Jan Beulich
  0 siblings, 1 reply; 27+ messages in thread
From: Wei Liu @ 2017-09-12 13:28 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Wei Liu, Jan Beulich, Xen-devel

On Tue, Sep 12, 2017 at 01:14:41PM +0100, Andrew Cooper wrote:
> This fixes a bug where the requested AVAIL* flags were not honoured in an
> unmap_and_replace operation.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 3/7] x86/mm: Misc cleanup to {create, replace}_grant_host_mapping()
  2017-09-12 12:14 ` [PATCH 3/7] x86/mm: Misc cleanup to {create, replace}_grant_host_mapping() Andrew Cooper
@ 2017-09-12 13:40   ` Wei Liu
  2017-09-12 15:25     ` Jan Beulich
  0 siblings, 1 reply; 27+ messages in thread
From: Wei Liu @ 2017-09-12 13:40 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Wei Liu, Jan Beulich, Xen-devel

On Tue, Sep 12, 2017 at 01:14:42PM +0100, Andrew Cooper wrote:
> The purpose of this patch is solely to simplify the resulting diff of later
> changes.
> 
>  * Factor out curr and currd at the start of the functions.
>  * Rename pte to nl1e.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 4/7] x86/mm: Combine create_grant_{pte, va}_mapping()
  2017-09-12 12:14 ` [PATCH 4/7] x86/mm: Combine create_grant_{pte, va}_mapping() Andrew Cooper
@ 2017-09-12 14:04   ` Wei Liu
  2017-09-12 15:31   ` Jan Beulich
  1 sibling, 0 replies; 27+ messages in thread
From: Wei Liu @ 2017-09-12 14:04 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Wei Liu, Jan Beulich, Xen-devel

On Tue, Sep 12, 2017 at 01:14:43PM +0100, Andrew Cooper wrote:
> create_grant_{pte,va}_mapping() are nearly identical; all that is really
> different between them is how they convert their addr parameter to the pte to
> install the grant into.
> 
> Reimplement their logic in create_grant_pv_mapping() in a mostly common way.
> 
> No (intended) change in behaviour from a guests point of view.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

With one optional request.

> @@ -4126,13 +4012,67 @@ int create_grant_pv_mapping(uint64_t addr, unsigned long frame,
>                              unsigned int flags, unsigned int cache_flags)
>  {
>      struct vcpu *curr = current;
> -    l1_pgentry_t nl1e;
> +    struct domain *currd = curr->domain;
> +    l1_pgentry_t nl1e, ol1e, *pl1e;
> +    struct page_info *page;
> +    mfn_t gl1mfn;
> +    int rc = GNTST_general_error;
>  
>      nl1e = l1e_from_pfn(frame, grant_to_pte_flags(flags, cache_flags));
> +    nl1e = adjust_guest_l1e(nl1e, currd);
>  
> +    /*
> +     * The meaning of addr depends on GNTMAP_contains_pte.  It is either a
> +     * machine address of an L1e the guest has nominated to be altered, or a
> +     * linear address we need to look up the appropriate L1e for.
> +     */
>      if ( flags & GNTMAP_contains_pte )
> -        return create_grant_pte_mapping(addr, nl1e, curr);
> -    return create_grant_va_mapping(addr, nl1e, curr);
> +    {
> +        /* addr must be suitably aligned, or we will corrupt adjacent ptes. */
> +        if ( !IS_ALIGNED(addr, sizeof(nl1e)) )
> +            goto out;

Can you sprinkle some gdprintk's in these error paths to aid debugging?

> +
> +        gl1mfn = _mfn(addr >> PAGE_SHIFT);
> +
> +        if ( !get_page_from_mfn(gl1mfn, currd) )
> +            goto out;
> +
> +        pl1e = map_domain_page(gl1mfn) + (addr & ~PAGE_MASK);
> +    }
> +    else
> +    {
> +        pl1e = map_guest_l1e(addr, &gl1mfn);
> +
> +        if ( !pl1e )
> +            goto out;
> +
> +        if ( !get_page_from_mfn(gl1mfn, currd) )
> +            goto out_unmap;
> +    }
> +
> +    page = mfn_to_page(gl1mfn);
> +    if ( !page_lock(page) )
> +        goto out_put;
> +
> +    if ( (page->u.inuse.type_info & PGT_type_mask) != PGT_l1_page_table )
> +        goto out_unlock;
> +
> +    ol1e = *pl1e;
> +    if ( UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, mfn_x(gl1mfn), curr, 0) )
> +        rc = GNTST_okay;
> +
> + out_unlock:
> +    page_unlock(page);
> + out_put:
> +    put_page(page);
> + out_unmap:
> +    unmap_domain_page(pl1e);
> +
> +    if ( rc == GNTST_okay )
> +        put_page_from_l1e(ol1e, currd);
> +
> + out:
> +    return rc;
>  }
>  
>  int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
> -- 
> 2.1.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 5/7] x86/mm: Carve steal_linear_address() out of replace_grant_host_mapping()
  2017-09-12 12:14 ` [PATCH 5/7] x86/mm: Carve steal_linear_address() out of replace_grant_host_mapping() Andrew Cooper
@ 2017-09-12 14:19   ` Wei Liu
  2017-09-12 15:41     ` Jan Beulich
  0 siblings, 1 reply; 27+ messages in thread
From: Wei Liu @ 2017-09-12 14:19 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Wei Liu, Jan Beulich, Xen-devel

On Tue, Sep 12, 2017 at 01:14:44PM +0100, Andrew Cooper wrote:
> Document its curious semantics.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
>  xen/arch/x86/mm.c | 105 ++++++++++++++++++++++++++++++------------------------
>  1 file changed, 58 insertions(+), 47 deletions(-)
> 
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index e08b4a8..afd98d9 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4075,14 +4075,68 @@ int create_grant_pv_mapping(uint64_t addr, unsigned long frame,
>      return rc;
>  }
>  
> -int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
> -                             uint64_t new_addr, unsigned int flags)
> +/*
> + * This exists soley for implementing GNTABOP_unmap_and_replace, the ABI of
> + * which is bizare.  This GNTTABOP isn't used any more, but was used by

Should be "bizarre".

> + * classic-xen kernels and PVOps Linux before the M2P_OVERRIDE infrastructure
> + * was replaced with something which actually worked.
> + *
> + * Look up the L1e mapping linear, and zap it.  Return the L1e via *out.
> + * Returns a boolean indicating success.  If success, the caller is
> + * responsible for calling put_page_from_l1e().
> + */

Can't say much about the history; code-wise:

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 6/7] x86/mm: Combine {destroy, replace}_grant_{pte, va}_mapping()
  2017-09-12 12:14 ` [PATCH 6/7] x86/mm: Combine {destroy, replace}_grant_{pte, va}_mapping() Andrew Cooper
@ 2017-09-12 14:58   ` Wei Liu
  2017-09-12 16:30     ` Andrew Cooper
  2017-09-12 15:46   ` Jan Beulich
  1 sibling, 1 reply; 27+ messages in thread
From: Wei Liu @ 2017-09-12 14:58 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Wei Liu, Jan Beulich, Xen-devel

On Tue, Sep 12, 2017 at 01:14:45PM +0100, Andrew Cooper wrote:
> As with the create side of things, these are largely identical.  Most cases
> are actually destroying the mapping rather than replacing it with a stolen
> entry.
> 
> Reimplement their logic in replace_grant_pv_mapping() in a mostly common
> way.
> 
> No (intended) change in behaviour from a guests point of view.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

With two suggestions:

>  int create_grant_pv_mapping(uint64_t addr, unsigned long frame,
>                              unsigned int flags, unsigned int cache_flags)
>  {
> @@ -4136,12 +3959,14 @@ int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
>  {
>      struct vcpu *curr = current;
>      struct domain *currd = curr->domain;
> -    l1_pgentry_t ol1e;
> -    int rc;
> +    l1_pgentry_t nl1e = l1e_empty(), ol1e, *pl1e;
> +    struct page_info *page;
> +    mfn_t gl1mfn;
> +    int rc = GNTST_general_error;
>      unsigned int grant_pte_flags = grant_to_pte_flags(flags, 0);
>  
>      /*
> -     * On top of the explicit settings done by create_grant_host_mapping()
> +     * On top of the explicit settings done by create_pv_host_mapping()
>       * also open-code relevant parts of adjust_guest_l1e(). Don't mirror
>       * available and cachability flags, though.
>       */
> @@ -4150,24 +3975,96 @@ int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
>                             ? _PAGE_GLOBAL
>                             : _PAGE_GUEST_KERNEL | _PAGE_USER;
>  
> +    /*
> +     * addr comes from Xen's active_entry tracking, and was used successfully
> +     * to create a grant.
> +     *
> +     * The meaning of addr depends on GNTMAP_contains_pte.  It is either a
> +     * machine address of an L1e the guest has nominated to be altered, or a
> +     * linear address we need to look up the appropriate L1e for.
> +     *
> +     * Passing a new_addr of zero is taken to mean destroy.  Passing a
> +     * non-zero new_addr has only ever been available via
> +     * GNTABOP_unmap_and_replace and only when using linear addresses.
> +     */

IMHO this should be moved before the function.

>      if ( flags & GNTMAP_contains_pte )
>      {
> -        if ( !new_addr )
> -            return destroy_grant_pte_mapping(addr, frame, grant_pte_flags,
> -                                             currd);
> +        /* Replace not available in this addressing mode. */
> +        if ( new_addr )
> +            goto out;
> +

   /*
    * addr comes from Xen's active_entry tracking so isn't guest controlled,
    * but it had still better be PTE-aligned.
    */

Consider keeping this comment?

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/7] x86/mm: Improvements to PV l1e mapping helpers
  2017-09-12 12:29   ` Wei Liu
@ 2017-09-12 15:22     ` Jan Beulich
  0 siblings, 0 replies; 27+ messages in thread
From: Jan Beulich @ 2017-09-12 15:22 UTC (permalink / raw)
  To: Andrew Cooper, WeiLiu; +Cc: Xen-devel

>>> On 12.09.17 at 14:29, <wei.liu2@citrix.com> wrote:
> On Tue, Sep 12, 2017 at 01:14:40PM +0100, Andrew Cooper wrote:
>> Drop guest_unmap_l1e() and use unmap_domain_page() directly.  This will
>> simplify future cleanup.  Rename guest_map_l1e() to map_guest_l1e() to 
> closer
>> match the mapping nomenclature.
>> 
>> Switch map_guest_l1e() to using mfn_t.  Correct the comment to indicate that
>> it takes a linear address (not a virtual address), and correct the parameter
>> name.
>> 
>> No functional change.
>> 
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 2/7] x86/mm: Factor out the grant flags to pte flags conversion logic
  2017-09-12 13:28   ` Wei Liu
@ 2017-09-12 15:25     ` Jan Beulich
  0 siblings, 0 replies; 27+ messages in thread
From: Jan Beulich @ 2017-09-12 15:25 UTC (permalink / raw)
  To: Andrew Cooper, WeiLiu; +Cc: Xen-devel

>>> On 12.09.17 at 15:28, <wei.liu2@citrix.com> wrote:
> On Tue, Sep 12, 2017 at 01:14:41PM +0100, Andrew Cooper wrote:
>> This fixes a bug where the requested AVAIL* flags were not honoured in an
>> unmap_and_replace operation.
>> 
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 3/7] x86/mm: Misc cleanup to {create, replace}_grant_host_mapping()
  2017-09-12 13:40   ` Wei Liu
@ 2017-09-12 15:25     ` Jan Beulich
  0 siblings, 0 replies; 27+ messages in thread
From: Jan Beulich @ 2017-09-12 15:25 UTC (permalink / raw)
  To: Andrew Cooper, WeiLiu; +Cc: Xen-devel

>>> On 12.09.17 at 15:40, <wei.liu2@citrix.com> wrote:
> On Tue, Sep 12, 2017 at 01:14:42PM +0100, Andrew Cooper wrote:
>> The purpose of this patch is solely to simplify the resulting diff of later
>> changes.
>> 
>>  * Factor out curr and currd at the start of the functions.
>>  * Rename pte to nl1e.
>> 
>> No functional change.
>> 
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 4/7] x86/mm: Combine create_grant_{pte, va}_mapping()
  2017-09-12 12:14 ` [PATCH 4/7] x86/mm: Combine create_grant_{pte, va}_mapping() Andrew Cooper
  2017-09-12 14:04   ` Wei Liu
@ 2017-09-12 15:31   ` Jan Beulich
  1 sibling, 0 replies; 27+ messages in thread
From: Jan Beulich @ 2017-09-12 15:31 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Wei Liu, Xen-devel

>>> On 12.09.17 at 14:14, <andrew.cooper3@citrix.com> wrote:
> create_grant_{pte,va}_mapping() are nearly identical; all that is really
> different between them is how they convert their addr parameter to the pte to
> install the grant into.
> 
> Reimplement their logic in create_grant_pv_mapping() in a mostly common way.
> 
> No (intended) change in behaviour from a guests point of view.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 5/7] x86/mm: Carve steal_linear_address() out of replace_grant_host_mapping()
  2017-09-12 14:19   ` Wei Liu
@ 2017-09-12 15:41     ` Jan Beulich
  0 siblings, 0 replies; 27+ messages in thread
From: Jan Beulich @ 2017-09-12 15:41 UTC (permalink / raw)
  To: Andrew Cooper, WeiLiu; +Cc: Xen-devel

>>> On 12.09.17 at 16:19, <wei.liu2@citrix.com> wrote:
> On Tue, Sep 12, 2017 at 01:14:44PM +0100, Andrew Cooper wrote:
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -4075,14 +4075,68 @@ int create_grant_pv_mapping(uint64_t addr, unsigned long frame,
>>      return rc;
>>  }
>>  
>> -int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
>> -                             uint64_t new_addr, unsigned int flags)
>> +/*
>> + * This exists soley for implementing GNTABOP_unmap_and_replace, the ABI of
>> + * which is bizare.  This GNTTABOP isn't used any more, but was used by
> 
> Should be "bizarre".
> 
>> + * classic-xen kernels and PVOps Linux before the M2P_OVERRIDE infrastructure
>> + * was replaced with something which actually worked.
>> + *
>> + * Look up the L1e mapping linear, and zap it.  Return the L1e via *out.
>> + * Returns a boolean indicating success.  If success, the caller is
>> + * responsible for calling put_page_from_l1e().
>> + */
> 
> Can't say much about the history; code-wise:
> 
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 6/7] x86/mm: Combine {destroy, replace}_grant_{pte, va}_mapping()
  2017-09-12 12:14 ` [PATCH 6/7] x86/mm: Combine {destroy, replace}_grant_{pte, va}_mapping() Andrew Cooper
  2017-09-12 14:58   ` Wei Liu
@ 2017-09-12 15:46   ` Jan Beulich
  1 sibling, 0 replies; 27+ messages in thread
From: Jan Beulich @ 2017-09-12 15:46 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Wei Liu, Xen-devel

>>> On 12.09.17 at 14:14, <andrew.cooper3@citrix.com> wrote:
> @@ -4136,12 +3959,14 @@ int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
>  {
>      struct vcpu *curr = current;
>      struct domain *currd = curr->domain;
> -    l1_pgentry_t ol1e;
> -    int rc;
> +    l1_pgentry_t nl1e = l1e_empty(), ol1e, *pl1e;
> +    struct page_info *page;
> +    mfn_t gl1mfn;
> +    int rc = GNTST_general_error;
>      unsigned int grant_pte_flags = grant_to_pte_flags(flags, 0);
>  
>      /*
> -     * On top of the explicit settings done by create_grant_host_mapping()
> +     * On top of the explicit settings done by create_pv_host_mapping()

create_grant_pv_mapping()

Other than that
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 7/7] x86/mm: Prevent 32bit PV guests using out-of-range linear addresses
  2017-09-12 12:14 ` [PATCH 7/7] x86/mm: Prevent 32bit PV guests using out-of-range linear addresses Andrew Cooper
@ 2017-09-12 15:50   ` Jan Beulich
  2017-09-12 16:04     ` Andrew Cooper
  0 siblings, 1 reply; 27+ messages in thread
From: Jan Beulich @ 2017-09-12 15:50 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Xen-devel

>>> On 12.09.17 at 14:14, <andrew.cooper3@citrix.com> wrote:
> The grant ABI uses 64 bit values, and allows a PV guest to specify linear
> addresses.  There is nothing interesting a 32bit PV guest can reference which
> will pass an __addr_ok() check, but it should still get an error for trying.

While I'm all for tightening checks, I'm not sure we reasonably can:
Existing guests may (perhaps inadvertently) rely on this behavior,
and hence may break with the change. I think a prereq to this is to
have a command line option (or even a per-guest one) to control
strict vs relaxed argument checking behavior, and tie the extra
checks to that option being true.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 7/7] x86/mm: Prevent 32bit PV guests using out-of-range linear addresses
  2017-09-12 15:50   ` Jan Beulich
@ 2017-09-12 16:04     ` Andrew Cooper
  2017-09-13  8:28       ` Jan Beulich
  0 siblings, 1 reply; 27+ messages in thread
From: Andrew Cooper @ 2017-09-12 16:04 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Xen-devel

On 12/09/17 16:50, Jan Beulich wrote:
>>>> On 12.09.17 at 14:14, <andrew.cooper3@citrix.com> wrote:
>> The grant ABI uses 64 bit values, and allows a PV guest to specify linear
>> addresses.  There is nothing interesting a 32bit PV guest can reference which
>> will pass an __addr_ok() check, but it should still get an error for trying.
> While I'm all for tightening checks, I'm not sure we reasonably can:
> Existing guests may (perhaps inadvertently) rely on this behavior,
> and hence may break with the change. I think a prereq to this is to
> have a command line option (or even a per-guest one) to control
> strict vs relaxed argument checking behavior, and tie the extra
> checks to that option being true.

At the moment, any attempt to use this behaviour will still cause a
general error, because we cant locate an L1e mapping the out-of-range
linear address.  Therefore, the guest wouldn't have had the grant
operation succeed before.

The problem is that its a latent security bug if we ever chose to reuse
these ranges for other purposes.

E.g. One idea I've had for a while is to move the XLAT translation logic
into guest mode, accessed via a modification to the hypercall page. 
This would mitigate security issues such as infinite loops or boundary
overflows, both of which we've had in the XLAT logic in the past.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 6/7] x86/mm: Combine {destroy, replace}_grant_{pte, va}_mapping()
  2017-09-12 14:58   ` Wei Liu
@ 2017-09-12 16:30     ` Andrew Cooper
  2017-09-12 16:32       ` Wei Liu
  0 siblings, 1 reply; 27+ messages in thread
From: Andrew Cooper @ 2017-09-12 16:30 UTC (permalink / raw)
  To: Wei Liu; +Cc: Jan Beulich, Xen-devel

On 12/09/17 15:58, Wei Liu wrote:
> On Tue, Sep 12, 2017 at 01:14:45PM +0100, Andrew Cooper wrote:
>> As with the create side of things, these are largely identical.  Most cases
>> are actually destroying the mapping rather than replacing it with a stolen
>> entry.
>>
>> Reimplement their logic in replace_grant_pv_mapping() in a mostly common
>> way.
>>
>> No (intended) change in behaviour from a guests point of view.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>
>
> With two suggestions:
>
>>  int create_grant_pv_mapping(uint64_t addr, unsigned long frame,
>>                              unsigned int flags, unsigned int cache_flags)
>>  {
>> @@ -4136,12 +3959,14 @@ int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
>>  {
>>      struct vcpu *curr = current;
>>      struct domain *currd = curr->domain;
>> -    l1_pgentry_t ol1e;
>> -    int rc;
>> +    l1_pgentry_t nl1e = l1e_empty(), ol1e, *pl1e;
>> +    struct page_info *page;
>> +    mfn_t gl1mfn;
>> +    int rc = GNTST_general_error;
>>      unsigned int grant_pte_flags = grant_to_pte_flags(flags, 0);
>>  
>>      /*
>> -     * On top of the explicit settings done by create_grant_host_mapping()
>> +     * On top of the explicit settings done by create_pv_host_mapping()
>>       * also open-code relevant parts of adjust_guest_l1e(). Don't mirror
>>       * available and cachability flags, though.
>>       */
>> @@ -4150,24 +3975,96 @@ int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
>>                             ? _PAGE_GLOBAL
>>                             : _PAGE_GUEST_KERNEL | _PAGE_USER;
>>  
>> +    /*
>> +     * addr comes from Xen's active_entry tracking, and was used successfully
>> +     * to create a grant.
>> +     *
>> +     * The meaning of addr depends on GNTMAP_contains_pte.  It is either a
>> +     * machine address of an L1e the guest has nominated to be altered, or a
>> +     * linear address we need to look up the appropriate L1e for.
>> +     *
>> +     * Passing a new_addr of zero is taken to mean destroy.  Passing a
>> +     * non-zero new_addr has only ever been available via
>> +     * GNTABOP_unmap_and_replace and only when using linear addresses.
>> +     */
> IMHO this should be moved before the function.

Which bit?  The addr and GNTMAP_contains_pte need to be here to explain
the curious if statement below.

The final paragraph only makes sense in the context of the middle paragraph.

>
>>      if ( flags & GNTMAP_contains_pte )
>>      {
>> -        if ( !new_addr )
>> -            return destroy_grant_pte_mapping(addr, frame, grant_pte_flags,
>> -                                             currd);
>> +        /* Replace not available in this addressing mode. */
>> +        if ( new_addr )
>> +            goto out;
>> +
>    /*
>     * addr comes from Xen's active_entry tracking so isn't guest controlled,
>     * but it had still better be PTE-aligned.
>     */
>
> Consider keeping this comment?

Is it really that helpful?  It is in the context of "addr comes from
Xen's active_entry tracking, and was used successfully to create the grant".

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 6/7] x86/mm: Combine {destroy, replace}_grant_{pte, va}_mapping()
  2017-09-12 16:30     ` Andrew Cooper
@ 2017-09-12 16:32       ` Wei Liu
  2017-09-12 16:36         ` Andrew Cooper
  0 siblings, 1 reply; 27+ messages in thread
From: Wei Liu @ 2017-09-12 16:32 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Wei Liu, Jan Beulich, Xen-devel

On Tue, Sep 12, 2017 at 05:30:11PM +0100, Andrew Cooper wrote:
> On 12/09/17 15:58, Wei Liu wrote:
> > On Tue, Sep 12, 2017 at 01:14:45PM +0100, Andrew Cooper wrote:
> >> As with the create side of things, these are largely identical.  Most cases
> >> are actually destroying the mapping rather than replacing it with a stolen
> >> entry.
> >>
> >> Reimplement their logic in replace_grant_pv_mapping() in a mostly common
> >> way.
> >>
> >> No (intended) change in behaviour from a guests point of view.
> >>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > Reviewed-by: Wei Liu <wei.liu2@citrix.com>
> >
> > With two suggestions:
> >
> >>  int create_grant_pv_mapping(uint64_t addr, unsigned long frame,
> >>                              unsigned int flags, unsigned int cache_flags)
> >>  {
> >> @@ -4136,12 +3959,14 @@ int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
> >>  {
> >>      struct vcpu *curr = current;
> >>      struct domain *currd = curr->domain;
> >> -    l1_pgentry_t ol1e;
> >> -    int rc;
> >> +    l1_pgentry_t nl1e = l1e_empty(), ol1e, *pl1e;
> >> +    struct page_info *page;
> >> +    mfn_t gl1mfn;
> >> +    int rc = GNTST_general_error;
> >>      unsigned int grant_pte_flags = grant_to_pte_flags(flags, 0);
> >>  
> >>      /*
> >> -     * On top of the explicit settings done by create_grant_host_mapping()
> >> +     * On top of the explicit settings done by create_pv_host_mapping()
> >>       * also open-code relevant parts of adjust_guest_l1e(). Don't mirror
> >>       * available and cachability flags, though.
> >>       */
> >> @@ -4150,24 +3975,96 @@ int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
> >>                             ? _PAGE_GLOBAL
> >>                             : _PAGE_GUEST_KERNEL | _PAGE_USER;
> >>  
> >> +    /*
> >> +     * addr comes from Xen's active_entry tracking, and was used successfully
> >> +     * to create a grant.
> >> +     *
> >> +     * The meaning of addr depends on GNTMAP_contains_pte.  It is either a
> >> +     * machine address of an L1e the guest has nominated to be altered, or a
> >> +     * linear address we need to look up the appropriate L1e for.
> >> +     *
> >> +     * Passing a new_addr of zero is taken to mean destroy.  Passing a
> >> +     * non-zero new_addr has only ever been available via
> >> +     * GNTABOP_unmap_and_replace and only when using linear addresses.
> >> +     */
> > IMHO this should be moved before the function.
> 
> Which bit?  The addr and GNTMAP_contains_pte need to be here to explain
> the curious if statement below.
> 
> The final paragraph only makes sense in the context of the middle paragraph.

At least the new_addr == 0 means destroying mapping bit.

> 
> >
> >>      if ( flags & GNTMAP_contains_pte )
> >>      {
> >> -        if ( !new_addr )
> >> -            return destroy_grant_pte_mapping(addr, frame, grant_pte_flags,
> >> -                                             currd);
> >> +        /* Replace not available in this addressing mode. */
> >> +        if ( new_addr )
> >> +            goto out;
> >> +
> >    /*
> >     * addr comes from Xen's active_entry tracking so isn't guest controlled,
> >     * but it had still better be PTE-aligned.
> >     */
> >
> > Consider keeping this comment?
> 
> Is it really that helpful?  It is in the context of "addr comes from
> Xen's active_entry tracking, and was used successfully to create the grant".

OK. I won't insist on this.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 6/7] x86/mm: Combine {destroy, replace}_grant_{pte, va}_mapping()
  2017-09-12 16:32       ` Wei Liu
@ 2017-09-12 16:36         ` Andrew Cooper
  2017-09-12 16:37           ` Wei Liu
  0 siblings, 1 reply; 27+ messages in thread
From: Andrew Cooper @ 2017-09-12 16:36 UTC (permalink / raw)
  To: Wei Liu; +Cc: Jan Beulich, Xen-devel

On 12/09/17 17:32, Wei Liu wrote:
> On Tue, Sep 12, 2017 at 05:30:11PM +0100, Andrew Cooper wrote:
>> On 12/09/17 15:58, Wei Liu wrote:
>>> On Tue, Sep 12, 2017 at 01:14:45PM +0100, Andrew Cooper wrote:
>>>> As with the create side of things, these are largely identical.  Most cases
>>>> are actually destroying the mapping rather than replacing it with a stolen
>>>> entry.
>>>>
>>>> Reimplement their logic in replace_grant_pv_mapping() in a mostly common
>>>> way.
>>>>
>>>> No (intended) change in behaviour from a guests point of view.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Reviewed-by: Wei Liu <wei.liu2@citrix.com>
>>>
>>> With two suggestions:
>>>
>>>>  int create_grant_pv_mapping(uint64_t addr, unsigned long frame,
>>>>                              unsigned int flags, unsigned int cache_flags)
>>>>  {
>>>> @@ -4136,12 +3959,14 @@ int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
>>>>  {
>>>>      struct vcpu *curr = current;
>>>>      struct domain *currd = curr->domain;
>>>> -    l1_pgentry_t ol1e;
>>>> -    int rc;
>>>> +    l1_pgentry_t nl1e = l1e_empty(), ol1e, *pl1e;
>>>> +    struct page_info *page;
>>>> +    mfn_t gl1mfn;
>>>> +    int rc = GNTST_general_error;
>>>>      unsigned int grant_pte_flags = grant_to_pte_flags(flags, 0);
>>>>  
>>>>      /*
>>>> -     * On top of the explicit settings done by create_grant_host_mapping()
>>>> +     * On top of the explicit settings done by create_pv_host_mapping()
>>>>       * also open-code relevant parts of adjust_guest_l1e(). Don't mirror
>>>>       * available and cachability flags, though.
>>>>       */
>>>> @@ -4150,24 +3975,96 @@ int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
>>>>                             ? _PAGE_GLOBAL
>>>>                             : _PAGE_GUEST_KERNEL | _PAGE_USER;
>>>>  
>>>> +    /*
>>>> +     * addr comes from Xen's active_entry tracking, and was used successfully
>>>> +     * to create a grant.
>>>> +     *
>>>> +     * The meaning of addr depends on GNTMAP_contains_pte.  It is either a
>>>> +     * machine address of an L1e the guest has nominated to be altered, or a
>>>> +     * linear address we need to look up the appropriate L1e for.
>>>> +     *
>>>> +     * Passing a new_addr of zero is taken to mean destroy.  Passing a
>>>> +     * non-zero new_addr has only ever been available via
>>>> +     * GNTABOP_unmap_and_replace and only when using linear addresses.
>>>> +     */
>>> IMHO this should be moved before the function.
>> Which bit?  The addr and GNTMAP_contains_pte need to be here to explain
>> the curious if statement below.
>>
>> The final paragraph only makes sense in the context of the middle paragraph.
> At least the new_addr == 0 means destroying mapping bit.

I've folded the following incremental delta.

~Andrew

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index f05a1d7..202eee2 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -3958,6 +3958,11 @@ static bool steal_linear_address(unsigned long
linear, l1_pgentry_t *out)
     return okay;
 }
 
+/*
+ * Passing a new_addr of zero is taken to mean destroy.  Passing a non-zero
+ * new_addr has only ever been available via GNTABOP_unmap_and_replace, and
+ * only when !(flags & GNTMAP_contains_pte).
+ */
 int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
                              uint64_t new_addr, unsigned int flags)
 {
@@ -3986,10 +3991,6 @@ int replace_grant_pv_mapping(uint64_t addr,
unsigned long frame,
      * The meaning of addr depends on GNTMAP_contains_pte.  It is either a
      * machine address of an L1e the guest has nominated to be altered,
or a
      * linear address we need to look up the appropriate L1e for.
-     *
-     * Passing a new_addr of zero is taken to mean destroy.  Passing a
-     * non-zero new_addr has only ever been available via
-     * GNTABOP_unmap_and_replace and only when using linear addresses.
      */
     if ( flags & GNTMAP_contains_pte )
     {
@@ -3997,6 +3998,7 @@ int replace_grant_pv_mapping(uint64_t addr,
unsigned long frame,
         if ( new_addr )
             goto out;
 
+        /* Sanity check that we won't clobber the pagetable. */
         if ( !IS_ALIGNED(addr, sizeof(nl1e)) )
         {
             ASSERT_UNREACHABLE();


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* Re: [PATCH 6/7] x86/mm: Combine {destroy, replace}_grant_{pte, va}_mapping()
  2017-09-12 16:36         ` Andrew Cooper
@ 2017-09-12 16:37           ` Wei Liu
  0 siblings, 0 replies; 27+ messages in thread
From: Wei Liu @ 2017-09-12 16:37 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Wei Liu, Jan Beulich, Xen-devel

On Tue, Sep 12, 2017 at 05:36:32PM +0100, Andrew Cooper wrote:
> On 12/09/17 17:32, Wei Liu wrote:
> > On Tue, Sep 12, 2017 at 05:30:11PM +0100, Andrew Cooper wrote:
> >> On 12/09/17 15:58, Wei Liu wrote:
> >>> On Tue, Sep 12, 2017 at 01:14:45PM +0100, Andrew Cooper wrote:
> >>>> As with the create side of things, these are largely identical.  Most cases
> >>>> are actually destroying the mapping rather than replacing it with a stolen
> >>>> entry.
> >>>>
> >>>> Reimplement their logic in replace_grant_pv_mapping() in a mostly common
> >>>> way.
> >>>>
> >>>> No (intended) change in behaviour from a guests point of view.
> >>>>
> >>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >>> Reviewed-by: Wei Liu <wei.liu2@citrix.com>
> >>>
> >>> With two suggestions:
> >>>
> >>>>  int create_grant_pv_mapping(uint64_t addr, unsigned long frame,
> >>>>                              unsigned int flags, unsigned int cache_flags)
> >>>>  {
> >>>> @@ -4136,12 +3959,14 @@ int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
> >>>>  {
> >>>>      struct vcpu *curr = current;
> >>>>      struct domain *currd = curr->domain;
> >>>> -    l1_pgentry_t ol1e;
> >>>> -    int rc;
> >>>> +    l1_pgentry_t nl1e = l1e_empty(), ol1e, *pl1e;
> >>>> +    struct page_info *page;
> >>>> +    mfn_t gl1mfn;
> >>>> +    int rc = GNTST_general_error;
> >>>>      unsigned int grant_pte_flags = grant_to_pte_flags(flags, 0);
> >>>>  
> >>>>      /*
> >>>> -     * On top of the explicit settings done by create_grant_host_mapping()
> >>>> +     * On top of the explicit settings done by create_pv_host_mapping()
> >>>>       * also open-code relevant parts of adjust_guest_l1e(). Don't mirror
> >>>>       * available and cachability flags, though.
> >>>>       */
> >>>> @@ -4150,24 +3975,96 @@ int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
> >>>>                             ? _PAGE_GLOBAL
> >>>>                             : _PAGE_GUEST_KERNEL | _PAGE_USER;
> >>>>  
> >>>> +    /*
> >>>> +     * addr comes from Xen's active_entry tracking, and was used successfully
> >>>> +     * to create a grant.
> >>>> +     *
> >>>> +     * The meaning of addr depends on GNTMAP_contains_pte.  It is either a
> >>>> +     * machine address of an L1e the guest has nominated to be altered, or a
> >>>> +     * linear address we need to look up the appropriate L1e for.
> >>>> +     *
> >>>> +     * Passing a new_addr of zero is taken to mean destroy.  Passing a
> >>>> +     * non-zero new_addr has only ever been available via
> >>>> +     * GNTABOP_unmap_and_replace and only when using linear addresses.
> >>>> +     */
> >>> IMHO this should be moved before the function.
> >> Which bit?  The addr and GNTMAP_contains_pte need to be here to explain
> >> the curious if statement below.
> >>
> >> The final paragraph only makes sense in the context of the middle paragraph.
> > At least the new_addr == 0 means destroying mapping bit.
> 
> I've folded the following incremental delta.
> 
> ~Andrew
> 
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index f05a1d7..202eee2 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -3958,6 +3958,11 @@ static bool steal_linear_address(unsigned long
> linear, l1_pgentry_t *out)
>      return okay;
>  }
>  
> +/*
> + * Passing a new_addr of zero is taken to mean destroy.  Passing a non-zero
> + * new_addr has only ever been available via GNTABOP_unmap_and_replace, and
> + * only when !(flags & GNTMAP_contains_pte).
> + */
>  int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
>                               uint64_t new_addr, unsigned int flags)
>  {
> @@ -3986,10 +3991,6 @@ int replace_grant_pv_mapping(uint64_t addr,
> unsigned long frame,
>       * The meaning of addr depends on GNTMAP_contains_pte.  It is either a
>       * machine address of an L1e the guest has nominated to be altered,
> or a
>       * linear address we need to look up the appropriate L1e for.
> -     *
> -     * Passing a new_addr of zero is taken to mean destroy.  Passing a
> -     * non-zero new_addr has only ever been available via
> -     * GNTABOP_unmap_and_replace and only when using linear addresses.
>       */
>      if ( flags & GNTMAP_contains_pte )
>      {
> @@ -3997,6 +3998,7 @@ int replace_grant_pv_mapping(uint64_t addr,
> unsigned long frame,
>          if ( new_addr )
>              goto out;
>  
> +        /* Sanity check that we won't clobber the pagetable. */
>          if ( !IS_ALIGNED(addr, sizeof(nl1e)) )
>          {
>              ASSERT_UNREACHABLE();
> 

LGTM. Thanks.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 7/7] x86/mm: Prevent 32bit PV guests using out-of-range linear addresses
  2017-09-12 16:04     ` Andrew Cooper
@ 2017-09-13  8:28       ` Jan Beulich
  0 siblings, 0 replies; 27+ messages in thread
From: Jan Beulich @ 2017-09-13  8:28 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Xen-devel

>>> On 12.09.17 at 18:04, <andrew.cooper3@citrix.com> wrote:
> On 12/09/17 16:50, Jan Beulich wrote:
>>>>> On 12.09.17 at 14:14, <andrew.cooper3@citrix.com> wrote:
>>> The grant ABI uses 64 bit values, and allows a PV guest to specify linear
>>> addresses.  There is nothing interesting a 32bit PV guest can reference which
>>> will pass an __addr_ok() check, but it should still get an error for trying.
>> While I'm all for tightening checks, I'm not sure we reasonably can:
>> Existing guests may (perhaps inadvertently) rely on this behavior,
>> and hence may break with the change. I think a prereq to this is to
>> have a command line option (or even a per-guest one) to control
>> strict vs relaxed argument checking behavior, and tie the extra
>> checks to that option being true.
> 
> At the moment, any attempt to use this behaviour will still cause a
> general error, because we cant locate an L1e mapping the out-of-range
> linear address.  Therefore, the guest wouldn't have had the grant
> operation succeed before.

Oh, good point.

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2017-09-13  8:28 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-12 12:14 [PATCH 0/7] x86/mm: Post XSA-234 cleanup Andrew Cooper
2017-09-12 12:14 ` [PATCH 1/7] x86/mm: Improvements to PV l1e mapping helpers Andrew Cooper
2017-09-12 12:29   ` Wei Liu
2017-09-12 15:22     ` Jan Beulich
2017-09-12 12:14 ` [PATCH 2/7] x86/mm: Factor out the grant flags to pte flags conversion logic Andrew Cooper
2017-09-12 13:28   ` Wei Liu
2017-09-12 15:25     ` Jan Beulich
2017-09-12 12:14 ` [PATCH 3/7] x86/mm: Misc cleanup to {create, replace}_grant_host_mapping() Andrew Cooper
2017-09-12 13:40   ` Wei Liu
2017-09-12 15:25     ` Jan Beulich
2017-09-12 12:14 ` [PATCH 4/7] x86/mm: Combine create_grant_{pte, va}_mapping() Andrew Cooper
2017-09-12 14:04   ` Wei Liu
2017-09-12 15:31   ` Jan Beulich
2017-09-12 12:14 ` [PATCH 5/7] x86/mm: Carve steal_linear_address() out of replace_grant_host_mapping() Andrew Cooper
2017-09-12 14:19   ` Wei Liu
2017-09-12 15:41     ` Jan Beulich
2017-09-12 12:14 ` [PATCH 6/7] x86/mm: Combine {destroy, replace}_grant_{pte, va}_mapping() Andrew Cooper
2017-09-12 14:58   ` Wei Liu
2017-09-12 16:30     ` Andrew Cooper
2017-09-12 16:32       ` Wei Liu
2017-09-12 16:36         ` Andrew Cooper
2017-09-12 16:37           ` Wei Liu
2017-09-12 15:46   ` Jan Beulich
2017-09-12 12:14 ` [PATCH 7/7] x86/mm: Prevent 32bit PV guests using out-of-range linear addresses Andrew Cooper
2017-09-12 15:50   ` Jan Beulich
2017-09-12 16:04     ` Andrew Cooper
2017-09-13  8:28       ` Jan Beulich

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.