* [PATCH v2 3/7] x86: Temporary disable SMAP to legally access user pages in kernel mode
@ 2014-04-23 14:35 Feng Wu
2014-04-23 10:34 ` Andrew Cooper
2014-04-23 10:39 ` Jan Beulich
0 siblings, 2 replies; 9+ messages in thread
From: Feng Wu @ 2014-04-23 14:35 UTC (permalink / raw)
To: xen-devel
Cc: kevin.tian, Feng Wu, JBeulich, andrew.cooper3, eddie.dong,
jun.nakajima, ian.campbell
Use STAC/CLAC to temporarily disable SMAP to allow legal accesses to
user pages in kernel mode
Signed-off-by: Feng Wu <feng.wu@intel.com>
---
xen/arch/x86/domain_build.c | 3 +++
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 ++
6 files changed, 21 insertions(+)
diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
index 84ce392..15af110 100644
--- a/xen/arch/x86/domain_build.c
+++ b/xen/arch/x86/domain_build.c
@@ -778,6 +778,7 @@ int __init construct_dom0(
}
bootstrap_map(NULL);
+ stac();
if ( UNSET_ADDR != parms.virt_hypercall )
{
if ( (parms.virt_hypercall < v_start) ||
@@ -787,6 +788,7 @@ int __init construct_dom0(
write_ptbase(current);
printk("Invalid HYPERCALL_PAGE field in ELF notes.\n");
rc = -1;
+ clac();
goto out;
}
hypercall_page_initialise(
@@ -1150,6 +1152,7 @@ int __init construct_dom0(
elf_check_broken(&elf));
iommu_dom0_init(dom0);
+ clac();
return 0;
out:
diff --git a/xen/arch/x86/usercopy.c b/xen/arch/x86/usercopy.c
index b79202b..946e40f 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 ac594c9..298c1a9 100644
--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -266,6 +266,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
@@ -337,6 +338,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 d294064..e49f9c4 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -380,6 +380,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
@@ -437,9 +438,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)
@@ -466,6 +469,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..9cb72fa 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..1dde8b9 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 v2 3/7] x86: Temporary disable SMAP to legally access user pages in kernel mode
2014-04-23 14:35 [PATCH v2 3/7] x86: Temporary disable SMAP to legally access user pages in kernel mode Feng Wu
@ 2014-04-23 10:34 ` Andrew Cooper
2014-04-23 13:43 ` Wu, Feng
2014-04-23 10:39 ` Jan Beulich
1 sibling, 1 reply; 9+ messages in thread
From: Andrew Cooper @ 2014-04-23 10:34 UTC (permalink / raw)
To: Feng Wu
Cc: kevin.tian, ian.campbell, eddie.dong, xen-devel, JBeulich,
jun.nakajima
On 23/04/14 15:35, Feng Wu wrote:
> Use STAC/CLAC to temporarily disable SMAP to allow legal accesses to
> user pages in kernel mode
>
> Signed-off-by: Feng Wu <feng.wu@intel.com>
> ---
> xen/arch/x86/domain_build.c | 3 +++
> 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 ++
> 6 files changed, 21 insertions(+)
>
> diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
> index 84ce392..15af110 100644
> --- a/xen/arch/x86/domain_build.c
> +++ b/xen/arch/x86/domain_build.c
> @@ -778,6 +778,7 @@ int __init construct_dom0(
> }
> bootstrap_map(NULL);
>
> + stac();
As constructing dom0 is trusted, this should be near the top of top of
the function
> if ( UNSET_ADDR != parms.virt_hypercall )
> {
> if ( (parms.virt_hypercall < v_start) ||
> @@ -787,6 +788,7 @@ int __init construct_dom0(
> write_ptbase(current);
> printk("Invalid HYPERCALL_PAGE field in ELF notes.\n");
> rc = -1;
> + clac();
and this should be after the out label.
> goto out;
> }
> hypercall_page_initialise(
> @@ -1150,6 +1152,7 @@ int __init construct_dom0(
> elf_check_broken(&elf));
>
> iommu_dom0_init(dom0);
> + clac();
This will need rebasing given recent changes in staging.
~Andrew
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 3/7] x86: Temporary disable SMAP to legally access user pages in kernel mode
2014-04-23 10:34 ` Andrew Cooper
@ 2014-04-23 13:43 ` Wu, Feng
2014-04-23 14:12 ` Jan Beulich
0 siblings, 1 reply; 9+ messages in thread
From: Wu, Feng @ 2014-04-23 13:43 UTC (permalink / raw)
To: Andrew Cooper
Cc: Tian, Kevin, ian.campbell@citrix.com, Dong, Eddie,
xen-devel@lists.xen.org, JBeulich@suse.com, Nakajima, Jun
> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Wednesday, April 23, 2014 6:35 PM
> To: Wu, Feng
> Cc: xen-devel@lists.xen.org; JBeulich@suse.com; Tian, Kevin; Dong, Eddie;
> Nakajima, Jun; ian.campbell@citrix.com
> Subject: Re: [PATCH v2 3/7] x86: Temporary disable SMAP to legally access user
> pages in kernel mode
>
> On 23/04/14 15:35, Feng Wu wrote:
> > Use STAC/CLAC to temporarily disable SMAP to allow legal accesses to
> > user pages in kernel mode
> >
> > Signed-off-by: Feng Wu <feng.wu@intel.com>
> > ---
> > xen/arch/x86/domain_build.c | 3 +++
> > 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 ++
> > 6 files changed, 21 insertions(+)
> >
> > diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
> > index 84ce392..15af110 100644
> > --- a/xen/arch/x86/domain_build.c
> > +++ b/xen/arch/x86/domain_build.c
> > @@ -778,6 +778,7 @@ int __init construct_dom0(
> > }
> > bootstrap_map(NULL);
> >
> > + stac();
>
> As constructing dom0 is trusted, this should be near the top of top of
> the function
We cannot call stac() near the top of the function, because construct_dom0() calls
elf_load_binary() which calls copy_from_user(), we can only add stac() after calling
elf_load_binary(), otherwise the AC bit will remain cleared after elf_load_binary().
I just sugguest another method in another mail, can you please have a look?
>
> > if ( UNSET_ADDR != parms.virt_hypercall )
> > {
> > if ( (parms.virt_hypercall < v_start) ||
> > @@ -787,6 +788,7 @@ int __init construct_dom0(
> > write_ptbase(current);
> > printk("Invalid HYPERCALL_PAGE field in ELF notes.\n");
> > rc = -1;
> > + clac();
>
> and this should be after the out label.
>
> > goto out;
> > }
> > hypercall_page_initialise(
> > @@ -1150,6 +1152,7 @@ int __init construct_dom0(
> > elf_check_broken(&elf));
> >
> > iommu_dom0_init(dom0);
> > + clac();
>
> This will need rebasing given recent changes in staging.
>
> ~Andrew
Thanks,
Feng
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 3/7] x86: Temporary disable SMAP to legally access user pages in kernel mode
2014-04-23 13:43 ` Wu, Feng
@ 2014-04-23 14:12 ` Jan Beulich
2014-04-23 22:39 ` Wu, Feng
0 siblings, 1 reply; 9+ messages in thread
From: Jan Beulich @ 2014-04-23 14:12 UTC (permalink / raw)
To: Andrew Cooper, Feng Wu
Cc: Kevin Tian, Eddie Dong, ian.campbell@citrix.com, Jun Nakajima,
xen-devel@lists.xen.org
>>> On 23.04.14 at 15:43, <feng.wu@intel.com> wrote:
>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>> On 23/04/14 15:35, Feng Wu wrote:
>> > --- a/xen/arch/x86/domain_build.c
>> > +++ b/xen/arch/x86/domain_build.c
>> > @@ -778,6 +778,7 @@ int __init construct_dom0(
>> > }
>> > bootstrap_map(NULL);
>> >
>> > + stac();
>>
>> As constructing dom0 is trusted, this should be near the top of top of
>> the function
>
> We cannot call stac() near the top of the function, because construct_dom0()
> calls
> elf_load_binary() which calls copy_from_user(), we can only add stac() after
> calling
> elf_load_binary(), otherwise the AC bit will remain cleared after
> elf_load_binary().
>
> I just sugguest another method in another mail, can you please have a look?
But that other method widened the scope even further, so would suffer
the same issue. How about enabling SMAP only after having built Dom0?
Jan
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 3/7] x86: Temporary disable SMAP to legally access user pages in kernel mode
2014-04-23 14:12 ` Jan Beulich
@ 2014-04-23 22:39 ` Wu, Feng
0 siblings, 0 replies; 9+ messages in thread
From: Wu, Feng @ 2014-04-23 22:39 UTC (permalink / raw)
To: Jan Beulich, Andrew Cooper
Cc: Tian, Kevin, Dong, Eddie, ian.campbell@citrix.com, Nakajima, Jun,
xen-devel@lists.xen.org
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Wednesday, April 23, 2014 10:13 PM
> To: Andrew Cooper; Wu, Feng
> Cc: ian.campbell@citrix.com; Dong, Eddie; Nakajima, Jun; Tian, Kevin;
> xen-devel@lists.xen.org
> Subject: RE: [PATCH v2 3/7] x86: Temporary disable SMAP to legally access user
> pages in kernel mode
>
> >>> On 23.04.14 at 15:43, <feng.wu@intel.com> wrote:
> >> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> >> On 23/04/14 15:35, Feng Wu wrote:
> >> > --- a/xen/arch/x86/domain_build.c
> >> > +++ b/xen/arch/x86/domain_build.c
> >> > @@ -778,6 +778,7 @@ int __init construct_dom0(
> >> > }
> >> > bootstrap_map(NULL);
> >> >
> >> > + stac();
> >>
> >> As constructing dom0 is trusted, this should be near the top of top of
> >> the function
> >
> > We cannot call stac() near the top of the function, because construct_dom0()
> > calls
> > elf_load_binary() which calls copy_from_user(), we can only add stac() after
> > calling
> > elf_load_binary(), otherwise the AC bit will remain cleared after
> > elf_load_binary().
> >
> > I just sugguest another method in another mail, can you please have a look?
>
> But that other method widened the scope even further, so would suffer
> the same issue. How about enabling SMAP only after having built Dom0?
Yes, it suffers the same problem. Enabling SMAP after constructing dom0 may
be a good suggestion.
>
> Jan
Thanks,
Feng
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 3/7] x86: Temporary disable SMAP to legally access user pages in kernel mode
2014-04-23 14:35 [PATCH v2 3/7] x86: Temporary disable SMAP to legally access user pages in kernel mode Feng Wu
2014-04-23 10:34 ` Andrew Cooper
@ 2014-04-23 10:39 ` Jan Beulich
2014-04-23 13:26 ` Wu, Feng
1 sibling, 1 reply; 9+ messages in thread
From: Jan Beulich @ 2014-04-23 10:39 UTC (permalink / raw)
To: Feng Wu
Cc: kevin.tian, ian.campbell, andrew.cooper3, eddie.dong, xen-devel,
jun.nakajima
>>> On 23.04.14 at 16:35, <feng.wu@intel.com> wrote:
> --- a/xen/arch/x86/domain_build.c
> +++ b/xen/arch/x86/domain_build.c
> @@ -778,6 +778,7 @@ int __init construct_dom0(
> }
> bootstrap_map(NULL);
>
> + stac();
> if ( UNSET_ADDR != parms.virt_hypercall )
> {
> if ( (parms.virt_hypercall < v_start) ||
> @@ -787,6 +788,7 @@ int __init construct_dom0(
> write_ptbase(current);
> printk("Invalid HYPERCALL_PAGE field in ELF notes.\n");
> rc = -1;
> + clac();
> goto out;
If done this way, this really should be moved to the out: label. But
I think the risk of missing a return path is quite high this way - I'd
much rather want the caller to wrap its call in a stac()/clac() pair.
> @@ -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"
Mismatched indentation (also further down).
> --- a/xen/arch/x86/x86_64/compat/entry.S
> +++ b/xen/arch/x86/x86_64/compat/entry.S
> @@ -266,6 +266,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
> @@ -337,6 +338,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:
This ignores the path(s) leading to asm_domain_crash_synchronous.
> diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
> index d294064..e49f9c4 100644
> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -380,6 +380,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
> @@ -437,9 +438,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 */
Interestingly here you spotted the need - you may want to consider
doing this at the asm_domain_crash_synchronous label instead.
Jan
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 3/7] x86: Temporary disable SMAP to legally access user pages in kernel mode
2014-04-23 10:39 ` Jan Beulich
@ 2014-04-23 13:26 ` Wu, Feng
2014-04-23 14:09 ` Jan Beulich
0 siblings, 1 reply; 9+ messages in thread
From: Wu, Feng @ 2014-04-23 13:26 UTC (permalink / raw)
To: Jan Beulich
Cc: Tian, Kevin, ian.campbell@citrix.com, andrew.cooper3@citrix.com,
Dong, Eddie, xen-devel@lists.xen.org, Nakajima, Jun
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Wednesday, April 23, 2014 6:39 PM
> To: Wu, Feng
> Cc: andrew.cooper3@citrix.com; ian.campbell@citrix.com; Dong, Eddie;
> Nakajima, Jun; Tian, Kevin; xen-devel@lists.xen.org
> Subject: Re: [PATCH v2 3/7] x86: Temporary disable SMAP to legally access user
> pages in kernel mode
>
> >>> On 23.04.14 at 16:35, <feng.wu@intel.com> wrote:
> > --- a/xen/arch/x86/domain_build.c
> > +++ b/xen/arch/x86/domain_build.c
> > @@ -778,6 +778,7 @@ int __init construct_dom0(
> > }
> > bootstrap_map(NULL);
> >
> > + stac();
> > if ( UNSET_ADDR != parms.virt_hypercall )
> > {
> > if ( (parms.virt_hypercall < v_start) ||
> > @@ -787,6 +788,7 @@ int __init construct_dom0(
> > write_ptbase(current);
> > printk("Invalid HYPERCALL_PAGE field in ELF notes.\n");
> > rc = -1;
> > + clac();
> > goto out;
>
> If done this way, this really should be moved to the out: label. But
> I think the risk of missing a return path is quite high this way - I'd
> much rather want the caller to wrap its call in a stac()/clac() pair.
So, do you think code like the following looks better?
stac();
construct_dom0();
clac();
>
> > @@ -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"
>
> Mismatched indentation (also further down).
>
> > --- a/xen/arch/x86/x86_64/compat/entry.S
> > +++ b/xen/arch/x86/x86_64/compat/entry.S
> > @@ -266,6 +266,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
> > @@ -337,6 +338,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:
>
> This ignores the path(s) leading to asm_domain_crash_synchronous.
>
> > diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
> > index d294064..e49f9c4 100644
> > --- a/xen/arch/x86/x86_64/entry.S
> > +++ b/xen/arch/x86/x86_64/entry.S
> > @@ -380,6 +380,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
> > @@ -437,9 +438,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), %rd
> i
> > + ASM_CLAC
> > jmp asm_domain_crash_synchronous /* Does not return */
>
> Interestingly here you spotted the need - you may want to consider
> doing this at the asm_domain_crash_synchronous label instead.
Yes, that should be a clean solution. But seems we don't need to call "ASM_CLAC" in
all the places where asm_domain_crash_synchronous() is called. For example:
In file xen/arch/x86/x86_64/entry.S:
create_bounce_frame:
......
......
UNLIKELY_START(g, create_bounce_frame_bad_sp)
lea UNLIKELY_DISPATCH_LABEL(create_bounce_frame_bad_sp)(%rip), %rdi
jmp asm_domain_crash_synchronous /* Does not return */
__UNLIKELY_END(create_bounce_frame_bad_sp)
......
>
> Jan
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 3/7] x86: Temporary disable SMAP to legally access user pages in kernel mode
2014-04-23 13:26 ` Wu, Feng
@ 2014-04-23 14:09 ` Jan Beulich
2014-04-23 22:41 ` Wu, Feng
0 siblings, 1 reply; 9+ messages in thread
From: Jan Beulich @ 2014-04-23 14:09 UTC (permalink / raw)
To: Feng Wu
Cc: Kevin Tian, ian.campbell@citrix.com, andrew.cooper3@citrix.com,
Eddie Dong, xen-devel@lists.xen.org, Jun Nakajima
>>> On 23.04.14 at 15:26, <feng.wu@intel.com> wrote:
>> If done this way, this really should be moved to the out: label. But
>> I think the risk of missing a return path is quite high this way - I'd
>> much rather want the caller to wrap its call in a stac()/clac() pair.
>
> So, do you think code like the following looks better?
>
> stac();
> construct_dom0();
> clac();
Yes.
>> > --- a/xen/arch/x86/x86_64/entry.S
>> > +++ b/xen/arch/x86/x86_64/entry.S
>> > @@ -380,6 +380,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
>> > @@ -437,9 +438,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), %rd
>> i
>> > + ASM_CLAC
>> > jmp asm_domain_crash_synchronous /* Does not return */
>>
>> Interestingly here you spotted the need - you may want to consider
>> doing this at the asm_domain_crash_synchronous label instead.
>
> Yes, that should be a clean solution. But seems we don't need to call
> "ASM_CLAC" in
> all the places where asm_domain_crash_synchronous() is called. For example:
>
> In file xen/arch/x86/x86_64/entry.S:
>
> create_bounce_frame:
> ......
> ......
>
> UNLIKELY_START(g, create_bounce_frame_bad_sp)
> lea UNLIKELY_DISPATCH_LABEL(create_bounce_frame_bad_sp)(%rip),
> %rdi
> jmp asm_domain_crash_synchronous /* Does not return */
> __UNLIKELY_END(create_bounce_frame_bad_sp)
Right, but the question you need to ask: Does it hurt anything? I
don't think so, and it covers all relevant cases in one go. It's a
code path that's being taken when something is wrong with the
guest anyway, so doing an clac in that case doesn't hurt a all.
Jan
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 3/7] x86: Temporary disable SMAP to legally access user pages in kernel mode
2014-04-23 14:09 ` Jan Beulich
@ 2014-04-23 22:41 ` Wu, Feng
0 siblings, 0 replies; 9+ messages in thread
From: Wu, Feng @ 2014-04-23 22:41 UTC (permalink / raw)
To: Jan Beulich
Cc: Tian, Kevin, ian.campbell@citrix.com, andrew.cooper3@citrix.com,
Dong, Eddie, xen-devel@lists.xen.org, Nakajima, Jun
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Wednesday, April 23, 2014 10:10 PM
> To: Wu, Feng
> Cc: andrew.cooper3@citrix.com; ian.campbell@citrix.com; Dong, Eddie;
> Nakajima, Jun; Tian, Kevin; xen-devel@lists.xen.org
> Subject: RE: [PATCH v2 3/7] x86: Temporary disable SMAP to legally access user
> pages in kernel mode
>
> >>> On 23.04.14 at 15:26, <feng.wu@intel.com> wrote:
> >> If done this way, this really should be moved to the out: label. But
> >> I think the risk of missing a return path is quite high this way - I'd
> >> much rather want the caller to wrap its call in a stac()/clac() pair.
> >
> > So, do you think code like the following looks better?
> >
> > stac();
> > construct_dom0();
> > clac();
>
> Yes.
>
> >> > --- a/xen/arch/x86/x86_64/entry.S
> >> > +++ b/xen/arch/x86/x86_64/entry.S
> >> > @@ -380,6 +380,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
> >> > @@ -437,9 +438,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), %rd
> >> i
> >> > + ASM_CLAC
> >> > jmp asm_domain_crash_synchronous /* Does not return
> */
> >>
> >> Interestingly here you spotted the need - you may want to consider
> >> doing this at the asm_domain_crash_synchronous label instead.
> >
> > Yes, that should be a clean solution. But seems we don't need to call
> > "ASM_CLAC" in
> > all the places where asm_domain_crash_synchronous() is called. For example:
> >
> > In file xen/arch/x86/x86_64/entry.S:
> >
> > create_bounce_frame:
> > ......
> > ......
> >
> > UNLIKELY_START(g, create_bounce_frame_bad_sp)
> > lea
> UNLIKELY_DISPATCH_LABEL(create_bounce_frame_bad_sp)(%rip),
> > %rdi
> > jmp asm_domain_crash_synchronous /* Does not return */
> > __UNLIKELY_END(create_bounce_frame_bad_sp)
>
> Right, but the question you need to ask: Does it hurt anything? I
> don't think so, and it covers all relevant cases in one go. It's a
> code path that's being taken when something is wrong with the
> guest anyway, so doing an clac in that case doesn't hurt a all.
Yes, it doesn't hurt anything.
>
> Jan
Thanks,
Feng
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-04-23 22:41 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-04-23 14:35 [PATCH v2 3/7] x86: Temporary disable SMAP to legally access user pages in kernel mode Feng Wu
2014-04-23 10:34 ` Andrew Cooper
2014-04-23 13:43 ` Wu, Feng
2014-04-23 14:12 ` Jan Beulich
2014-04-23 22:39 ` Wu, Feng
2014-04-23 10:39 ` Jan Beulich
2014-04-23 13:26 ` Wu, Feng
2014-04-23 14:09 ` Jan Beulich
2014-04-23 22:41 ` Wu, Feng
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.