* [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* 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 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
* [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* 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 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
* [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* 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 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
* [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* 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 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
* [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* 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 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
* [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* 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 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 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
* [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 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 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