* Re: [PATCH v2 2/7] Clear AC bit in RFLAGS to protect Xen itself by SMAP
2014-04-23 14:35 [PATCH v2 2/7] Clear AC bit in RFLAGS to protect Xen itself by SMAP Feng Wu
@ 2014-04-23 10:19 ` Andrew Cooper
2014-04-24 6:45 ` Wu, Feng
2014-04-23 10:30 ` Jan Beulich
1 sibling, 1 reply; 10+ messages in thread
From: Andrew Cooper @ 2014-04-23 10:19 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:
> Clear AC bit in RFLAGS at the beginning of exception, interrupt, hypercall,
> so Xen itself can be protected by SMAP mechanism.
>
> Signed-off-by: Feng Wu <feng.wu@intel.com>
> ---
> xen/arch/x86/x86_64/compat/entry.S | 1 +
> xen/arch/x86/x86_64/entry.S | 26 ++++++++++++++++++++++++++
> xen/include/asm-x86/x86_64/asm_defns.h | 1 +
> 3 files changed, 28 insertions(+)
>
> diff --git a/xen/arch/x86/x86_64/compat/entry.S b/xen/arch/x86/x86_64/compat/entry.S
> index 32b3bcc..ac594c9 100644
> --- a/xen/arch/x86/x86_64/compat/entry.S
> +++ b/xen/arch/x86/x86_64/compat/entry.S
> @@ -13,6 +13,7 @@
> #include <irq_vectors.h>
>
> ENTRY(compat_hypercall)
> + ASM_CLAC
> pushq $0
> SAVE_VOLATILE type=TRAP_syscall compat=1
>
> diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
> index 3ea4683..d294064 100644
> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -120,6 +120,7 @@ restore_all_xen:
> * the space left by the trampoline.
> */
> ENTRY(syscall_enter)
> + ASM_CLAC
Surely this can be sorted more succinctly by setting X86_EFLAGS_AC in
MSR 0xc0000084 ?
You also need to patch the entry points in the compat trampoline in
> sti
> movl $FLAT_KERNEL_SS,24(%rsp)
> pushq %rcx
> @@ -268,6 +269,7 @@ bad_hypercall:
> jmp test_all_events
>
> ENTRY(sysenter_entry)
> + ASM_CLAC
> sti
> pushq $FLAT_USER_SS
> pushq $0
> @@ -309,6 +311,7 @@ UNLIKELY_END(sysenter_gpf)
> jmp .Lbounce_exception
>
...
>
> .pushsection .init.text, "ax", @progbits
> ENTRY(early_page_fault)
> + ASM_CLAC
I don't think CLAC is appropriate here. This is a pagefault handler for
Xen early boot, and is replaced with a real handler substantially before
dom0 is created.
> SAVE_ALL
> movq %rsp,%rdi
> call do_early_page_fault
> @@ -622,6 +644,7 @@ ENTRY(early_page_fault)
> .popsection
>
> ENTRY(nmi)
> + ASM_CLAC
> pushq $0
> movl $TRAP_nmi,4(%rsp)
> handle_ist_exception:
> @@ -659,6 +682,7 @@ handle_ist_exception:
> jmp compat_restore_all_guest
>
> ENTRY(nmi_crash)
> + ASM_CLAC
> pushq $0
> movl $TRAP_nmi,4(%rsp)
> SAVE_ALL
> @@ -667,6 +691,7 @@ ENTRY(nmi_crash)
> ud2
>
> ENTRY(machine_check)
> + ASM_CLAC
> pushq $0
> movl $TRAP_machine_check,4(%rsp)
> jmp handle_ist_exception
> @@ -689,6 +714,7 @@ ENTRY(enable_nmis)
>
> /* No op trap handler. Required for kexec crash path. */
> GLOBAL(trap_nop)
> + ASM_CLAC
> iretq
This is not sensible in the slightest, given the following instruction.
~Andrew
>
>
> diff --git a/xen/include/asm-x86/x86_64/asm_defns.h b/xen/include/asm-x86/x86_64/asm_defns.h
> index bf63ac1..69f76b2 100644
> --- a/xen/include/asm-x86/x86_64/asm_defns.h
> +++ b/xen/include/asm-x86/x86_64/asm_defns.h
> @@ -212,6 +212,7 @@
> __asm__( \
> "\n" __ALIGN_STR"\n" \
> "common_interrupt:\n\t" \
> + ASM_CLAC(%)"\n\t" \
> STR(SAVE_ALL) "\n\t" \
> "movq %rsp,%rdi\n\t" \
> "callq " STR(do_IRQ) "\n\t" \
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/7] Clear AC bit in RFLAGS to protect Xen itself by SMAP
2014-04-23 14:35 [PATCH v2 2/7] Clear AC bit in RFLAGS to protect Xen itself by SMAP Feng Wu
2014-04-23 10:19 ` Andrew Cooper
@ 2014-04-23 10:30 ` Jan Beulich
1 sibling, 0 replies; 10+ messages in thread
From: Jan Beulich @ 2014-04-23 10:30 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/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -120,6 +120,7 @@ restore_all_xen:
> * the space left by the trampoline.
> */
> ENTRY(syscall_enter)
> + ASM_CLAC
This should be avoidable by instead adjusting the value written to
MSR_SYSCALL_MASK.
> @@ -476,6 +479,7 @@ ENTRY(ret_from_intr)
> jmp compat_test_all_events
>
> ENTRY(page_fault)
> + ASM_CLAC
> movl $TRAP_page_fault,4(%rsp)
> /* No special register assumptions. */
> GLOBAL(handle_exception)
Considering that ASM_CLAC already contains a memory access (which
hence isn't guarded) I wonder whether this wouldn't better be moved
into handle_exception at least for now (reducing code redundancy).
Jan
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 2/7] Clear AC bit in RFLAGS to protect Xen itself by SMAP
@ 2014-04-23 14:35 Feng Wu
2014-04-23 10:19 ` Andrew Cooper
2014-04-23 10:30 ` Jan Beulich
0 siblings, 2 replies; 10+ 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
Clear AC bit in RFLAGS at the beginning of exception, interrupt, hypercall,
so Xen itself can be protected by SMAP mechanism.
Signed-off-by: Feng Wu <feng.wu@intel.com>
---
xen/arch/x86/x86_64/compat/entry.S | 1 +
xen/arch/x86/x86_64/entry.S | 26 ++++++++++++++++++++++++++
xen/include/asm-x86/x86_64/asm_defns.h | 1 +
3 files changed, 28 insertions(+)
diff --git a/xen/arch/x86/x86_64/compat/entry.S b/xen/arch/x86/x86_64/compat/entry.S
index 32b3bcc..ac594c9 100644
--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -13,6 +13,7 @@
#include <irq_vectors.h>
ENTRY(compat_hypercall)
+ ASM_CLAC
pushq $0
SAVE_VOLATILE type=TRAP_syscall compat=1
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index 3ea4683..d294064 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -120,6 +120,7 @@ restore_all_xen:
* the space left by the trampoline.
*/
ENTRY(syscall_enter)
+ ASM_CLAC
sti
movl $FLAT_KERNEL_SS,24(%rsp)
pushq %rcx
@@ -268,6 +269,7 @@ bad_hypercall:
jmp test_all_events
ENTRY(sysenter_entry)
+ ASM_CLAC
sti
pushq $FLAT_USER_SS
pushq $0
@@ -309,6 +311,7 @@ UNLIKELY_END(sysenter_gpf)
jmp .Lbounce_exception
ENTRY(int80_direct_trap)
+ ASM_CLAC
pushq $0
SAVE_VOLATILE 0x80
@@ -476,6 +479,7 @@ ENTRY(ret_from_intr)
jmp compat_test_all_events
ENTRY(page_fault)
+ ASM_CLAC
movl $TRAP_page_fault,4(%rsp)
/* No special register assumptions. */
GLOBAL(handle_exception)
@@ -532,81 +536,98 @@ FATAL_exception_with_ints_disabled:
ud2
ENTRY(divide_error)
+ ASM_CLAC
pushq $0
movl $TRAP_divide_error,4(%rsp)
jmp handle_exception
ENTRY(coprocessor_error)
+ ASM_CLAC
pushq $0
movl $TRAP_copro_error,4(%rsp)
jmp handle_exception
ENTRY(simd_coprocessor_error)
+ ASM_CLAC
pushq $0
movl $TRAP_simd_error,4(%rsp)
jmp handle_exception
ENTRY(device_not_available)
+ ASM_CLAC
pushq $0
movl $TRAP_no_device,4(%rsp)
jmp handle_exception
ENTRY(debug)
+ ASM_CLAC
pushq $0
movl $TRAP_debug,4(%rsp)
jmp handle_exception
ENTRY(int3)
+ ASM_CLAC
pushq $0
movl $TRAP_int3,4(%rsp)
jmp handle_exception
ENTRY(overflow)
+ ASM_CLAC
pushq $0
movl $TRAP_overflow,4(%rsp)
jmp handle_exception
ENTRY(bounds)
+ ASM_CLAC
pushq $0
movl $TRAP_bounds,4(%rsp)
jmp handle_exception
ENTRY(invalid_op)
+ ASM_CLAC
pushq $0
movl $TRAP_invalid_op,4(%rsp)
jmp handle_exception
ENTRY(coprocessor_segment_overrun)
+ ASM_CLAC
pushq $0
movl $TRAP_copro_seg,4(%rsp)
jmp handle_exception
ENTRY(invalid_TSS)
+ ASM_CLAC
movl $TRAP_invalid_tss,4(%rsp)
jmp handle_exception
ENTRY(segment_not_present)
+ ASM_CLAC
movl $TRAP_no_segment,4(%rsp)
jmp handle_exception
ENTRY(stack_segment)
+ ASM_CLAC
movl $TRAP_stack_error,4(%rsp)
jmp handle_exception
ENTRY(general_protection)
+ ASM_CLAC
movl $TRAP_gp_fault,4(%rsp)
jmp handle_exception
ENTRY(alignment_check)
+ ASM_CLAC
movl $TRAP_alignment_check,4(%rsp)
jmp handle_exception
ENTRY(spurious_interrupt_bug)
+ ASM_CLAC
pushq $0
movl $TRAP_spurious_int,4(%rsp)
jmp handle_exception
ENTRY(double_fault)
+ ASM_CLAC
movl $TRAP_double_fault,4(%rsp)
SAVE_ALL
movq %rsp,%rdi
@@ -615,6 +636,7 @@ ENTRY(double_fault)
.pushsection .init.text, "ax", @progbits
ENTRY(early_page_fault)
+ ASM_CLAC
SAVE_ALL
movq %rsp,%rdi
call do_early_page_fault
@@ -622,6 +644,7 @@ ENTRY(early_page_fault)
.popsection
ENTRY(nmi)
+ ASM_CLAC
pushq $0
movl $TRAP_nmi,4(%rsp)
handle_ist_exception:
@@ -659,6 +682,7 @@ handle_ist_exception:
jmp compat_restore_all_guest
ENTRY(nmi_crash)
+ ASM_CLAC
pushq $0
movl $TRAP_nmi,4(%rsp)
SAVE_ALL
@@ -667,6 +691,7 @@ ENTRY(nmi_crash)
ud2
ENTRY(machine_check)
+ ASM_CLAC
pushq $0
movl $TRAP_machine_check,4(%rsp)
jmp handle_ist_exception
@@ -689,6 +714,7 @@ ENTRY(enable_nmis)
/* No op trap handler. Required for kexec crash path. */
GLOBAL(trap_nop)
+ ASM_CLAC
iretq
diff --git a/xen/include/asm-x86/x86_64/asm_defns.h b/xen/include/asm-x86/x86_64/asm_defns.h
index bf63ac1..69f76b2 100644
--- a/xen/include/asm-x86/x86_64/asm_defns.h
+++ b/xen/include/asm-x86/x86_64/asm_defns.h
@@ -212,6 +212,7 @@
__asm__( \
"\n" __ALIGN_STR"\n" \
"common_interrupt:\n\t" \
+ ASM_CLAC(%)"\n\t" \
STR(SAVE_ALL) "\n\t" \
"movq %rsp,%rdi\n\t" \
"callq " STR(do_IRQ) "\n\t" \
--
1.8.3.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/7] Clear AC bit in RFLAGS to protect Xen itself by SMAP
2014-04-23 10:19 ` Andrew Cooper
@ 2014-04-24 6:45 ` Wu, Feng
2014-04-24 7:19 ` Jan Beulich
0 siblings, 1 reply; 10+ messages in thread
From: Wu, Feng @ 2014-04-24 6:45 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:20 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 2/7] Clear AC bit in RFLAGS to protect Xen itself by SMAP
>
> On 23/04/14 15:35, Feng Wu wrote:
> > Clear AC bit in RFLAGS at the beginning of exception, interrupt, hypercall,
> > so Xen itself can be protected by SMAP mechanism.
> >
> > Signed-off-by: Feng Wu <feng.wu@intel.com>
> > ---
> > xen/arch/x86/x86_64/compat/entry.S | 1 +
> > xen/arch/x86/x86_64/entry.S | 26
> ++++++++++++++++++++++++++
> > xen/include/asm-x86/x86_64/asm_defns.h | 1 +
> > 3 files changed, 28 insertions(+)
> >
> > diff --git a/xen/arch/x86/x86_64/compat/entry.S
> b/xen/arch/x86/x86_64/compat/entry.S
> > index 32b3bcc..ac594c9 100644
> > --- a/xen/arch/x86/x86_64/compat/entry.S
> > +++ b/xen/arch/x86/x86_64/compat/entry.S
> > @@ -13,6 +13,7 @@
> > #include <irq_vectors.h>
> >
> > ENTRY(compat_hypercall)
> > + ASM_CLAC
> > pushq $0
> > SAVE_VOLATILE type=TRAP_syscall compat=1
> >
> > diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
> > index 3ea4683..d294064 100644
> > --- a/xen/arch/x86/x86_64/entry.S
> > +++ b/xen/arch/x86/x86_64/entry.S
> > @@ -120,6 +120,7 @@ restore_all_xen:
> > * the space left by the trampoline.
> > */
> > ENTRY(syscall_enter)
> > + ASM_CLAC
>
> Surely this can be sorted more succinctly by setting X86_EFLAGS_AC in
> MSR 0xc0000084 ?
Okay.
>
> You also need to patch the entry points in the compat trampoline in
The MSR_SYSCALL_MASK is common in long mode and compat mode, right?
Seems no need to do anything else for compat mode.
>
> > sti
> > movl $FLAT_KERNEL_SS,24(%rsp)
> > pushq %rcx
> > @@ -268,6 +269,7 @@ bad_hypercall:
> > jmp test_all_events
> >
> > ENTRY(sysenter_entry)
> > + ASM_CLAC
> > sti
> > pushq $FLAT_USER_SS
> > pushq $0
> > @@ -309,6 +311,7 @@ UNLIKELY_END(sysenter_gpf)
> > jmp .Lbounce_exception
> >
> ...
> >
> > .pushsection .init.text, "ax", @progbits
> > ENTRY(early_page_fault)
> > + ASM_CLAC
>
> I don't think CLAC is appropriate here. This is a pagefault handler for
> Xen early boot, and is replaced with a real handler substantially before
> dom0 is created.
Adding CLAC here is not so useful, but harmful neither. If you think it should be
removed, I will do that in the next post.
>
> > SAVE_ALL
> > movq %rsp,%rdi
> > call do_early_page_fault
> > @@ -622,6 +644,7 @@ ENTRY(early_page_fault)
> > .popsection
> >
> > ENTRY(nmi)
> > + ASM_CLAC
> > pushq $0
> > movl $TRAP_nmi,4(%rsp)
> > handle_ist_exception:
> > @@ -659,6 +682,7 @@ handle_ist_exception:
> > jmp compat_restore_all_guest
> >
> > ENTRY(nmi_crash)
> > + ASM_CLAC
> > pushq $0
> > movl $TRAP_nmi,4(%rsp)
> > SAVE_ALL
> > @@ -667,6 +691,7 @@ ENTRY(nmi_crash)
> > ud2
> >
> > ENTRY(machine_check)
> > + ASM_CLAC
> > pushq $0
> > movl $TRAP_machine_check,4(%rsp)
> > jmp handle_ist_exception
> > @@ -689,6 +714,7 @@ ENTRY(enable_nmis)
> >
> > /* No op trap handler. Required for kexec crash path. */
> > GLOBAL(trap_nop)
> > + ASM_CLAC
> > iretq
>
> This is not sensible in the slightest, given the following instruction.
>
> ~Andrew
>
The same comments as early_page_fault case.
> >
> >
> > diff --git a/xen/include/asm-x86/x86_64/asm_defns.h
> b/xen/include/asm-x86/x86_64/asm_defns.h
> > index bf63ac1..69f76b2 100644
> > --- a/xen/include/asm-x86/x86_64/asm_defns.h
> > +++ b/xen/include/asm-x86/x86_64/asm_defns.h
> > @@ -212,6 +212,7 @@
> > __asm__( \
> > "\n" __ALIGN_STR"\n" \
> > "common_interrupt:\n\t" \
> > + ASM_CLAC(%)"\n\t" \
> > STR(SAVE_ALL) "\n\t" \
> > "movq %rsp,%rdi\n\t" \
> > "callq " STR(do_IRQ) "\n\t" \
Thanks,
Feng
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/7] Clear AC bit in RFLAGS to protect Xen itself by SMAP
2014-04-24 6:45 ` Wu, Feng
@ 2014-04-24 7:19 ` Jan Beulich
2014-04-24 7:20 ` Wu, Feng
0 siblings, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2014-04-24 7:19 UTC (permalink / raw)
To: Feng Wu
Cc: Kevin Tian, ian.campbell@citrix.com, Andrew Cooper, Eddie Dong,
xen-devel@lists.xen.org, Jun Nakajima
>>> On 24.04.14 at 08:45, <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/x86_64/entry.S
>> > +++ b/xen/arch/x86/x86_64/entry.S
>> > @@ -120,6 +120,7 @@ restore_all_xen:
>> > * the space left by the trampoline.
>> > */
>> > ENTRY(syscall_enter)
>> > + ASM_CLAC
>>
>> Surely this can be sorted more succinctly by setting X86_EFLAGS_AC in
>> MSR 0xc0000084 ?
>
> Okay.
>
>>
>> You also need to patch the entry points in the compat trampoline in
>
> The MSR_SYSCALL_MASK is common in long mode and compat mode, right?
> Seems no need to do anything else for compat mode.
The same stub is being written there twice, just with different CS
selectors, and both are behind that EFLAGS masking MSR. So I'm not
sure what Andrew's apparently incomplete sentence was actually
intended to mean.
>> > @@ -268,6 +269,7 @@ bad_hypercall:
>> > jmp test_all_events
>> >
>> > ENTRY(sysenter_entry)
>> > + ASM_CLAC
>> > sti
>> > pushq $FLAT_USER_SS
>> > pushq $0
Looking at this again, btw, makes me thing that the clac should go
after the sti here.
>> > @@ -309,6 +311,7 @@ UNLIKELY_END(sysenter_gpf)
>> > jmp .Lbounce_exception
>> >
>> ...
>> >
>> > .pushsection .init.text, "ax", @progbits
>> > ENTRY(early_page_fault)
>> > + ASM_CLAC
>>
>> I don't think CLAC is appropriate here. This is a pagefault handler for
>> Xen early boot, and is replaced with a real handler substantially before
>> dom0 is created.
>
> Adding CLAC here is not so useful, but harmful neither. If you think it
> should be removed, I will do that in the next post.
Yes, let's not scatter it around pointlessly (even more so now that
you plan on enabling SMAP only after having built Dom0).
>> > @@ -689,6 +714,7 @@ ENTRY(enable_nmis)
>> >
>> > /* No op trap handler. Required for kexec crash path. */
>> > GLOBAL(trap_nop)
>> > + ASM_CLAC
>> > iretq
>>
>> This is not sensible in the slightest, given the following instruction.
>
> The same comments as early_page_fault case.
The situation is different here, but the addition indeed doesn't seem
to make sense.
Jan
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/7] Clear AC bit in RFLAGS to protect Xen itself by SMAP
2014-04-24 7:19 ` Jan Beulich
@ 2014-04-24 7:20 ` Wu, Feng
2014-04-24 10:51 ` Andrew Cooper
0 siblings, 1 reply; 10+ messages in thread
From: Wu, Feng @ 2014-04-24 7:20 UTC (permalink / raw)
To: Jan Beulich
Cc: Tian, Kevin, ian.campbell@citrix.com, Andrew Cooper, Dong, Eddie,
xen-devel@lists.xen.org, Nakajima, Jun
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Thursday, April 24, 2014 3:19 PM
> To: Wu, Feng
> Cc: Andrew Cooper; ian.campbell@citrix.com; Dong, Eddie; Nakajima, Jun; Tian,
> Kevin; xen-devel@lists.xen.org
> Subject: RE: [PATCH v2 2/7] Clear AC bit in RFLAGS to protect Xen itself by SMAP
>
> >>> On 24.04.14 at 08:45, <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/x86_64/entry.S
> >> > +++ b/xen/arch/x86/x86_64/entry.S
> >> > @@ -120,6 +120,7 @@ restore_all_xen:
> >> > * the space left by the trampoline.
> >> > */
> >> > ENTRY(syscall_enter)
> >> > + ASM_CLAC
> >>
> >> Surely this can be sorted more succinctly by setting X86_EFLAGS_AC in
> >> MSR 0xc0000084 ?
> >
> > Okay.
> >
> >>
> >> You also need to patch the entry points in the compat trampoline in
> >
> > The MSR_SYSCALL_MASK is common in long mode and compat mode, right?
> > Seems no need to do anything else for compat mode.
>
> The same stub is being written there twice, just with different CS
> selectors, and both are behind that EFLAGS masking MSR. So I'm not
> sure what Andrew's apparently incomplete sentence was actually
> intended to mean.
Yes, that is also my understanding.
>
> >> > @@ -268,6 +269,7 @@ bad_hypercall:
> >> > jmp test_all_events
> >> >
> >> > ENTRY(sysenter_entry)
> >> > + ASM_CLAC
> >> > sti
> >> > pushq $FLAT_USER_SS
> >> > pushq $0
>
> Looking at this again, btw, makes me thing that the clac should go
> after the sti here.
>
> >> > @@ -309,6 +311,7 @@ UNLIKELY_END(sysenter_gpf)
> >> > jmp .Lbounce_exception
> >> >
> >> ...
> >> >
> >> > .pushsection .init.text, "ax", @progbits
> >> > ENTRY(early_page_fault)
> >> > + ASM_CLAC
> >>
> >> I don't think CLAC is appropriate here. This is a pagefault handler for
> >> Xen early boot, and is replaced with a real handler substantially before
> >> dom0 is created.
> >
> > Adding CLAC here is not so useful, but harmful neither. If you think it
> > should be removed, I will do that in the next post.
>
> Yes, let's not scatter it around pointlessly (even more so now that
> you plan on enabling SMAP only after having built Dom0).
>
> >> > @@ -689,6 +714,7 @@ ENTRY(enable_nmis)
> >> >
> >> > /* No op trap handler. Required for kexec crash path. */
> >> > GLOBAL(trap_nop)
> >> > + ASM_CLAC
> >> > iretq
> >>
> >> This is not sensible in the slightest, given the following instruction.
> >
> > The same comments as early_page_fault case.
>
> The situation is different here, but the addition indeed doesn't seem
> to make sense.
>
> Jan
Thanks,
Feng
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/7] Clear AC bit in RFLAGS to protect Xen itself by SMAP
2014-04-24 7:20 ` Wu, Feng
@ 2014-04-24 10:51 ` Andrew Cooper
2014-04-24 11:37 ` Jan Beulich
2014-04-25 2:10 ` Wu, Feng
0 siblings, 2 replies; 10+ messages in thread
From: Andrew Cooper @ 2014-04-24 10:51 UTC (permalink / raw)
To: Wu, Feng
Cc: Tian, Kevin, ian.campbell@citrix.com, Dong, Eddie,
xen-devel@lists.xen.org, Jan Beulich, Nakajima, Jun
On 24/04/14 08:20, Wu, Feng wrote:
>
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Thursday, April 24, 2014 3:19 PM
>> To: Wu, Feng
>> Cc: Andrew Cooper; ian.campbell@citrix.com; Dong, Eddie; Nakajima, Jun; Tian,
>> Kevin; xen-devel@lists.xen.org
>> Subject: RE: [PATCH v2 2/7] Clear AC bit in RFLAGS to protect Xen itself by SMAP
>>
>>>>> On 24.04.14 at 08:45, <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/x86_64/entry.S
>>>>> +++ b/xen/arch/x86/x86_64/entry.S
>>>>> @@ -120,6 +120,7 @@ restore_all_xen:
>>>>> * the space left by the trampoline.
>>>>> */
>>>>> ENTRY(syscall_enter)
>>>>> + ASM_CLAC
>>>> Surely this can be sorted more succinctly by setting X86_EFLAGS_AC in
>>>> MSR 0xc0000084 ?
>>> Okay.
>>>
>>>> You also need to patch the entry points in the compat trampoline in
>>> The MSR_SYSCALL_MASK is common in long mode and compat mode, right?
>>> Seems no need to do anything else for compat mode.
>> The same stub is being written there twice, just with different CS
>> selectors, and both are behind that EFLAGS masking MSR. So I'm not
>> sure what Andrew's apparently incomplete sentence was actually
>> intended to mean.
> Yes, that is also my understanding.
It was my mistake - I got the sysenter and syscall entry points confused
given a brief look at the code yesterday. On more careful review it is
currently fine.
>
>>>>> @@ -268,6 +269,7 @@ bad_hypercall:
>>>>> jmp test_all_events
>>>>>
>>>>> ENTRY(sysenter_entry)
>>>>> + ASM_CLAC
>>>>> sti
>>>>> pushq $FLAT_USER_SS
>>>>> pushq $0
>> Looking at this again, btw, makes me thing that the clac should go
>> after the sti here.
It must be after sysenter_eflags_saved, or we will erroneously clear the
AC flag from the flags used to restore guest context.
>>
>>>>> @@ -309,6 +311,7 @@ UNLIKELY_END(sysenter_gpf)
>>>>> jmp .Lbounce_exception
>>>>>
>>>> ...
>>>>> .pushsection .init.text, "ax", @progbits
>>>>> ENTRY(early_page_fault)
>>>>> + ASM_CLAC
>>>> I don't think CLAC is appropriate here. This is a pagefault handler for
>>>> Xen early boot, and is replaced with a real handler substantially before
>>>> dom0 is created.
>>> Adding CLAC here is not so useful, but harmful neither. If you think it
>>> should be removed, I will do that in the next post.
>> Yes, let's not scatter it around pointlessly (even more so now that
>> you plan on enabling SMAP only after having built Dom0).
>>
>>>>> @@ -689,6 +714,7 @@ ENTRY(enable_nmis)
>>>>>
>>>>> /* No op trap handler. Required for kexec crash path. */
>>>>> GLOBAL(trap_nop)
>>>>> + ASM_CLAC
>>>>> iretq
>>>> This is not sensible in the slightest, given the following instruction.
>>> The same comments as early_page_fault case.
>> The situation is different here, but the addition indeed doesn't seem
>> to make sense.
The handler is used to prevent NMIs and MCEs getting in the way of the
crash path. Slowing down the return to the crash path is counter
productive in all cases.
~Andrew
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/7] Clear AC bit in RFLAGS to protect Xen itself by SMAP
2014-04-24 10:51 ` Andrew Cooper
@ 2014-04-24 11:37 ` Jan Beulich
2014-04-25 2:02 ` Wu, Feng
2014-04-25 2:10 ` Wu, Feng
1 sibling, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2014-04-24 11:37 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 24.04.14 at 12:51, <andrew.cooper3@citrix.com> wrote:
> On 24/04/14 08:20, Wu, Feng wrote:
>>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>>>>> On 24.04.14 at 08:45, <feng.wu@intel.com> wrote:
>>>>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>>>>> On 23/04/14 15:35, Feng Wu wrote:
>>>>>> @@ -268,6 +269,7 @@ bad_hypercall:
>>>>>> jmp test_all_events
>>>>>>
>>>>>> ENTRY(sysenter_entry)
>>>>>> + ASM_CLAC
>>>>>> sti
>>>>>> pushq $FLAT_USER_SS
>>>>>> pushq $0
>>> Looking at this again, btw, makes me thing that the clac should go
>>> after the sti here.
>
> It must be after sysenter_eflags_saved, or we will erroneously clear the
> AC flag from the flags used to restore guest context.
Indeed, and not just AC considering that the macro right now involves
a conditional branch.
Jan
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/7] Clear AC bit in RFLAGS to protect Xen itself by SMAP
2014-04-24 11:37 ` Jan Beulich
@ 2014-04-25 2:02 ` Wu, Feng
0 siblings, 0 replies; 10+ messages in thread
From: Wu, Feng @ 2014-04-25 2:02 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: Thursday, April 24, 2014 7:37 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 2/7] Clear AC bit in RFLAGS to protect Xen itself by SMAP
>
> >>> On 24.04.14 at 12:51, <andrew.cooper3@citrix.com> wrote:
> > On 24/04/14 08:20, Wu, Feng wrote:
> >>> From: Jan Beulich [mailto:JBeulich@suse.com]
> >>>>>> On 24.04.14 at 08:45, <feng.wu@intel.com> wrote:
> >>>>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> >>>>> On 23/04/14 15:35, Feng Wu wrote:
> >>>>>> @@ -268,6 +269,7 @@ bad_hypercall:
> >>>>>> jmp test_all_events
> >>>>>>
> >>>>>> ENTRY(sysenter_entry)
> >>>>>> + ASM_CLAC
> >>>>>> sti
> >>>>>> pushq $FLAT_USER_SS
> >>>>>> pushq $0
> >>> Looking at this again, btw, makes me thing that the clac should go
> >>> after the sti here.
> >
> > It must be after sysenter_eflags_saved, or we will erroneously clear the
> > AC flag from the flags used to restore guest context.
>
> Indeed, and not just AC considering that the macro right now involves
> a conditional branch.
>
Thanks for your comments, I will change this, which may involve bugs here.
> Jan
Thanks,
Feng
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/7] Clear AC bit in RFLAGS to protect Xen itself by SMAP
2014-04-24 10:51 ` Andrew Cooper
2014-04-24 11:37 ` Jan Beulich
@ 2014-04-25 2:10 ` Wu, Feng
1 sibling, 0 replies; 10+ messages in thread
From: Wu, Feng @ 2014-04-25 2:10 UTC (permalink / raw)
To: Andrew Cooper
Cc: Tian, Kevin, ian.campbell@citrix.com, Dong, Eddie,
xen-devel@lists.xen.org, Jan Beulich, Nakajima, Jun
> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Thursday, April 24, 2014 6:51 PM
> To: Wu, Feng
> Cc: Jan Beulich; ian.campbell@citrix.com; Dong, Eddie; Nakajima, Jun; Tian,
> Kevin; xen-devel@lists.xen.org
> Subject: Re: [PATCH v2 2/7] Clear AC bit in RFLAGS to protect Xen itself by SMAP
>
> On 24/04/14 08:20, Wu, Feng wrote:
> >
> >> -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: Thursday, April 24, 2014 3:19 PM
> >> To: Wu, Feng
> >> Cc: Andrew Cooper; ian.campbell@citrix.com; Dong, Eddie; Nakajima, Jun;
> Tian,
> >> Kevin; xen-devel@lists.xen.org
> >> Subject: RE: [PATCH v2 2/7] Clear AC bit in RFLAGS to protect Xen itself by
> SMAP
> >>
> >>>>> On 24.04.14 at 08:45, <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/x86_64/entry.S
> >>>>> +++ b/xen/arch/x86/x86_64/entry.S
> >>>>> @@ -120,6 +120,7 @@ restore_all_xen:
> >>>>> * the space left by the trampoline.
> >>>>> */
> >>>>> ENTRY(syscall_enter)
> >>>>> + ASM_CLAC
> >>>> Surely this can be sorted more succinctly by setting X86_EFLAGS_AC in
> >>>> MSR 0xc0000084 ?
> >>> Okay.
> >>>
> >>>> You also need to patch the entry points in the compat trampoline in
> >>> The MSR_SYSCALL_MASK is common in long mode and compat mode,
> right?
> >>> Seems no need to do anything else for compat mode.
> >> The same stub is being written there twice, just with different CS
> >> selectors, and both are behind that EFLAGS masking MSR. So I'm not
> >> sure what Andrew's apparently incomplete sentence was actually
> >> intended to mean.
> > Yes, that is also my understanding.
>
> It was my mistake - I got the sysenter and syscall entry points confused
> given a brief look at the code yesterday. On more careful review it is
> currently fine.
>
> >
> >>>>> @@ -268,6 +269,7 @@ bad_hypercall:
> >>>>> jmp test_all_events
> >>>>>
> >>>>> ENTRY(sysenter_entry)
> >>>>> + ASM_CLAC
> >>>>> sti
> >>>>> pushq $FLAT_USER_SS
> >>>>> pushq $0
> >> Looking at this again, btw, makes me thing that the clac should go
> >> after the sti here.
>
> It must be after sysenter_eflags_saved, or we will erroneously clear the
> AC flag from the flags used to restore guest context.
Great comments, will follow it!
>
> >>
> >>>>> @@ -309,6 +311,7 @@ UNLIKELY_END(sysenter_gpf)
> >>>>> jmp .Lbounce_exception
> >>>>>
> >>>> ...
> >>>>> .pushsection .init.text, "ax", @progbits
> >>>>> ENTRY(early_page_fault)
> >>>>> + ASM_CLAC
> >>>> I don't think CLAC is appropriate here. This is a pagefault handler for
> >>>> Xen early boot, and is replaced with a real handler substantially before
> >>>> dom0 is created.
> >>> Adding CLAC here is not so useful, but harmful neither. If you think it
> >>> should be removed, I will do that in the next post.
> >> Yes, let's not scatter it around pointlessly (even more so now that
> >> you plan on enabling SMAP only after having built Dom0).
> >>
> >>>>> @@ -689,6 +714,7 @@ ENTRY(enable_nmis)
> >>>>>
> >>>>> /* No op trap handler. Required for kexec crash path. */
> >>>>> GLOBAL(trap_nop)
> >>>>> + ASM_CLAC
> >>>>> iretq
> >>>> This is not sensible in the slightest, given the following instruction.
> >>> The same comments as early_page_fault case.
> >> The situation is different here, but the addition indeed doesn't seem
> >> to make sense.
>
> The handler is used to prevent NMIs and MCEs getting in the way of the
> crash path. Slowing down the return to the crash path is counter
> productive in all cases.
>
Got it, thanks a lot for the clear explanation!
> ~Andrew
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-04-25 2:10 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-04-23 14:35 [PATCH v2 2/7] Clear AC bit in RFLAGS to protect Xen itself by SMAP Feng Wu
2014-04-23 10:19 ` Andrew Cooper
2014-04-24 6:45 ` Wu, Feng
2014-04-24 7:19 ` Jan Beulich
2014-04-24 7:20 ` Wu, Feng
2014-04-24 10:51 ` Andrew Cooper
2014-04-24 11:37 ` Jan Beulich
2014-04-25 2:02 ` Wu, Feng
2014-04-25 2:10 ` Wu, Feng
2014-04-23 10:30 ` Jan Beulich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).