* [PATCH v1 2/6] x86: Temporary disable SMAP to legally access user pages in kernel mode
@ 2014-04-15 13:01 Feng Wu
2014-04-15 6:08 ` Tian, Kevin
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Feng Wu @ 2014-04-15 13:01 UTC (permalink / raw)
To: JBeulich, Ian.Campbell, xen-devel; +Cc: Feng Wu, eddie.dong, jun.nakajima
Use STAC/CLAC to temporary disable SMAP to allow legal accesses to
user pages in kernel mode
Signed-off-by: Feng Wu <feng.wu@intel.com>
---
xen/arch/x86/clear_page.S | 3 +++
xen/arch/x86/domain_build.c | 16 ++++++++++++++++
xen/arch/x86/usercopy.c | 6 ++++++
xen/arch/x86/x86_64/compat/entry.S | 2 ++
xen/arch/x86/x86_64/entry.S | 4 ++++
xen/include/asm-x86/uaccess.h | 4 ++++
xen/include/asm-x86/x86_64/system.h | 2 ++
7 files changed, 37 insertions(+)
diff --git a/xen/arch/x86/clear_page.S b/xen/arch/x86/clear_page.S
index 96315ad..2ef4bb0 100644
--- a/xen/arch/x86/clear_page.S
+++ b/xen/arch/x86/clear_page.S
@@ -1,9 +1,11 @@
#include <xen/config.h>
#include <asm/page.h>
+#include <asm/asm_defns.h>
#define ptr_reg %rdi
ENTRY(clear_page_sse2)
+ ASM_STAC
mov $PAGE_SIZE/16, %ecx
xor %eax,%eax
@@ -15,5 +17,6 @@ ENTRY(clear_page_sse2)
lea 16(ptr_reg), ptr_reg
jnz 0b
+ ASM_CLAC
sfence
ret
diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
index 84ce392..1ba138b 100644
--- a/xen/arch/x86/domain_build.c
+++ b/xen/arch/x86/domain_build.c
@@ -789,8 +789,10 @@ int __init construct_dom0(
rc = -1;
goto out;
}
+ stac();
hypercall_page_initialise(
d, (void *)(unsigned long)parms.virt_hypercall);
+ clac();
}
/* Free temporary buffers. */
@@ -799,6 +801,7 @@ int __init construct_dom0(
/* Set up start info area. */
si = (start_info_t *)vstartinfo_start;
clear_page(si);
+ stac();
si->nr_pages = nr_pages;
si->shared_info = virt_to_maddr(d->shared_info);
@@ -813,6 +816,7 @@ int __init construct_dom0(
snprintf(si->magic, sizeof(si->magic), "xen-3.0-x86_%d%s",
elf_64bit(&elf) ? 64 : 32, parms.pae ? "p" : "");
+ clac();
count = d->tot_pages;
l4start = map_domain_page(pagetable_get_pfn(v->arch.guest_table));
l3tab = NULL;
@@ -956,16 +960,20 @@ int __init construct_dom0(
if ( pfn > REVERSE_START && (vinitrd_start || pfn < initrd_pfn) )
mfn = alloc_epfn - (pfn - REVERSE_START);
#endif
+ stac();
if ( !is_pv_32on64_domain(d) )
((unsigned long *)vphysmap_start)[pfn] = mfn;
else
((unsigned int *)vphysmap_start)[pfn] = mfn;
+ clac();
set_gpfn_from_mfn(mfn, pfn);
if (!(pfn & 0xfffff))
process_pending_softirqs();
}
+ stac();
si->first_p2m_pfn = pfn;
si->nr_p2m_frames = d->tot_pages - count;
+ clac();
page_list_for_each ( page, &d->page_list )
{
mfn = page_to_mfn(page);
@@ -976,7 +984,9 @@ int __init construct_dom0(
if ( !page->u.inuse.type_info &&
!get_page_and_type(page, d, PGT_writable_page) )
BUG();
+ stac();
((unsigned long *)vphysmap_start)[pfn] = mfn;
+ clac();
set_gpfn_from_mfn(mfn, pfn);
++pfn;
if (!(pfn & 0xfffff))
@@ -985,7 +995,9 @@ int __init construct_dom0(
}
BUG_ON(pfn != d->tot_pages);
#ifndef NDEBUG
+ stac();
alloc_epfn += PFN_UP(initrd_len) + si->nr_p2m_frames;
+ clac();
#endif
while ( pfn < nr_pages )
{
@@ -997,10 +1009,12 @@ int __init construct_dom0(
#ifndef NDEBUG
#define pfn (nr_pages - 1 - (pfn - (alloc_epfn - alloc_spfn)))
#endif
+ stac();
if ( !is_pv_32on64_domain(d) )
((unsigned long *)vphysmap_start)[pfn] = mfn;
else
((unsigned int *)vphysmap_start)[pfn] = mfn;
+ clac();
set_gpfn_from_mfn(mfn, pfn);
#undef pfn
page++; pfn++;
@@ -1009,6 +1023,7 @@ int __init construct_dom0(
}
}
+ stac();
if ( initrd_len != 0 )
{
si->mod_start = vinitrd_start ?: initrd_pfn;
@@ -1024,6 +1039,7 @@ int __init construct_dom0(
si->console.dom0.info_off = sizeof(struct start_info);
si->console.dom0.info_size = sizeof(struct dom0_vga_console_info);
}
+ clac();
if ( is_pv_32on64_domain(d) )
xlat_start_info(si, XLAT_start_info_console_dom0);
diff --git a/xen/arch/x86/usercopy.c b/xen/arch/x86/usercopy.c
index b79202b..5dd77db 100644
--- a/xen/arch/x86/usercopy.c
+++ b/xen/arch/x86/usercopy.c
@@ -15,6 +15,7 @@ unsigned long __copy_to_user_ll(void __user *to, const void *from, unsigned n)
unsigned long __d0, __d1, __d2, __n = n;
asm volatile (
+ ASM_STAC"\n"
" cmp $"STR(2*BYTES_PER_LONG-1)",%0\n"
" jbe 1f\n"
" mov %1,%0\n"
@@ -30,6 +31,7 @@ unsigned long __copy_to_user_ll(void __user *to, const void *from, unsigned n)
" mov %3,%0\n"
"1: rep movsb\n" /* ...remainder copied as bytes */
"2:\n"
+ ASM_CLAC"\n"
".section .fixup,\"ax\"\n"
"5: add %3,%0\n"
" jmp 2b\n"
@@ -52,6 +54,7 @@ __copy_from_user_ll(void *to, const void __user *from, unsigned n)
unsigned long __d0, __d1, __d2, __n = n;
asm volatile (
+ ASM_STAC"\n"
" cmp $"STR(2*BYTES_PER_LONG-1)",%0\n"
" jbe 1f\n"
" mov %1,%0\n"
@@ -67,6 +70,7 @@ __copy_from_user_ll(void *to, const void __user *from, unsigned n)
" mov %3,%0\n"
"1: rep; movsb\n" /* ...remainder copied as bytes */
"2:\n"
+ ASM_CLAC"\n"
".section .fixup,\"ax\"\n"
"5: add %3,%0\n"
" jmp 6f\n"
@@ -114,10 +118,12 @@ copy_to_user(void __user *to, const void *from, unsigned n)
do { \
long __d0; \
__asm__ __volatile__( \
+ ASM_STAC"\n" \
"0: rep; stosl\n" \
" movl %2,%0\n" \
"1: rep; stosb\n" \
"2:\n" \
+ ASM_CLAC"\n" \
".section .fixup,\"ax\"\n" \
"3: lea 0(%2,%0,4),%0\n" \
" jmp 2b\n" \
diff --git a/xen/arch/x86/x86_64/compat/entry.S b/xen/arch/x86/x86_64/compat/entry.S
index 32b3bcc..b0fabf4 100644
--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -265,6 +265,7 @@ ENTRY(compat_int80_direct_trap)
/* On return only %rbx and %rdx are guaranteed non-clobbered. */
compat_create_bounce_frame:
ASSERT_INTERRUPTS_ENABLED
+ ASM_STAC
mov %fs,%edi
testb $2,UREGS_cs+8(%rsp)
jz 1f
@@ -336,6 +337,7 @@ __UNLIKELY_END(compat_bounce_null_selector)
movl %eax,UREGS_cs+8(%rsp)
movl TRAPBOUNCE_eip(%rdx),%eax
movl %eax,UREGS_rip+8(%rsp)
+ ASM_CLAC
ret
.section .fixup,"ax"
.Lfx13:
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index 3ea4683..63ba795 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -377,6 +377,7 @@ __UNLIKELY_END(create_bounce_frame_bad_sp)
movb TRAPBOUNCE_flags(%rdx),%cl
subq $40,%rsi
movq UREGS_ss+8(%rsp),%rax
+ ASM_STAC
.Lft2: movq %rax,32(%rsi) # SS
movq UREGS_rsp+8(%rsp),%rax
.Lft3: movq %rax,24(%rsi) # RSP
@@ -434,9 +435,11 @@ UNLIKELY_END(bounce_failsafe)
testq %rax,%rax
UNLIKELY_START(z, create_bounce_frame_bad_bounce_ip)
lea UNLIKELY_DISPATCH_LABEL(create_bounce_frame_bad_bounce_ip)(%rip), %rdi
+ ASM_CLAC
jmp asm_domain_crash_synchronous /* Does not return */
__UNLIKELY_END(create_bounce_frame_bad_bounce_ip)
movq %rax,UREGS_rip+8(%rsp)
+ ASM_CLAC
ret
_ASM_EXTABLE(.Lft2, dom_crash_sync_extable)
_ASM_EXTABLE(.Lft3, dom_crash_sync_extable)
@@ -463,6 +466,7 @@ ENTRY(dom_crash_sync_extable)
leal (%rax,%rax,2),%eax
orb %al,UREGS_cs(%rsp)
xorl %edi,%edi
+ ASM_CLAC
jmp asm_domain_crash_synchronous /* Does not return */
/* No special register assumptions. */
diff --git a/xen/include/asm-x86/uaccess.h b/xen/include/asm-x86/uaccess.h
index 88b4ba2..ce1af4a 100644
--- a/xen/include/asm-x86/uaccess.h
+++ b/xen/include/asm-x86/uaccess.h
@@ -147,8 +147,10 @@ struct __large_struct { unsigned long buf[100]; };
*/
#define __put_user_asm(x, addr, err, itype, rtype, ltype, errret) \
__asm__ __volatile__( \
+ ASM_STAC"\n" \
"1: mov"itype" %"rtype"1,%2\n" \
"2:\n" \
+ ASM_CLAC"\n" \
".section .fixup,\"ax\"\n" \
"3: mov %3,%0\n" \
" jmp 2b\n" \
@@ -159,8 +161,10 @@ struct __large_struct { unsigned long buf[100]; };
#define __get_user_asm(x, addr, err, itype, rtype, ltype, errret) \
__asm__ __volatile__( \
+ ASM_STAC"\n" \
"1: mov"itype" %2,%"rtype"1\n" \
"2:\n" \
+ ASM_CLAC"\n" \
".section .fixup,\"ax\"\n" \
"3: mov %3,%0\n" \
" xor"itype" %"rtype"1,%"rtype"1\n" \
diff --git a/xen/include/asm-x86/x86_64/system.h b/xen/include/asm-x86/x86_64/system.h
index 20f038b..b8394b9 100644
--- a/xen/include/asm-x86/x86_64/system.h
+++ b/xen/include/asm-x86/x86_64/system.h
@@ -13,8 +13,10 @@
*/
#define __cmpxchg_user(_p,_o,_n,_isuff,_oppre,_regtype) \
asm volatile ( \
+ ASM_STAC"\n" \
"1: lock; cmpxchg"_isuff" %"_oppre"2,%3\n" \
"2:\n" \
+ ASM_CLAC"\n" \
".section .fixup,\"ax\"\n" \
"3: movl $1,%1\n" \
" jmp 2b\n" \
--
1.8.3.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v1 2/6] x86: Temporary disable SMAP to legally access user pages in kernel mode
2014-04-15 13:01 [PATCH v1 2/6] x86: Temporary disable SMAP to legally access user pages in kernel mode Feng Wu
@ 2014-04-15 6:08 ` Tian, Kevin
2014-04-15 6:20 ` Wu, Feng
2014-04-15 8:46 ` Jan Beulich
2014-04-15 10:21 ` Andrew Cooper
2 siblings, 1 reply; 9+ messages in thread
From: Tian, Kevin @ 2014-04-15 6:08 UTC (permalink / raw)
To: JBeulich@suse.com, Ian.Campbell@citrix.com,
xen-devel@lists.xen.org
Cc: Dong, Eddie, Wu, Feng, Nakajima, Jun
> From: Feng Wu
> Sent: Tuesday, April 15, 2014 9:02 PM
>
> Use STAC/CLAC to temporary disable SMAP to allow legal accesses to
> user pages in kernel mode
>
> Signed-off-by: Feng Wu <feng.wu@intel.com>
> ---
> xen/arch/x86/clear_page.S | 3 +++
> xen/arch/x86/domain_build.c | 16 ++++++++++++++++
> xen/arch/x86/usercopy.c | 6 ++++++
> xen/arch/x86/x86_64/compat/entry.S | 2 ++
> xen/arch/x86/x86_64/entry.S | 4 ++++
> xen/include/asm-x86/uaccess.h | 4 ++++
> xen/include/asm-x86/x86_64/system.h | 2 ++
> 7 files changed, 37 insertions(+)
>
> diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
> index 84ce392..1ba138b 100644
> --- a/xen/arch/x86/domain_build.c
> +++ b/xen/arch/x86/domain_build.c
> @@ -789,8 +789,10 @@ int __init construct_dom0(
> rc = -1;
> goto out;
> }
> + stac();
> hypercall_page_initialise(
> d, (void *)(unsigned long)parms.virt_hypercall);
> + clac();
> }
construct_dom0 happens before dom0 starts execution. It should be fine to
keep AC cleared in the whole phase.
Thanks
Kevin
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 2/6] x86: Temporary disable SMAP to legally access user pages in kernel mode
2014-04-15 6:08 ` Tian, Kevin
@ 2014-04-15 6:20 ` Wu, Feng
2014-04-15 6:35 ` Tian, Kevin
0 siblings, 1 reply; 9+ messages in thread
From: Wu, Feng @ 2014-04-15 6:20 UTC (permalink / raw)
To: Tian, Kevin, JBeulich@suse.com, Ian.Campbell@citrix.com,
xen-devel@lists.xen.org
Cc: Dong, Eddie, Nakajima, Jun
> -----Original Message-----
> From: Tian, Kevin
> Sent: Tuesday, April 15, 2014 2:08 PM
> To: Wu, Feng; JBeulich@suse.com; Ian.Campbell@citrix.com;
> xen-devel@lists.xen.org
> Cc: Wu, Feng; Dong, Eddie; Nakajima, Jun
> Subject: RE: [Xen-devel] [PATCH v1 2/6] x86: Temporary disable SMAP to legally
> access user pages in kernel mode
>
> > From: Feng Wu
> > Sent: Tuesday, April 15, 2014 9:02 PM
> >
> > Use STAC/CLAC to temporary disable SMAP to allow legal accesses to
> > user pages in kernel mode
> >
> > Signed-off-by: Feng Wu <feng.wu@intel.com>
> > ---
> > xen/arch/x86/clear_page.S | 3 +++
> > xen/arch/x86/domain_build.c | 16 ++++++++++++++++
> > xen/arch/x86/usercopy.c | 6 ++++++
> > xen/arch/x86/x86_64/compat/entry.S | 2 ++
> > xen/arch/x86/x86_64/entry.S | 4 ++++
> > xen/include/asm-x86/uaccess.h | 4 ++++
> > xen/include/asm-x86/x86_64/system.h | 2 ++
> > 7 files changed, 37 insertions(+)
> >
> > diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
> > index 84ce392..1ba138b 100644
> > --- a/xen/arch/x86/domain_build.c
> > +++ b/xen/arch/x86/domain_build.c
> > @@ -789,8 +789,10 @@ int __init construct_dom0(
> > rc = -1;
> > goto out;
> > }
> > + stac();
> > hypercall_page_initialise(
> > d, (void *)(unsigned long)parms.virt_hypercall);
> > + clac();
> > }
>
> construct_dom0 happens before dom0 starts execution. It should be fine to
> keep AC cleared in the whole phase.
construct_dom0() accesses user pages in the supervisor mode, so I added the stac()/clac()
one by one, otherwise, Xen hypervisor will receive an SMAP violation.
>
> Thanks
> Kevin
Thanks,
Feng
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 2/6] x86: Temporary disable SMAP to legally access user pages in kernel mode
2014-04-15 6:20 ` Wu, Feng
@ 2014-04-15 6:35 ` Tian, Kevin
2014-04-15 6:53 ` Wu, Feng
0 siblings, 1 reply; 9+ messages in thread
From: Tian, Kevin @ 2014-04-15 6:35 UTC (permalink / raw)
To: Wu, Feng, JBeulich@suse.com, Ian.Campbell@citrix.com,
xen-devel@lists.xen.org
Cc: Dong, Eddie, Nakajima, Jun
> From: Wu, Feng
> Sent: Tuesday, April 15, 2014 2:21 PM
>
> > -----Original Message-----
> > From: Tian, Kevin
> > Sent: Tuesday, April 15, 2014 2:08 PM
> > To: Wu, Feng; JBeulich@suse.com; Ian.Campbell@citrix.com;
> > xen-devel@lists.xen.org
> > Cc: Wu, Feng; Dong, Eddie; Nakajima, Jun
> > Subject: RE: [Xen-devel] [PATCH v1 2/6] x86: Temporary disable SMAP to
> legally
> > access user pages in kernel mode
> >
> > > From: Feng Wu
> > > Sent: Tuesday, April 15, 2014 9:02 PM
> > >
> > > Use STAC/CLAC to temporary disable SMAP to allow legal accesses to
> > > user pages in kernel mode
> > >
> > > Signed-off-by: Feng Wu <feng.wu@intel.com>
> > > ---
> > > xen/arch/x86/clear_page.S | 3 +++
> > > xen/arch/x86/domain_build.c | 16 ++++++++++++++++
> > > xen/arch/x86/usercopy.c | 6 ++++++
> > > xen/arch/x86/x86_64/compat/entry.S | 2 ++
> > > xen/arch/x86/x86_64/entry.S | 4 ++++
> > > xen/include/asm-x86/uaccess.h | 4 ++++
> > > xen/include/asm-x86/x86_64/system.h | 2 ++
> > > 7 files changed, 37 insertions(+)
> > >
> > > diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
> > > index 84ce392..1ba138b 100644
> > > --- a/xen/arch/x86/domain_build.c
> > > +++ b/xen/arch/x86/domain_build.c
> > > @@ -789,8 +789,10 @@ int __init construct_dom0(
> > > rc = -1;
> > > goto out;
> > > }
> > > + stac();
> > > hypercall_page_initialise(
> > > d, (void *)(unsigned long)parms.virt_hypercall);
> > > + clac();
> > > }
> >
> > construct_dom0 happens before dom0 starts execution. It should be fine to
> > keep AC cleared in the whole phase.
>
> construct_dom0() accesses user pages in the supervisor mode, so I added the
> stac()/clac()
> one by one, otherwise, Xen hypervisor will receive an SMAP violation.
>
I meant you can do once to enable user page accesses, and then disable it
at end of construct_dom0. There should be no security concern at this stage.
Thanks
Kevin
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 2/6] x86: Temporary disable SMAP to legally access user pages in kernel mode
2014-04-15 6:35 ` Tian, Kevin
@ 2014-04-15 6:53 ` Wu, Feng
0 siblings, 0 replies; 9+ messages in thread
From: Wu, Feng @ 2014-04-15 6:53 UTC (permalink / raw)
To: Tian, Kevin, JBeulich@suse.com, Ian.Campbell@citrix.com,
xen-devel@lists.xen.org
Cc: Dong, Eddie, Nakajima, Jun
> -----Original Message-----
> From: Tian, Kevin
> Sent: Tuesday, April 15, 2014 2:36 PM
> To: Wu, Feng; JBeulich@suse.com; Ian.Campbell@citrix.com;
> xen-devel@lists.xen.org
> Cc: Dong, Eddie; Nakajima, Jun
> Subject: RE: [Xen-devel] [PATCH v1 2/6] x86: Temporary disable SMAP to legally
> access user pages in kernel mode
>
> > From: Wu, Feng
> > Sent: Tuesday, April 15, 2014 2:21 PM
> >
> > > -----Original Message-----
> > > From: Tian, Kevin
> > > Sent: Tuesday, April 15, 2014 2:08 PM
> > > To: Wu, Feng; JBeulich@suse.com; Ian.Campbell@citrix.com;
> > > xen-devel@lists.xen.org
> > > Cc: Wu, Feng; Dong, Eddie; Nakajima, Jun
> > > Subject: RE: [Xen-devel] [PATCH v1 2/6] x86: Temporary disable SMAP to
> > legally
> > > access user pages in kernel mode
> > >
> > > > From: Feng Wu
> > > > Sent: Tuesday, April 15, 2014 9:02 PM
> > > >
> > > > Use STAC/CLAC to temporary disable SMAP to allow legal accesses to
> > > > user pages in kernel mode
> > > >
> > > > Signed-off-by: Feng Wu <feng.wu@intel.com>
> > > > ---
> > > > xen/arch/x86/clear_page.S | 3 +++
> > > > xen/arch/x86/domain_build.c | 16 ++++++++++++++++
> > > > xen/arch/x86/usercopy.c | 6 ++++++
> > > > xen/arch/x86/x86_64/compat/entry.S | 2 ++
> > > > xen/arch/x86/x86_64/entry.S | 4 ++++
> > > > xen/include/asm-x86/uaccess.h | 4 ++++
> > > > xen/include/asm-x86/x86_64/system.h | 2 ++
> > > > 7 files changed, 37 insertions(+)
> > > >
> > > > diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
> > > > index 84ce392..1ba138b 100644
> > > > --- a/xen/arch/x86/domain_build.c
> > > > +++ b/xen/arch/x86/domain_build.c
> > > > @@ -789,8 +789,10 @@ int __init construct_dom0(
> > > > rc = -1;
> > > > goto out;
> > > > }
> > > > + stac();
> > > > hypercall_page_initialise(
> > > > d, (void *)(unsigned long)parms.virt_hypercall);
> > > > + clac();
> > > > }
> > >
> > > construct_dom0 happens before dom0 starts execution. It should be fine to
> > > keep AC cleared in the whole phase.
> >
> > construct_dom0() accesses user pages in the supervisor mode, so I added the
> > stac()/clac()
> > one by one, otherwise, Xen hypervisor will receive an SMAP violation.
> >
>
> I meant you can do once to enable user page accesses, and then disable it
> at end of construct_dom0. There should be no security concern at this stage.
Yes, that is a clean solution. I also thought about it before, but I was not sure whether
there was security issues with it. As you said, if there is no security concern, your
suggestion is better for sure.
>
> Thanks
> Kevin
Thanks,
Feng
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 2/6] x86: Temporary disable SMAP to legally access user pages in kernel mode
2014-04-15 13:01 [PATCH v1 2/6] x86: Temporary disable SMAP to legally access user pages in kernel mode Feng Wu
2014-04-15 6:08 ` Tian, Kevin
@ 2014-04-15 8:46 ` Jan Beulich
2014-04-16 6:14 ` Wu, Feng
2014-04-15 10:21 ` Andrew Cooper
2 siblings, 1 reply; 9+ messages in thread
From: Jan Beulich @ 2014-04-15 8:46 UTC (permalink / raw)
To: Feng Wu; +Cc: eddie.dong, Ian.Campbell, jun.nakajima, xen-devel
>>> On 15.04.14 at 15:01, <feng.wu@intel.com> wrote:
> --- a/xen/arch/x86/clear_page.S
> +++ b/xen/arch/x86/clear_page.S
> @@ -1,9 +1,11 @@
> #include <xen/config.h>
> #include <asm/page.h>
> +#include <asm/asm_defns.h>
>
> #define ptr_reg %rdi
>
> ENTRY(clear_page_sse2)
> + ASM_STAC
> mov $PAGE_SIZE/16, %ecx
> xor %eax,%eax
>
> @@ -15,5 +17,6 @@ ENTRY(clear_page_sse2)
> lea 16(ptr_reg), ptr_reg
> jnz 0b
>
> + ASM_CLAC
> sfence
> ret
Wrong code being modified - this isn't used to clear guest memory (or
else we have a security problem). If there are pages having the U bit
set, then I guess you need to first go and clean those up.
> --- a/xen/arch/x86/x86_64/compat/entry.S
> +++ b/xen/arch/x86/x86_64/compat/entry.S
> @@ -265,6 +265,7 @@ ENTRY(compat_int80_direct_trap)
> /* On return only %rbx and %rdx are guaranteed non-clobbered. */
> compat_create_bounce_frame:
> ASSERT_INTERRUPTS_ENABLED
> + ASM_STAC
> mov %fs,%edi
> testb $2,UREGS_cs+8(%rsp)
> jz 1f
> @@ -336,6 +337,7 @@ __UNLIKELY_END(compat_bounce_null_selector)
> movl %eax,UREGS_cs+8(%rsp)
> movl TRAPBOUNCE_eip(%rdx),%eax
> movl %eax,UREGS_rip+8(%rsp)
> + ASM_CLAC
> ret
These and the similar changes to xen/arch/x86/x86_64/entry.S seem
pretty pointless without also adding ASM_CLAC to the exception and
interrupt entry points.
Jan
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 2/6] x86: Temporary disable SMAP to legally access user pages in kernel mode
2014-04-15 8:46 ` Jan Beulich
@ 2014-04-16 6:14 ` Wu, Feng
0 siblings, 0 replies; 9+ messages in thread
From: Wu, Feng @ 2014-04-16 6:14 UTC (permalink / raw)
To: Jan Beulich
Cc: Dong, Eddie, Ian.Campbell@citrix.com, Nakajima, Jun,
xen-devel@lists.xen.org
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Tuesday, April 15, 2014 4:47 PM
> To: Wu, Feng
> Cc: Ian.Campbell@citrix.com; Dong, Eddie; Nakajima, Jun;
> xen-devel@lists.xen.org
> Subject: Re: [PATCH v1 2/6] x86: Temporary disable SMAP to legally access user
> pages in kernel mode
>
> >>> On 15.04.14 at 15:01, <feng.wu@intel.com> wrote:
> > --- a/xen/arch/x86/clear_page.S
> > +++ b/xen/arch/x86/clear_page.S
> > @@ -1,9 +1,11 @@
> > #include <xen/config.h>
> > #include <asm/page.h>
> > +#include <asm/asm_defns.h>
> >
> > #define ptr_reg %rdi
> >
> > ENTRY(clear_page_sse2)
> > + ASM_STAC
> > mov $PAGE_SIZE/16, %ecx
> > xor %eax,%eax
> >
> > @@ -15,5 +17,6 @@ ENTRY(clear_page_sse2)
> > lea 16(ptr_reg), ptr_reg
> > jnz 0b
> >
> > + ASM_CLAC
> > sfence
> > ret
>
> Wrong code being modified - this isn't used to clear guest memory (or
> else we have a security problem). If there are pages having the U bit
> set, then I guess you need to first go and clean those up.
I got an SMAP violation in function clear_page_sse2() while debugging,
which makes me think that this function accesses user pages and the AC
bit needs to be set. However, today I find that the real fault happens in
construct_dom0() like the following code path. After adopt Kevin's suggestion,
we will make construct_dom0() wrapped in a stac()/clac() part as a whole.
So the changes here should be removed. Thanks for your comments!
__start_xen() --> construct_dom0() --> clear_page() --> clear_page_sse2()
>
> > --- a/xen/arch/x86/x86_64/compat/entry.S
> > +++ b/xen/arch/x86/x86_64/compat/entry.S
> > @@ -265,6 +265,7 @@ ENTRY(compat_int80_direct_trap)
> > /* On return only %rbx and %rdx are guaranteed non-clobbered.
> */
> > compat_create_bounce_frame:
> > ASSERT_INTERRUPTS_ENABLED
> > + ASM_STAC
> > mov %fs,%edi
> > testb $2,UREGS_cs+8(%rsp)
> > jz 1f
> > @@ -336,6 +337,7 @@ __UNLIKELY_END(compat_bounce_null_selector)
> > movl %eax,UREGS_cs+8(%rsp)
> > movl TRAPBOUNCE_eip(%rdx),%eax
> > movl %eax,UREGS_rip+8(%rsp)
> > + ASM_CLAC
> > ret
>
> These and the similar changes to xen/arch/x86/x86_64/entry.S seem
> pretty pointless without also adding ASM_CLAC to the exception and
> interrupt entry points.
>
Yes, you are right, we should add ASM_CLAC in those places! Thanks a lot!
> Jan
Thanks,
Feng
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 2/6] x86: Temporary disable SMAP to legally access user pages in kernel mode
2014-04-15 13:01 [PATCH v1 2/6] x86: Temporary disable SMAP to legally access user pages in kernel mode Feng Wu
2014-04-15 6:08 ` Tian, Kevin
2014-04-15 8:46 ` Jan Beulich
@ 2014-04-15 10:21 ` Andrew Cooper
2014-04-15 10:31 ` Jan Beulich
2 siblings, 1 reply; 9+ messages in thread
From: Andrew Cooper @ 2014-04-15 10:21 UTC (permalink / raw)
To: Feng Wu; +Cc: jun.nakajima, eddie.dong, Ian.Campbell, JBeulich, xen-devel
On 15/04/14 14:01, Feng Wu wrote:
> Use STAC/CLAC to temporary disable SMAP to allow legal accesses to
> user pages in kernel mode
>
> Signed-off-by: Feng Wu <feng.wu@intel.com>
> ---
> xen/arch/x86/clear_page.S | 3 +++
> xen/arch/x86/domain_build.c | 16 ++++++++++++++++
> xen/arch/x86/usercopy.c | 6 ++++++
> xen/arch/x86/x86_64/compat/entry.S | 2 ++
> xen/arch/x86/x86_64/entry.S | 4 ++++
> xen/include/asm-x86/uaccess.h | 4 ++++
> xen/include/asm-x86/x86_64/system.h | 2 ++
> 7 files changed, 37 insertions(+)
>
> diff --git a/xen/arch/x86/clear_page.S b/xen/arch/x86/clear_page.S
> index 96315ad..2ef4bb0 100644
> --- a/xen/arch/x86/clear_page.S
> +++ b/xen/arch/x86/clear_page.S
> @@ -1,9 +1,11 @@
> #include <xen/config.h>
> #include <asm/page.h>
> +#include <asm/asm_defns.h>
>
> #define ptr_reg %rdi
>
> ENTRY(clear_page_sse2)
> + ASM_STAC
> mov $PAGE_SIZE/16, %ecx
> xor %eax,%eax
>
> @@ -15,5 +17,6 @@ ENTRY(clear_page_sse2)
> lea 16(ptr_reg), ptr_reg
> jnz 0b
>
> + ASM_CLAC
> sfence
> ret
> diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
> index 84ce392..1ba138b 100644
> --- a/xen/arch/x86/domain_build.c
> +++ b/xen/arch/x86/domain_build.c
> @@ -789,8 +789,10 @@ int __init construct_dom0(
> rc = -1;
> goto out;
> }
> + stac();
> hypercall_page_initialise(
> d, (void *)(unsigned long)parms.virt_hypercall);
> + clac();
> }
>
> /* Free temporary buffers. */
> @@ -799,6 +801,7 @@ int __init construct_dom0(
> /* Set up start info area. */
> si = (start_info_t *)vstartinfo_start;
> clear_page(si);
> + stac();
> si->nr_pages = nr_pages;
>
> si->shared_info = virt_to_maddr(d->shared_info);
> @@ -813,6 +816,7 @@ int __init construct_dom0(
> snprintf(si->magic, sizeof(si->magic), "xen-3.0-x86_%d%s",
> elf_64bit(&elf) ? 64 : 32, parms.pae ? "p" : "");
>
> + clac();
> count = d->tot_pages;
> l4start = map_domain_page(pagetable_get_pfn(v->arch.guest_table));
> l3tab = NULL;
> @@ -956,16 +960,20 @@ int __init construct_dom0(
> if ( pfn > REVERSE_START && (vinitrd_start || pfn < initrd_pfn) )
> mfn = alloc_epfn - (pfn - REVERSE_START);
> #endif
> + stac();
> if ( !is_pv_32on64_domain(d) )
> ((unsigned long *)vphysmap_start)[pfn] = mfn;
> else
> ((unsigned int *)vphysmap_start)[pfn] = mfn;
> + clac();
> set_gpfn_from_mfn(mfn, pfn);
> if (!(pfn & 0xfffff))
> process_pending_softirqs();
> }
> + stac();
> si->first_p2m_pfn = pfn;
> si->nr_p2m_frames = d->tot_pages - count;
> + clac();
> page_list_for_each ( page, &d->page_list )
> {
> mfn = page_to_mfn(page);
> @@ -976,7 +984,9 @@ int __init construct_dom0(
> if ( !page->u.inuse.type_info &&
> !get_page_and_type(page, d, PGT_writable_page) )
> BUG();
> + stac();
> ((unsigned long *)vphysmap_start)[pfn] = mfn;
> + clac();
> set_gpfn_from_mfn(mfn, pfn);
> ++pfn;
> if (!(pfn & 0xfffff))
> @@ -985,7 +995,9 @@ int __init construct_dom0(
> }
> BUG_ON(pfn != d->tot_pages);
> #ifndef NDEBUG
> + stac();
> alloc_epfn += PFN_UP(initrd_len) + si->nr_p2m_frames;
> + clac();
This could presumably be eliminated by keeping a stack copy of
si->nr_p2m_frames ?
~Andrew
> #endif
> while ( pfn < nr_pages )
> {
> @@ -997,10 +1009,12 @@ int __init construct_dom0(
> #ifndef NDEBUG
> #define pfn (nr_pages - 1 - (pfn - (alloc_epfn - alloc_spfn)))
> #endif
> + stac();
> if ( !is_pv_32on64_domain(d) )
> ((unsigned long *)vphysmap_start)[pfn] = mfn;
> else
> ((unsigned int *)vphysmap_start)[pfn] = mfn;
> + clac();
> set_gpfn_from_mfn(mfn, pfn);
> #undef pfn
> page++; pfn++;
> @@ -1009,6 +1023,7 @@ int __init construct_dom0(
> }
> }
>
> + stac();
> if ( initrd_len != 0 )
> {
> si->mod_start = vinitrd_start ?: initrd_pfn;
> @@ -1024,6 +1039,7 @@ int __init construct_dom0(
> si->console.dom0.info_off = sizeof(struct start_info);
> si->console.dom0.info_size = sizeof(struct dom0_vga_console_info);
> }
> + clac();
>
> if ( is_pv_32on64_domain(d) )
> xlat_start_info(si, XLAT_start_info_console_dom0);
> diff --git a/xen/arch/x86/usercopy.c b/xen/arch/x86/usercopy.c
> index b79202b..5dd77db 100644
> --- a/xen/arch/x86/usercopy.c
> +++ b/xen/arch/x86/usercopy.c
> @@ -15,6 +15,7 @@ unsigned long __copy_to_user_ll(void __user *to, const void *from, unsigned n)
> unsigned long __d0, __d1, __d2, __n = n;
>
> asm volatile (
> + ASM_STAC"\n"
> " cmp $"STR(2*BYTES_PER_LONG-1)",%0\n"
> " jbe 1f\n"
> " mov %1,%0\n"
> @@ -30,6 +31,7 @@ unsigned long __copy_to_user_ll(void __user *to, const void *from, unsigned n)
> " mov %3,%0\n"
> "1: rep movsb\n" /* ...remainder copied as bytes */
> "2:\n"
> + ASM_CLAC"\n"
> ".section .fixup,\"ax\"\n"
> "5: add %3,%0\n"
> " jmp 2b\n"
> @@ -52,6 +54,7 @@ __copy_from_user_ll(void *to, const void __user *from, unsigned n)
> unsigned long __d0, __d1, __d2, __n = n;
>
> asm volatile (
> + ASM_STAC"\n"
> " cmp $"STR(2*BYTES_PER_LONG-1)",%0\n"
> " jbe 1f\n"
> " mov %1,%0\n"
> @@ -67,6 +70,7 @@ __copy_from_user_ll(void *to, const void __user *from, unsigned n)
> " mov %3,%0\n"
> "1: rep; movsb\n" /* ...remainder copied as bytes */
> "2:\n"
> + ASM_CLAC"\n"
> ".section .fixup,\"ax\"\n"
> "5: add %3,%0\n"
> " jmp 6f\n"
> @@ -114,10 +118,12 @@ copy_to_user(void __user *to, const void *from, unsigned n)
> do { \
> long __d0; \
> __asm__ __volatile__( \
> + ASM_STAC"\n" \
> "0: rep; stosl\n" \
> " movl %2,%0\n" \
> "1: rep; stosb\n" \
> "2:\n" \
> + ASM_CLAC"\n" \
> ".section .fixup,\"ax\"\n" \
> "3: lea 0(%2,%0,4),%0\n" \
> " jmp 2b\n" \
> diff --git a/xen/arch/x86/x86_64/compat/entry.S b/xen/arch/x86/x86_64/compat/entry.S
> index 32b3bcc..b0fabf4 100644
> --- a/xen/arch/x86/x86_64/compat/entry.S
> +++ b/xen/arch/x86/x86_64/compat/entry.S
> @@ -265,6 +265,7 @@ ENTRY(compat_int80_direct_trap)
> /* On return only %rbx and %rdx are guaranteed non-clobbered. */
> compat_create_bounce_frame:
> ASSERT_INTERRUPTS_ENABLED
> + ASM_STAC
> mov %fs,%edi
> testb $2,UREGS_cs+8(%rsp)
> jz 1f
> @@ -336,6 +337,7 @@ __UNLIKELY_END(compat_bounce_null_selector)
> movl %eax,UREGS_cs+8(%rsp)
> movl TRAPBOUNCE_eip(%rdx),%eax
> movl %eax,UREGS_rip+8(%rsp)
> + ASM_CLAC
> ret
> .section .fixup,"ax"
> .Lfx13:
> diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
> index 3ea4683..63ba795 100644
> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -377,6 +377,7 @@ __UNLIKELY_END(create_bounce_frame_bad_sp)
> movb TRAPBOUNCE_flags(%rdx),%cl
> subq $40,%rsi
> movq UREGS_ss+8(%rsp),%rax
> + ASM_STAC
> .Lft2: movq %rax,32(%rsi) # SS
> movq UREGS_rsp+8(%rsp),%rax
> .Lft3: movq %rax,24(%rsi) # RSP
> @@ -434,9 +435,11 @@ UNLIKELY_END(bounce_failsafe)
> testq %rax,%rax
> UNLIKELY_START(z, create_bounce_frame_bad_bounce_ip)
> lea UNLIKELY_DISPATCH_LABEL(create_bounce_frame_bad_bounce_ip)(%rip), %rdi
> + ASM_CLAC
> jmp asm_domain_crash_synchronous /* Does not return */
> __UNLIKELY_END(create_bounce_frame_bad_bounce_ip)
> movq %rax,UREGS_rip+8(%rsp)
> + ASM_CLAC
> ret
> _ASM_EXTABLE(.Lft2, dom_crash_sync_extable)
> _ASM_EXTABLE(.Lft3, dom_crash_sync_extable)
> @@ -463,6 +466,7 @@ ENTRY(dom_crash_sync_extable)
> leal (%rax,%rax,2),%eax
> orb %al,UREGS_cs(%rsp)
> xorl %edi,%edi
> + ASM_CLAC
> jmp asm_domain_crash_synchronous /* Does not return */
>
> /* No special register assumptions. */
> diff --git a/xen/include/asm-x86/uaccess.h b/xen/include/asm-x86/uaccess.h
> index 88b4ba2..ce1af4a 100644
> --- a/xen/include/asm-x86/uaccess.h
> +++ b/xen/include/asm-x86/uaccess.h
> @@ -147,8 +147,10 @@ struct __large_struct { unsigned long buf[100]; };
> */
> #define __put_user_asm(x, addr, err, itype, rtype, ltype, errret) \
> __asm__ __volatile__( \
> + ASM_STAC"\n" \
> "1: mov"itype" %"rtype"1,%2\n" \
> "2:\n" \
> + ASM_CLAC"\n" \
> ".section .fixup,\"ax\"\n" \
> "3: mov %3,%0\n" \
> " jmp 2b\n" \
> @@ -159,8 +161,10 @@ struct __large_struct { unsigned long buf[100]; };
>
> #define __get_user_asm(x, addr, err, itype, rtype, ltype, errret) \
> __asm__ __volatile__( \
> + ASM_STAC"\n" \
> "1: mov"itype" %2,%"rtype"1\n" \
> "2:\n" \
> + ASM_CLAC"\n" \
> ".section .fixup,\"ax\"\n" \
> "3: mov %3,%0\n" \
> " xor"itype" %"rtype"1,%"rtype"1\n" \
> diff --git a/xen/include/asm-x86/x86_64/system.h b/xen/include/asm-x86/x86_64/system.h
> index 20f038b..b8394b9 100644
> --- a/xen/include/asm-x86/x86_64/system.h
> +++ b/xen/include/asm-x86/x86_64/system.h
> @@ -13,8 +13,10 @@
> */
> #define __cmpxchg_user(_p,_o,_n,_isuff,_oppre,_regtype) \
> asm volatile ( \
> + ASM_STAC"\n" \
> "1: lock; cmpxchg"_isuff" %"_oppre"2,%3\n" \
> "2:\n" \
> + ASM_CLAC"\n" \
> ".section .fixup,\"ax\"\n" \
> "3: movl $1,%1\n" \
> " jmp 2b\n" \
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v1 2/6] x86: Temporary disable SMAP to legally access user pages in kernel mode
2014-04-15 10:21 ` Andrew Cooper
@ 2014-04-15 10:31 ` Jan Beulich
0 siblings, 0 replies; 9+ messages in thread
From: Jan Beulich @ 2014-04-15 10:31 UTC (permalink / raw)
To: Andrew Cooper, Feng Wu; +Cc: eddie.dong, Ian.Campbell, jun.nakajima, xen-devel
>>> On 15.04.14 at 12:21, <andrew.cooper3@citrix.com> wrote:
> On 15/04/14 14:01, Feng Wu wrote:
>> @@ -985,7 +995,9 @@ int __init construct_dom0(
>> }
>> BUG_ON(pfn != d->tot_pages);
>> #ifndef NDEBUG
>> + stac();
>> alloc_epfn += PFN_UP(initrd_len) + si->nr_p2m_frames;
>> + clac();
>
> This could presumably be eliminated by keeping a stack copy of
> si->nr_p2m_frames ?
That'll go away together with everything else here in favor of
construct_dom0() as a whole getting wrapped in a stac()/clac()
part (as suggested by Kevin).
Jan
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-04-16 6:14 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-04-15 13:01 [PATCH v1 2/6] x86: Temporary disable SMAP to legally access user pages in kernel mode Feng Wu
2014-04-15 6:08 ` Tian, Kevin
2014-04-15 6:20 ` Wu, Feng
2014-04-15 6:35 ` Tian, Kevin
2014-04-15 6:53 ` Wu, Feng
2014-04-15 8:46 ` Jan Beulich
2014-04-16 6:14 ` Wu, Feng
2014-04-15 10:21 ` Andrew Cooper
2014-04-15 10:31 ` 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.