[PATCH 0/3] sp0, ss1, and sp1 docs and minor fixes

[PATCH 0/3] sp0, ss1, and sp1 docs and minor fixes

[Andy Lutomirski]

This documents things, clarifies the code, and fixes an apparently
inconsequential bug that caused the init sp0 to be wrong.

(My earlier 32-bit bug was caused because I thought that the code
I'm fixing was correct.  Whoops.)

Andy Lutomirski (3):
  x86: Create and use a TOP_OF_KERNEL_STACK_PADDING macro
  x86: Unify and fix init sp0
  x86_32: Document our abuse of ss1 and sp1

 arch/x86/include/asm/processor.h   | 31 +++++++++++++++++++++++++------
 arch/x86/include/asm/thread_info.h | 30 ++++++++++++++++++++++++++++++
 arch/x86/kernel/process.c          |  2 +-
 3 files changed, 56 insertions(+), 7 deletions(-)

-- 
2.3.0

[PATCH 1/3] x86: Create and use a TOP_OF_KERNEL_STACK_PADDING macro

[Andy Lutomirski]

x86_32, unlike x86_64, pads the top of the kernel stack.  Document
this padding and give it a name.

This should make no change whatsoever to the compiled kernel image.
It also doesn't fix any of the current bugs in this area.

Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 arch/x86/include/asm/processor.h   |  3 ++-
 arch/x86/include/asm/thread_info.h | 30 ++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 48a61c1c626e..88d9aa745898 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -849,7 +849,8 @@ extern unsigned long thread_saved_pc(struct task_struct *tsk);
 #define task_pt_regs(task)                                             \
 ({                                                                     \
        struct pt_regs *__regs__;                                       \
-       __regs__ = (struct pt_regs *)(KSTK_TOP(task_stack_page(task))-8); \
+       __regs__ = (struct pt_regs *)(KSTK_TOP(task_stack_page(task)) - \
+				     TOP_OF_KERNEL_STACK_PADDING);     \
        __regs__ - 1;                                                   \
 })
 
diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
index 7740edd56fed..74fd74ca50d3 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -49,6 +49,36 @@ struct thread_info {
 #define init_thread_info	(init_thread_union.thread_info)
 #define init_stack		(init_thread_union.stack)
 
+#ifdef CONFIG_X86_32
+
+/*
+ * TOP_OF_KERNEL_STACK_PADDING is a number of unused bytes that we
+ * reserve at the top of the kernel stack.  We do it because of a nasty
+ * 32-bit corner case.  On x86_32, the hardware stack frame is
+ * variable-length.  Except for vm86 mode, struct pt_regs assumes a
+ * maximum-length frame.  If we enter from CPL 0, the top 8 bytes of
+ * pt_regs don't actually exist.  Ordinarily this doesn't matter, but it
+ * does in at least one case:
+ *
+ * If we take an NMI early enough in sysenter, the we can end up with
+ * pt_regs that extends above sp0.  On the way out, in the espfix code,
+ * we can read the saved SS value, but that value will be above sp0.
+ * Without this offset, that can result in a page fault.  (We are
+ * careful that, in this case, the value we read doesn't matter.)
+ *
+ * In vm86 mode, the hardware frame is much longer still, but we neither
+ * access the extra members from NMI context, nor do we write such a
+ * frame at sp0 at all.
+ */
+#define TOP_OF_KERNEL_STACK_PADDING 8
+
+#else
+
+/* Phew, x86_64 has a fixed-length stack frame! */
+#define TOP_OF_KERNEL_STACK_PADDING 0
+
+#endif
+
 #else /* !__ASSEMBLY__ */
 
 #include <asm/asm-offsets.h>
-- 
2.3.0

[PATCH 2/3] x86: Unify and fix init sp0

[Andy Lutomirski]

x86_32 and x86_64 need slightly different sp0 values, and x86_32's was
incorrect for init.  (This never mattered -- the init thread never
runs user code, so we never used sp0 for anything.)

Fix it and mostly unify them.

Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 arch/x86/include/asm/processor.h | 7 +++++--
 arch/x86/kernel/process.c        | 2 +-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 88d9aa745898..fc6d8d0d8d53 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -811,6 +811,9 @@ static inline void spin_lock_prefetch(const void *x)
 	prefetchw(x);
 }
 
+#define TOP_OF_INIT_STACK ((unsigned long)&init_stack + sizeof(init_stack) - \
+			   TOP_OF_KERNEL_STACK_PADDING)
+
 #ifdef CONFIG_X86_32
 /*
  * User space process size: 3GB (default).
@@ -821,7 +824,7 @@ static inline void spin_lock_prefetch(const void *x)
 #define STACK_TOP_MAX		STACK_TOP
 
 #define INIT_THREAD  {							  \
-	.sp0			= sizeof(init_stack) + (long)&init_stack, \
+	.sp0			= TOP_OF_INIT_STACK,			  \
 	.vm86_info		= NULL,					  \
 	.sysenter_cs		= __KERNEL_CS,				  \
 	.io_bitmap_ptr		= NULL,					  \
@@ -883,7 +886,7 @@ extern unsigned long thread_saved_pc(struct task_struct *tsk);
 #define STACK_TOP_MAX		TASK_SIZE_MAX
 
 #define INIT_THREAD  { \
-	.sp0 = (unsigned long)&init_stack + sizeof(init_stack) \
+	.sp0 = TOP_OF_INIT_STACK \
 }
 
 /*
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index f4c0af7fc3a0..12b1cf606ddf 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -39,7 +39,7 @@
  */
 __visible DEFINE_PER_CPU_SHARED_ALIGNED(struct tss_struct, cpu_tss) = {
 	.x86_tss = {
-		.sp0 = (unsigned long)&init_stack + sizeof(init_stack),
+		.sp0 = TOP_OF_INIT_STACK,
 #ifdef CONFIG_X86_32
 		.ss0 = __KERNEL_DS,
 		.ss1 = __KERNEL_CS,
-- 
2.3.0

[PATCH 3/3] x86_32: Document our abuse of ss1 and sp1

[Andy Lutomirski]

This has confused me for a while.  Now that I figured it out,
document it.

Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 arch/x86/include/asm/processor.h | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index fc6d8d0d8d53..b26208998b7c 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -209,9 +209,24 @@ struct x86_hw_tss {
 	unsigned short		back_link, __blh;
 	unsigned long		sp0;
 	unsigned short		ss0, __ss0h;
-	unsigned long		sp1;
-	/* ss1 caches MSR_IA32_SYSENTER_CS: */
-	unsigned short		ss1, __ss1h;
+
+	/*
+	 * We don't use ring 1, so sp1 and ss1 are convenient scratch
+	 * spaces in the same cacheline as sp0.  We use them to cache
+	 * some MSR values to avoid unnecessary wrmsr instructions.
+	 *
+	 * We use SYSENTER_ESP to find sp0 and for the NMI emergency
+	 * stack, but we need to context switch it because we do
+	 * horrible things to the kernel stack in vm86 mode.
+	 *
+	 * We use SYSENTER_CS to disable sysenter in vm86 mode to avoid
+	 * corrupting the stack if we went through the sysenter path
+	 * from vm86 mode.
+	 */
+	unsigned long		sp1;	/* MSR_IA32_SYSENTER_ESP */
+	unsigned short		ss1;	/* MSR_IA32_SYSENTER_CS */
+
+	unsigned short		__ss1h;
 	unsigned long		sp2;
 	unsigned short		ss2, __ss2h;
 	unsigned long		__cr3;
-- 
2.3.0

Re: [PATCH 3/3] x86_32: Document our abuse of ss1 and sp1

[Denys Vlasenko]

On 03/10/2015 07:06 PM, Andy Lutomirski wrote:
> This has confused me for a while.  Now that I figured it out,
> document it.

Great!

> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> ---
>  arch/x86/include/asm/processor.h | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index fc6d8d0d8d53..b26208998b7c 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -209,9 +209,24 @@ struct x86_hw_tss {
>  	unsigned short		back_link, __blh;
>  	unsigned long		sp0;
>  	unsigned short		ss0, __ss0h;
> -	unsigned long		sp1;
> -	/* ss1 caches MSR_IA32_SYSENTER_CS: */
> -	unsigned short		ss1, __ss1h;
> +
> +	/*
> +	 * We don't use ring 1, so sp1 and ss1 are convenient scratch
> +	 * spaces in the same cacheline as sp0.  We use them to cache
> +	 * some MSR values to avoid unnecessary wrmsr instructions.

I don't see where exactly tss.ss1/sp1 is getting used as cache.
Grepping for "sp1" string, I found only this:

$ grep -r '[.>]e*sp1' .
./kernel/cpu/common.c:	tss->x86_tss.sp1 = sizeof(struct tss_struct) + (unsigned long) tss;
./kernel/cpu/common.c:	wrmsr(MSR_IA32_SYSENTER_ESP, tss->x86_tss.sp1, 0);

void enable_sep_cpu(void)
{
        int cpu = get_cpu();
        struct tss_struct *tss = &per_cpu(init_tss, cpu);
...
        tss->x86_tss.ss1 = __KERNEL_CS;
        tss->x86_tss.sp1 = sizeof(struct tss_struct) + (unsigned long) tss;
        wrmsr(MSR_IA32_SYSENTER_CS, __KERNEL_CS, 0);
        wrmsr(MSR_IA32_SYSENTER_ESP, tss->x86_tss.sp1, 0);
        wrmsr(MSR_IA32_SYSENTER_EIP, (unsigned long) ia32_sysenter_target, 0);
        put_cpu();
}

It's trivial to rewrite this wrmsr(MSR_IA32_SYSENTER_ESP)
without the detour through x86_tss.sp1.

Apart from this, x86_tss.sp1 appears unused...   ????confused????



.ss1 also seems to be a write-only field:

$ grep -r '[.>]ss1' .
./include/asm/processor.h:	if (unlikely(tss->x86_tss.ss1 != thread->sysenter_cs)) {
./include/asm/processor.h:		tss->x86_tss.ss1 = thread->sysenter_cs;
./include/asm/processor.h:		.ss1		= __KERNEL_CS,				  \
./kernel/cpu/common.c:	tss->x86_tss.ss1 = __KERNEL_CS;



> +	 *
> +	 * We use SYSENTER_ESP to find sp0 and for the NMI emergency
> +	 * stack,

We use what? SYSENTER_ESP is a MSR, right? We don't use it (the MSR)
to find anything... I don't understand what you are saying here.


 but we need to context switch it because we do
> +	 * horrible things to the kernel stack in vm86 mode.
> +	 *
> +	 * We use SYSENTER_CS to disable sysenter in vm86 mode to avoid
> +	 * corrupting the stack if we went through the sysenter path
> +	 * from vm86 mode.
> +	 */

I'm confused how loading ss1/sp1 with anything can disable sysenter.
SYSENTER insn does not use those fields.

What you _can_ disable is you can make it impossible to enter RING1
if tss.ss1 is invalid.


> +	unsigned long		sp1;	/* MSR_IA32_SYSENTER_ESP */
> +	unsigned short		ss1;	/* MSR_IA32_SYSENTER_CS */

The comments in the right don't explain anything (to me, at least).

Sorry for sounding negative.

Re: [PATCH 1/3] x86: Create and use a TOP_OF_KERNEL_STACK_PADDING macro

[Denys Vlasenko]

On 03/10/2015 07:05 PM, Andy Lutomirski wrote:
> x86_32, unlike x86_64, pads the top of the kernel stack.  Document
> this padding and give it a name.
> 
> This should make no change whatsoever to the compiled kernel image.
> It also doesn't fix any of the current bugs in this area.
> 
> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> ---
>  arch/x86/include/asm/processor.h   |  3 ++-
>  arch/x86/include/asm/thread_info.h | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index 48a61c1c626e..88d9aa745898 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -849,7 +849,8 @@ extern unsigned long thread_saved_pc(struct task_struct *tsk);
>  #define task_pt_regs(task)                                             \
>  ({                                                                     \
>         struct pt_regs *__regs__;                                       \
> -       __regs__ = (struct pt_regs *)(KSTK_TOP(task_stack_page(task))-8); \
> +       __regs__ = (struct pt_regs *)(KSTK_TOP(task_stack_page(task)) - \
> +				     TOP_OF_KERNEL_STACK_PADDING);     \
>         __regs__ - 1;                                                   \
>  })

The "magic 8" also hides here:

        /*
         * Push current_thread_info()->sysenter_return to the stack.
         * A tiny bit of offset fixup is necessary - 4*4 means the 4 words
         * pushed above; +8 corresponds to copy_thread's esp0 setting.
         */
        pushl_cfi ((TI_sysenter_return)-THREAD_SIZE+8+4*4)(%esp)
                                                   ^^^
        CFI_REL_OFFSET eip, 0


It may make sense to mention TOP_OF_KERNEL_STACK_PADDING here
(there are no useful comments in copy_thread() anymore):

        movl PT_EFLAGS(%esp), %eax      # mix EFLAGS, SS and CS
        # Warning: PT_OLDSS(%esp) contains the wrong/random values if we
        # are returning to the kernel.
        # See comments in process.c:copy_thread() for details.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
maybe s/comments in process.c:copy_thread()/TOP_OF_KERNEL_STACK_PADDING/ ?
        movb PT_OLDSS(%esp), %ah
        movb PT_CS(%esp), %al
        andl $(X86_EFLAGS_VM | (SEGMENT_TI_MASK << 8) | SEGMENT_RPL_MASK), %eax
        cmpl $((SEGMENT_LDT << 8) | USER_RPL), %eax
        CFI_REMEMBER_STATE

Re: [PATCH 1/3] x86: Create and use a TOP_OF_KERNEL_STACK_PADDING macro

[Andy Lutomirski]

On Tue, Mar 10, 2015 at 12:22 PM, Denys Vlasenko <dvlasenk@redhat.com> wrote:
> On 03/10/2015 07:05 PM, Andy Lutomirski wrote:
>> x86_32, unlike x86_64, pads the top of the kernel stack.  Document
>> this padding and give it a name.
>>
>> This should make no change whatsoever to the compiled kernel image.
>> It also doesn't fix any of the current bugs in this area.
>>
>> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
>> ---
>>  arch/x86/include/asm/processor.h   |  3 ++-
>>  arch/x86/include/asm/thread_info.h | 30 ++++++++++++++++++++++++++++++
>>  2 files changed, 32 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
>> index 48a61c1c626e..88d9aa745898 100644
>> --- a/arch/x86/include/asm/processor.h
>> +++ b/arch/x86/include/asm/processor.h
>> @@ -849,7 +849,8 @@ extern unsigned long thread_saved_pc(struct task_struct *tsk);
>>  #define task_pt_regs(task)                                             \
>>  ({                                                                     \
>>         struct pt_regs *__regs__;                                       \
>> -       __regs__ = (struct pt_regs *)(KSTK_TOP(task_stack_page(task))-8); \
>> +       __regs__ = (struct pt_regs *)(KSTK_TOP(task_stack_page(task)) - \
>> +                                  TOP_OF_KERNEL_STACK_PADDING);     \
>>         __regs__ - 1;                                                   \
>>  })
>
> The "magic 8" also hides here:
>
>         /*
>          * Push current_thread_info()->sysenter_return to the stack.
>          * A tiny bit of offset fixup is necessary - 4*4 means the 4 words
>          * pushed above; +8 corresponds to copy_thread's esp0 setting.
>          */
>         pushl_cfi ((TI_sysenter_return)-THREAD_SIZE+8+4*4)(%esp)
>                                                    ^^^

I'll just put TOP_OF_KERNEL_STACK_PADDING there, too.

--Andy

>         CFI_REL_OFFSET eip, 0
>
>
> It may make sense to mention TOP_OF_KERNEL_STACK_PADDING here
> (there are no useful comments in copy_thread() anymore):
>
>         movl PT_EFLAGS(%esp), %eax      # mix EFLAGS, SS and CS
>         # Warning: PT_OLDSS(%esp) contains the wrong/random values if we
>         # are returning to the kernel.
>         # See comments in process.c:copy_thread() for details.
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> maybe s/comments in process.c:copy_thread()/TOP_OF_KERNEL_STACK_PADDING/ ?
>         movb PT_OLDSS(%esp), %ah
>         movb PT_CS(%esp), %al
>         andl $(X86_EFLAGS_VM | (SEGMENT_TI_MASK << 8) | SEGMENT_RPL_MASK), %eax
>         cmpl $((SEGMENT_LDT << 8) | USER_RPL), %eax
>         CFI_REMEMBER_STATE
>



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH 3/3] x86_32: Document our abuse of ss1 and sp1

[Andy Lutomirski]

On Tue, Mar 10, 2015 at 12:13 PM, Denys Vlasenko <dvlasenk@redhat.com> wrote:
> On 03/10/2015 07:06 PM, Andy Lutomirski wrote:
>> This has confused me for a while.  Now that I figured it out,
>> document it.
>
> Great!
>
>> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
>> ---
>>  arch/x86/include/asm/processor.h | 21 ++++++++++++++++++---
>>  1 file changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
>> index fc6d8d0d8d53..b26208998b7c 100644
>> --- a/arch/x86/include/asm/processor.h
>> +++ b/arch/x86/include/asm/processor.h
>> @@ -209,9 +209,24 @@ struct x86_hw_tss {
>>       unsigned short          back_link, __blh;
>>       unsigned long           sp0;
>>       unsigned short          ss0, __ss0h;
>> -     unsigned long           sp1;
>> -     /* ss1 caches MSR_IA32_SYSENTER_CS: */
>> -     unsigned short          ss1, __ss1h;
>> +
>> +     /*
>> +      * We don't use ring 1, so sp1 and ss1 are convenient scratch
>> +      * spaces in the same cacheline as sp0.  We use them to cache
>> +      * some MSR values to avoid unnecessary wrmsr instructions.
>
> I don't see where exactly tss.ss1/sp1 is getting used as cache.
> Grepping for "sp1" string, I found only this:
>
> $ grep -r '[.>]e*sp1' .
> ./kernel/cpu/common.c:  tss->x86_tss.sp1 = sizeof(struct tss_struct) + (unsigned long) tss;
> ./kernel/cpu/common.c:  wrmsr(MSR_IA32_SYSENTER_ESP, tss->x86_tss.sp1, 0);
>
> void enable_sep_cpu(void)
> {
>         int cpu = get_cpu();
>         struct tss_struct *tss = &per_cpu(init_tss, cpu);
> ...
>         tss->x86_tss.ss1 = __KERNEL_CS;
>         tss->x86_tss.sp1 = sizeof(struct tss_struct) + (unsigned long) tss;
>         wrmsr(MSR_IA32_SYSENTER_CS, __KERNEL_CS, 0);
>         wrmsr(MSR_IA32_SYSENTER_ESP, tss->x86_tss.sp1, 0);
>         wrmsr(MSR_IA32_SYSENTER_EIP, (unsigned long) ia32_sysenter_target, 0);
>         put_cpu();
> }
>
> It's trivial to rewrite this wrmsr(MSR_IA32_SYSENTER_ESP)
> without the detour through x86_tss.sp1.
>
> Apart from this, x86_tss.sp1 appears unused...   ????confused????
>

Hmm.  Perhaps I hallucinated it.  Maybe we should just remove this
instead.  We change sp0, but not SYSENTER_ESP.  I'll add a fourth
patch to the series.

>
>
> .ss1 also seems to be a write-only field:
>
> $ grep -r '[.>]ss1' .
> ./include/asm/processor.h:      if (unlikely(tss->x86_tss.ss1 != thread->sysenter_cs)) {

This is a read :)

> ./include/asm/processor.h:              tss->x86_tss.ss1 = thread->sysenter_cs;
> ./include/asm/processor.h:              .ss1            = __KERNEL_CS,                            \
> ./kernel/cpu/common.c:  tss->x86_tss.ss1 = __KERNEL_CS;
>
>
>
>> +      *
>> +      * We use SYSENTER_ESP to find sp0 and for the NMI emergency
>> +      * stack,
>
> We use what? SYSENTER_ESP is a MSR, right? We don't use it (the MSR)
> to find anything... I don't understand what you are saying here.
>

As noted above, I'm wrong, so I won't bother clarifying.  Will fix.

>
>  but we need to context switch it because we do
>> +      * horrible things to the kernel stack in vm86 mode.
>> +      *
>> +      * We use SYSENTER_CS to disable sysenter in vm86 mode to avoid
>> +      * corrupting the stack if we went through the sysenter path
>> +      * from vm86 mode.
>> +      */
>
> I'm confused how loading ss1/sp1 with anything can disable sysenter.
> SYSENTER insn does not use those fields.
>
> What you _can_ disable is you can make it impossible to enter RING1
> if tss.ss1 is invalid.

Does it make sense now that I pointed out the read of ss1?  If not,
I'll improve the comments.

>
>
>> +     unsigned long           sp1;    /* MSR_IA32_SYSENTER_ESP */
>> +     unsigned short          ss1;    /* MSR_IA32_SYSENTER_CS */
>
> The comments in the right don't explain anything (to me, at least).
>
> Sorry for sounding negative.

No problem :)

--Andy

-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH 3/3] x86_32: Document our abuse of ss1 and sp1

[Denys Vlasenko]

On Tue, Mar 10, 2015 at 9:06 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>> .ss1 also seems to be a write-only field:
>>
>> $ grep -r '[.>]ss1' .
>> ./include/asm/processor.h:      if (unlikely(tss->x86_tss.ss1 != thread->sysenter_cs)) {
>
> This is a read :)

You are right.

>> ./include/asm/processor.h:              tss->x86_tss.ss1 = thread->sysenter_cs;
>> ./include/asm/processor.h:              .ss1            = __KERNEL_CS,                            \
>> ./kernel/cpu/common.c:  tss->x86_tss.ss1 = __KERNEL_CS;

>>> +      * but we need to context switch it because we do
>>> +      * horrible things to the kernel stack in vm86 mode.
>>> +      *
>>> +      * We use SYSENTER_CS to disable sysenter in vm86 mode to avoid
>>> +      * corrupting the stack if we went through the sysenter path
>>> +      * from vm86 mode.
>>> +      */
>>
>> I'm confused how loading ss1/sp1 with anything can disable sysenter.
>> SYSENTER insn does not use those fields.
>>
>> What you _can_ disable is you can make it impossible to enter RING1
>> if tss.ss1 is invalid.
>
> Does it make sense now that I pointed out the read of ss1?  If not,
> I'll improve the comments.

I propose the following comment about tss.ss1:

/*
tss.ss1 is used to avoid redundant wrmsr(MSR_IA32_SYSENTER_CS).
After wrmsr, tss.ss1 is set to the written value. If on future task switches
tss.ss1 already contains the value to be written, wrmsr is skipped.
*/


>>> +      * We use SYSENTER_CS to disable sysenter in vm86 mode to avoid
>>> +      * corrupting the stack if we went through the sysenter path
>>> +      * from vm86 mode.

This appears to be untrue - SYSENTER_CS isn't used to disable sysenter.
Zero is.

Disabling sysenter happens in vm86_32.c here, by setting it to 0:

static void do_sys_vm86(struct kernel_vm86_struct *info, struct
task_struct *tsk)
{
...
        if (cpu_has_sep)
                tsk->thread.sysenter_cs = 0;
        load_sp0(tss, &tsk->thread);   <-- this sets
wrmsr(MSR_IA32_SYSENTER_CS, 0);
...

Re: [PATCH 2/3] x86: Unify and fix init sp0

[Borislav Petkov]

On Tue, Mar 10, 2015 at 11:05:59AM -0700, Andy Lutomirski wrote:
> x86_32 and x86_64 need slightly different sp0 values, and x86_32's was
> incorrect for init.  (This never mattered -- the init thread never
> runs user code, so we never used sp0 for anything.)

Damn old x86 cruft - sp0 is the stack pointer for CPL0 in the TSS. Had
to go dig that out.

> Fix it and mostly unify them.
> 
> Signed-off-by: Andy Lutomirski <luto@amacapital.net>

Acked-by: Borislav Petkov <bp@suse.de>

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH 1/3] x86: Create and use a TOP_OF_KERNEL_STACK_PADDING macro

[Denys Vlasenko]

On Tue, Mar 10, 2015 at 8:22 PM, Denys Vlasenko <dvlasenk@redhat.com> wrote:
> On 03/10/2015 07:05 PM, Andy Lutomirski wrote:
>> x86_32, unlike x86_64, pads the top of the kernel stack.  Document
>> this padding and give it a name.
>>
>> This should make no change whatsoever to the compiled kernel image.
>> It also doesn't fix any of the current bugs in this area.
>>
>> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
>> ---
>>  arch/x86/include/asm/processor.h   |  3 ++-
>>  arch/x86/include/asm/thread_info.h | 30 ++++++++++++++++++++++++++++++
>>  2 files changed, 32 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
>> index 48a61c1c626e..88d9aa745898 100644
>> --- a/arch/x86/include/asm/processor.h
>> +++ b/arch/x86/include/asm/processor.h
>> @@ -849,7 +849,8 @@ extern unsigned long thread_saved_pc(struct task_struct *tsk);
>>  #define task_pt_regs(task)                                             \
>>  ({                                                                     \
>>         struct pt_regs *__regs__;                                       \
>> -       __regs__ = (struct pt_regs *)(KSTK_TOP(task_stack_page(task))-8); \
>> +       __regs__ = (struct pt_regs *)(KSTK_TOP(task_stack_page(task)) - \
>> +                                  TOP_OF_KERNEL_STACK_PADDING);     \
>>         __regs__ - 1;                                                   \
>>  })

While we are at it, this macro can be simplified further. It uses KSTK_TOP,
and it's the only use of that macro. In turn, KSTK_TOP uses THREAD_SIZE_LONGS,
and it is the only use of that macro too!
(And to nitpick, this macro is whitespace-damaged. Seven space indent).

I'll send a patch based on your current git.

Re: [PATCH 1/3] x86: Create and use a TOP_OF_KERNEL_STACK_PADDING macro

[Ingo Molnar]

* Andy Lutomirski <luto@amacapital.net> wrote:

> x86_32, unlike x86_64, pads the top of the kernel stack.  Document
> this padding and give it a name.
> 
> This should make no change whatsoever to the compiled kernel image.
> It also doesn't fix any of the current bugs in this area.
> 
> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> ---
>  arch/x86/include/asm/processor.h   |  3 ++-
>  arch/x86/include/asm/thread_info.h | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index 48a61c1c626e..88d9aa745898 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -849,7 +849,8 @@ extern unsigned long thread_saved_pc(struct task_struct *tsk);
>  #define task_pt_regs(task)                                             \
>  ({                                                                     \
>         struct pt_regs *__regs__;                                       \
> -       __regs__ = (struct pt_regs *)(KSTK_TOP(task_stack_page(task))-8); \
> +       __regs__ = (struct pt_regs *)(KSTK_TOP(task_stack_page(task)) - \
> +				     TOP_OF_KERNEL_STACK_PADDING);     \
>         __regs__ - 1;                                                   \
>  })
>  
> diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
> index 7740edd56fed..74fd74ca50d3 100644
> --- a/arch/x86/include/asm/thread_info.h
> +++ b/arch/x86/include/asm/thread_info.h
> @@ -49,6 +49,36 @@ struct thread_info {
>  #define init_thread_info	(init_thread_union.thread_info)
>  #define init_stack		(init_thread_union.stack)
>  
> +#ifdef CONFIG_X86_32
> +
> +/*
> + * TOP_OF_KERNEL_STACK_PADDING is a number of unused bytes that we
> + * reserve at the top of the kernel stack.  We do it because of a nasty
> + * 32-bit corner case.  On x86_32, the hardware stack frame is
> + * variable-length.  Except for vm86 mode, struct pt_regs assumes a
> + * maximum-length frame.  If we enter from CPL 0, the top 8 bytes of
> + * pt_regs don't actually exist.  Ordinarily this doesn't matter, but it
> + * does in at least one case:
> + *
> + * If we take an NMI early enough in sysenter, the we can end up with

s/the/then

I fixed this up in the commit.

> + * pt_regs that extends above sp0.  On the way out, in the espfix code,
> + * we can read the saved SS value, but that value will be above sp0.
> + * Without this offset, that can result in a page fault.  (We are
> + * careful that, in this case, the value we read doesn't matter.)
> + *
> + * In vm86 mode, the hardware frame is much longer still, but we neither
> + * access the extra members from NMI context, nor do we write such a
> + * frame at sp0 at all.
> + */

Thanks,

	Ingo

[tip:x86/asm] x86/asm/entry: Create and use a ' TOP_OF_KERNEL_STACK_PADDING' macro

[tip-bot for Andy Lutomirski]

Commit-ID:  3bfef48e27863ebbd47d68a3f4a081d2cb5286aa
Gitweb:     http://git.kernel.org/tip/3bfef48e27863ebbd47d68a3f4a081d2cb5286aa
Author:     Andy Lutomirski <luto@amacapital.net>
AuthorDate: Tue, 10 Mar 2015 11:05:58 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Mon, 16 Mar 2015 11:05:30 +0100

x86/asm/entry: Create and use a 'TOP_OF_KERNEL_STACK_PADDING' macro

x86_32, unlike x86_64, pads the top of the kernel stack, because the
hardware stack frame formats are variable in size.

Document this padding and give it a name.

This should make no change whatsoever to the compiled kernel
image. It also doesn't fix any of the current bugs in this area.

Signed-off-by: Andy Lutomirski <luto@amacapital.net>
Acked-by: Denys Vlasenko <dvlasenk@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/02bf2f54b8dcb76a62a142b6dfe07d4ef7fc582e.1426009661.git.luto@amacapital.net
[ Fixed small details, such as a missed magic constant in entry_32.S pointed out by Denys Vlasenko. ]
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/processor.h   |  3 ++-
 arch/x86/include/asm/thread_info.h | 27 +++++++++++++++++++++++++++
 arch/x86/kernel/entry_32.S         |  2 +-
 3 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index c77605d..554da61 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -848,7 +848,8 @@ extern unsigned long thread_saved_pc(struct task_struct *tsk);
 #define task_pt_regs(task)                                             \
 ({                                                                     \
        struct pt_regs *__regs__;                                       \
-       __regs__ = (struct pt_regs *)(KSTK_TOP(task_stack_page(task))-8); \
+       __regs__ = (struct pt_regs *)(KSTK_TOP(task_stack_page(task)) - \
+				     TOP_OF_KERNEL_STACK_PADDING);     \
        __regs__ - 1;                                                   \
 })
 
diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
index 7740edd..ba115eb 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -13,6 +13,33 @@
 #include <asm/types.h>
 
 /*
+ * TOP_OF_KERNEL_STACK_PADDING is a number of unused bytes that we
+ * reserve at the top of the kernel stack.  We do it because of a nasty
+ * 32-bit corner case.  On x86_32, the hardware stack frame is
+ * variable-length.  Except for vm86 mode, struct pt_regs assumes a
+ * maximum-length frame.  If we enter from CPL 0, the top 8 bytes of
+ * pt_regs don't actually exist.  Ordinarily this doesn't matter, but it
+ * does in at least one case:
+ *
+ * If we take an NMI early enough in SYSENTER, then we can end up with
+ * pt_regs that extends above sp0.  On the way out, in the espfix code,
+ * we can read the saved SS value, but that value will be above sp0.
+ * Without this offset, that can result in a page fault.  (We are
+ * careful that, in this case, the value we read doesn't matter.)
+ *
+ * In vm86 mode, the hardware frame is much longer still, but we neither
+ * access the extra members from NMI context, nor do we write such a
+ * frame at sp0 at all.
+ *
+ * x86_64 has a fixed-length stack frame.
+ */
+#ifdef CONFIG_X86_32
+# define TOP_OF_KERNEL_STACK_PADDING 8
+#else
+# define TOP_OF_KERNEL_STACK_PADDING 0
+#endif
+
+/*
  * low level task data that entry.S needs immediate access to
  * - this struct should fit entirely inside of one cache line
  * - this struct shares the supervisor stack pages
diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
index e33ba51..4c8cc34 100644
--- a/arch/x86/kernel/entry_32.S
+++ b/arch/x86/kernel/entry_32.S
@@ -398,7 +398,7 @@ sysenter_past_esp:
 	 * A tiny bit of offset fixup is necessary - 4*4 means the 4 words
 	 * pushed above; +8 corresponds to copy_thread's esp0 setting.
 	 */
-	pushl_cfi ((TI_sysenter_return)-THREAD_SIZE+8+4*4)(%esp)
+	pushl_cfi ((TI_sysenter_return)-THREAD_SIZE+TOP_OF_KERNEL_STACK_PADDING+4*4)(%esp)
 	CFI_REL_OFFSET eip, 0
 
 	pushl_cfi %eax

[tip:x86/asm] x86/asm/entry: Unify and fix initial thread_struct: :sp0 values

[tip-bot for Andy Lutomirski]

Commit-ID:  6ecb31635fd0d342b04b9a8e58f2e11382b99bee
Gitweb:     http://git.kernel.org/tip/6ecb31635fd0d342b04b9a8e58f2e11382b99bee
Author:     Andy Lutomirski <luto@amacapital.net>
AuthorDate: Tue, 10 Mar 2015 11:05:59 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Mon, 16 Mar 2015 11:05:36 +0100

x86/asm/entry: Unify and fix initial thread_struct::sp0 values

x86_32 and x86_64 need slightly different thread_struct::sp0 values, and
x86_32's was incorrect for init.

This never mattered -- the init thread never runs user code, so we never
used thread_struct::sp0 for anything.

Fix it and mostly unify them.

Signed-off-by: Andy Lutomirski <luto@amacapital.net>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/1b810c1d2e797e27bb4a7708c426101161edd1f6.1426009661.git.luto@amacapital.net
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/processor.h | 7 +++++--
 arch/x86/kernel/process.c        | 2 +-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 554da61..aed6d4f 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -810,6 +810,9 @@ static inline void spin_lock_prefetch(const void *x)
 	prefetchw(x);
 }
 
+#define TOP_OF_INIT_STACK ((unsigned long)&init_stack + sizeof(init_stack) - \
+			   TOP_OF_KERNEL_STACK_PADDING)
+
 #ifdef CONFIG_X86_32
 /*
  * User space process size: 3GB (default).
@@ -820,7 +823,7 @@ static inline void spin_lock_prefetch(const void *x)
 #define STACK_TOP_MAX		STACK_TOP
 
 #define INIT_THREAD  {							  \
-	.sp0			= sizeof(init_stack) + (long)&init_stack, \
+	.sp0			= TOP_OF_INIT_STACK,			  \
 	.vm86_info		= NULL,					  \
 	.sysenter_cs		= __KERNEL_CS,				  \
 	.io_bitmap_ptr		= NULL,					  \
@@ -882,7 +885,7 @@ extern unsigned long thread_saved_pc(struct task_struct *tsk);
 #define STACK_TOP_MAX		TASK_SIZE_MAX
 
 #define INIT_THREAD  { \
-	.sp0 = (unsigned long)&init_stack + sizeof(init_stack) \
+	.sp0 = TOP_OF_INIT_STACK \
 }
 
 /*
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index f4c0af7..12b1cf6 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -39,7 +39,7 @@
  */
 __visible DEFINE_PER_CPU_SHARED_ALIGNED(struct tss_struct, cpu_tss) = {
 	.x86_tss = {
-		.sp0 = (unsigned long)&init_stack + sizeof(init_stack),
+		.sp0 = TOP_OF_INIT_STACK,
 #ifdef CONFIG_X86_32
 		.ss0 = __KERNEL_DS,
 		.ss1 = __KERNEL_CS,

[tip:x86/asm] x86/asm/entry/32: Document our abuse of x86_hw_tss: :ss1 and x86_hw_tss::sp1

[tip-bot for Andy Lutomirski]

Commit-ID:  f7113ffa3bcd630066119723f539db75a5721c88
Gitweb:     http://git.kernel.org/tip/f7113ffa3bcd630066119723f539db75a5721c88
Author:     Andy Lutomirski <luto@amacapital.net>
AuthorDate: Tue, 10 Mar 2015 11:06:00 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Mon, 16 Mar 2015 11:05:36 +0100

x86/asm/entry/32: Document our abuse of x86_hw_tss::ss1 and x86_hw_tss::sp1

This has confused me for a while.  Now that I figured it out, document it.

Signed-off-by: Andy Lutomirski <luto@amacapital.net>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/b7efc1b7364039824776f68e9ddee9ec1500e894.1426009661.git.luto@amacapital.net
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/processor.h | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index aed6d4f..c3a037b 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -209,9 +209,24 @@ struct x86_hw_tss {
 	unsigned short		back_link, __blh;
 	unsigned long		sp0;
 	unsigned short		ss0, __ss0h;
-	unsigned long		sp1;
-	/* ss1 caches MSR_IA32_SYSENTER_CS: */
-	unsigned short		ss1, __ss1h;
+
+	/*
+	 * We don't use ring 1, so sp1 and ss1 are convenient scratch
+	 * spaces in the same cacheline as sp0.  We use them to cache
+	 * some MSR values to avoid unnecessary wrmsr instructions.
+	 *
+	 * We use SYSENTER_ESP to find sp0 and for the NMI emergency
+	 * stack, but we need to context switch it because we do
+	 * horrible things to the kernel stack in vm86 mode.
+	 *
+	 * We use SYSENTER_CS to disable sysenter in vm86 mode to avoid
+	 * corrupting the stack if we went through the sysenter path
+	 * from vm86 mode.
+	 */
+	unsigned long		sp1;	/* MSR_IA32_SYSENTER_ESP */
+	unsigned short		ss1;	/* MSR_IA32_SYSENTER_CS */
+
+	unsigned short		__ss1h;
 	unsigned long		sp2;
 	unsigned short		ss2, __ss2h;
 	unsigned long		__cr3;

Re: [tip:x86/asm] x86/asm/entry/32: Document our abuse of x86_hw_tss: :ss1 and x86_hw_tss::sp1

[Andy Lutomirski]

On Mon, Mar 16, 2015 at 5:09 AM, tip-bot for Andy Lutomirski
<tipbot@zytor.com> wrote:
> Commit-ID:  f7113ffa3bcd630066119723f539db75a5721c88
> Gitweb:     http://git.kernel.org/tip/f7113ffa3bcd630066119723f539db75a5721c88
> Author:     Andy Lutomirski <luto@amacapital.net>
> AuthorDate: Tue, 10 Mar 2015 11:06:00 -0700
> Committer:  Ingo Molnar <mingo@kernel.org>
> CommitDate: Mon, 16 Mar 2015 11:05:36 +0100
>
> x86/asm/entry/32: Document our abuse of x86_hw_tss::ss1 and x86_hw_tss::sp1
>
> This has confused me for a while.  Now that I figured it out, document it.
>
> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Denys Vlasenko <dvlasenk@redhat.com>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Link: http://lkml.kernel.org/r/b7efc1b7364039824776f68e9ddee9ec1500e894.1426009661.git.luto@amacapital.net
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> ---
>  arch/x86/include/asm/processor.h | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index aed6d4f..c3a037b 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -209,9 +209,24 @@ struct x86_hw_tss {
>         unsigned short          back_link, __blh;
>         unsigned long           sp0;
>         unsigned short          ss0, __ss0h;
> -       unsigned long           sp1;
> -       /* ss1 caches MSR_IA32_SYSENTER_CS: */
> -       unsigned short          ss1, __ss1h;
> +
> +       /*
> +        * We don't use ring 1, so sp1 and ss1 are convenient scratch
> +        * spaces in the same cacheline as sp0.  We use them to cache
> +        * some MSR values to avoid unnecessary wrmsr instructions.
> +        *
> +        * We use SYSENTER_ESP to find sp0 and for the NMI emergency
> +        * stack, but we need to context switch it because we do
> +        * horrible things to the kernel stack in vm86 mode.

I should have sent out my v2 sooner.  Denys correctly noted that the
sp1 cache is completely useless.  I'll send an incremental patch
today.

--Andy

> +        *
> +        * We use SYSENTER_CS to disable sysenter in vm86 mode to avoid
> +        * corrupting the stack if we went through the sysenter path
> +        * from vm86 mode.
> +        */
> +       unsigned long           sp1;    /* MSR_IA32_SYSENTER_ESP */
> +       unsigned short          ss1;    /* MSR_IA32_SYSENTER_CS */
> +
> +       unsigned short          __ss1h;
>         unsigned long           sp2;
>         unsigned short          ss2, __ss2h;
>         unsigned long           __cr3;



-- 
Andy Lutomirski
AMA Capital Management, LLC

[tip:x86/asm] x86/asm/entry: Create and use a ' TOP_OF_KERNEL_STACK_PADDING' macro

[tip-bot for Andy Lutomirski]

Commit-ID:  3ee4298f440c81638cbb5ec06f2497fb7a9a9eb4
Gitweb:     http://git.kernel.org/tip/3ee4298f440c81638cbb5ec06f2497fb7a9a9eb4
Author:     Andy Lutomirski <luto@amacapital.net>
AuthorDate: Tue, 10 Mar 2015 11:05:58 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 17 Mar 2015 09:25:26 +0100

x86/asm/entry: Create and use a 'TOP_OF_KERNEL_STACK_PADDING' macro

x86_32, unlike x86_64, pads the top of the kernel stack, because the
hardware stack frame formats are variable in size.

Document this padding and give it a name.

This should make no change whatsoever to the compiled kernel
image. It also doesn't fix any of the current bugs in this area.

Signed-off-by: Andy Lutomirski <luto@amacapital.net>
Acked-by: Denys Vlasenko <dvlasenk@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/02bf2f54b8dcb76a62a142b6dfe07d4ef7fc582e.1426009661.git.luto@amacapital.net
[ Fixed small details, such as a missed magic constant in entry_32.S pointed out by Denys Vlasenko. ]
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/processor.h   |  3 ++-
 arch/x86/include/asm/thread_info.h | 27 +++++++++++++++++++++++++++
 arch/x86/kernel/entry_32.S         |  2 +-
 3 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 48a61c1..88d9aa7 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -849,7 +849,8 @@ extern unsigned long thread_saved_pc(struct task_struct *tsk);
 #define task_pt_regs(task)                                             \
 ({                                                                     \
        struct pt_regs *__regs__;                                       \
-       __regs__ = (struct pt_regs *)(KSTK_TOP(task_stack_page(task))-8); \
+       __regs__ = (struct pt_regs *)(KSTK_TOP(task_stack_page(task)) - \
+				     TOP_OF_KERNEL_STACK_PADDING);     \
        __regs__ - 1;                                                   \
 })
 
diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
index 7740edd..ba115eb 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -13,6 +13,33 @@
 #include <asm/types.h>
 
 /*
+ * TOP_OF_KERNEL_STACK_PADDING is a number of unused bytes that we
+ * reserve at the top of the kernel stack.  We do it because of a nasty
+ * 32-bit corner case.  On x86_32, the hardware stack frame is
+ * variable-length.  Except for vm86 mode, struct pt_regs assumes a
+ * maximum-length frame.  If we enter from CPL 0, the top 8 bytes of
+ * pt_regs don't actually exist.  Ordinarily this doesn't matter, but it
+ * does in at least one case:
+ *
+ * If we take an NMI early enough in SYSENTER, then we can end up with
+ * pt_regs that extends above sp0.  On the way out, in the espfix code,
+ * we can read the saved SS value, but that value will be above sp0.
+ * Without this offset, that can result in a page fault.  (We are
+ * careful that, in this case, the value we read doesn't matter.)
+ *
+ * In vm86 mode, the hardware frame is much longer still, but we neither
+ * access the extra members from NMI context, nor do we write such a
+ * frame at sp0 at all.
+ *
+ * x86_64 has a fixed-length stack frame.
+ */
+#ifdef CONFIG_X86_32
+# define TOP_OF_KERNEL_STACK_PADDING 8
+#else
+# define TOP_OF_KERNEL_STACK_PADDING 0
+#endif
+
+/*
  * low level task data that entry.S needs immediate access to
  * - this struct should fit entirely inside of one cache line
  * - this struct shares the supervisor stack pages
diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
index e33ba51..4c8cc34 100644
--- a/arch/x86/kernel/entry_32.S
+++ b/arch/x86/kernel/entry_32.S
@@ -398,7 +398,7 @@ sysenter_past_esp:
 	 * A tiny bit of offset fixup is necessary - 4*4 means the 4 words
 	 * pushed above; +8 corresponds to copy_thread's esp0 setting.
 	 */
-	pushl_cfi ((TI_sysenter_return)-THREAD_SIZE+8+4*4)(%esp)
+	pushl_cfi ((TI_sysenter_return)-THREAD_SIZE+TOP_OF_KERNEL_STACK_PADDING+4*4)(%esp)
 	CFI_REL_OFFSET eip, 0
 
 	pushl_cfi %eax

[tip:x86/asm] x86/asm/entry: Unify and fix initial thread_struct: :sp0 values

[tip-bot for Andy Lutomirski]

Commit-ID:  d9e05cc5a53246e074dc2b84956252e4bbe392cd
Gitweb:     http://git.kernel.org/tip/d9e05cc5a53246e074dc2b84956252e4bbe392cd
Author:     Andy Lutomirski <luto@amacapital.net>
AuthorDate: Tue, 10 Mar 2015 11:05:59 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 17 Mar 2015 09:25:27 +0100

x86/asm/entry: Unify and fix initial thread_struct::sp0 values

x86_32 and x86_64 need slightly different thread_struct::sp0 values, and
x86_32's was incorrect for init.

This never mattered -- the init thread never runs user code, so we never
used thread_struct::sp0 for anything.

Fix it and mostly unify them.

Signed-off-by: Andy Lutomirski <luto@amacapital.net>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/1b810c1d2e797e27bb4a7708c426101161edd1f6.1426009661.git.luto@amacapital.net
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/processor.h | 7 +++++--
 arch/x86/kernel/process.c        | 2 +-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 88d9aa7..fc6d8d0 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -811,6 +811,9 @@ static inline void spin_lock_prefetch(const void *x)
 	prefetchw(x);
 }
 
+#define TOP_OF_INIT_STACK ((unsigned long)&init_stack + sizeof(init_stack) - \
+			   TOP_OF_KERNEL_STACK_PADDING)
+
 #ifdef CONFIG_X86_32
 /*
  * User space process size: 3GB (default).
@@ -821,7 +824,7 @@ static inline void spin_lock_prefetch(const void *x)
 #define STACK_TOP_MAX		STACK_TOP
 
 #define INIT_THREAD  {							  \
-	.sp0			= sizeof(init_stack) + (long)&init_stack, \
+	.sp0			= TOP_OF_INIT_STACK,			  \
 	.vm86_info		= NULL,					  \
 	.sysenter_cs		= __KERNEL_CS,				  \
 	.io_bitmap_ptr		= NULL,					  \
@@ -883,7 +886,7 @@ extern unsigned long thread_saved_pc(struct task_struct *tsk);
 #define STACK_TOP_MAX		TASK_SIZE_MAX
 
 #define INIT_THREAD  { \
-	.sp0 = (unsigned long)&init_stack + sizeof(init_stack) \
+	.sp0 = TOP_OF_INIT_STACK \
 }
 
 /*
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index f4c0af7..12b1cf6 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -39,7 +39,7 @@
  */
 __visible DEFINE_PER_CPU_SHARED_ALIGNED(struct tss_struct, cpu_tss) = {
 	.x86_tss = {
-		.sp0 = (unsigned long)&init_stack + sizeof(init_stack),
+		.sp0 = TOP_OF_INIT_STACK,
 #ifdef CONFIG_X86_32
 		.ss0 = __KERNEL_DS,
 		.ss1 = __KERNEL_CS,

[tip:x86/asm] x86/asm/entry/32: Document our abuse of x86_hw_tss: :ss1 and x86_hw_tss::sp1

[tip-bot for Andy Lutomirski]

Commit-ID:  76e4c4908a4904a61aa67ae5eb0b2a7588c4a546
Gitweb:     http://git.kernel.org/tip/76e4c4908a4904a61aa67ae5eb0b2a7588c4a546
Author:     Andy Lutomirski <luto@amacapital.net>
AuthorDate: Tue, 10 Mar 2015 11:06:00 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 17 Mar 2015 09:25:27 +0100

x86/asm/entry/32: Document our abuse of x86_hw_tss::ss1 and x86_hw_tss::sp1

This has confused me for a while.  Now that I figured it out, document it.

Signed-off-by: Andy Lutomirski <luto@amacapital.net>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/b7efc1b7364039824776f68e9ddee9ec1500e894.1426009661.git.luto@amacapital.net
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/processor.h | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index fc6d8d0..b262089 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -209,9 +209,24 @@ struct x86_hw_tss {
 	unsigned short		back_link, __blh;
 	unsigned long		sp0;
 	unsigned short		ss0, __ss0h;
-	unsigned long		sp1;
-	/* ss1 caches MSR_IA32_SYSENTER_CS: */
-	unsigned short		ss1, __ss1h;
+
+	/*
+	 * We don't use ring 1, so sp1 and ss1 are convenient scratch
+	 * spaces in the same cacheline as sp0.  We use them to cache
+	 * some MSR values to avoid unnecessary wrmsr instructions.
+	 *
+	 * We use SYSENTER_ESP to find sp0 and for the NMI emergency
+	 * stack, but we need to context switch it because we do
+	 * horrible things to the kernel stack in vm86 mode.
+	 *
+	 * We use SYSENTER_CS to disable sysenter in vm86 mode to avoid
+	 * corrupting the stack if we went through the sysenter path
+	 * from vm86 mode.
+	 */
+	unsigned long		sp1;	/* MSR_IA32_SYSENTER_ESP */
+	unsigned short		ss1;	/* MSR_IA32_SYSENTER_CS */
+
+	unsigned short		__ss1h;
 	unsigned long		sp2;
 	unsigned short		ss2, __ss2h;
 	unsigned long		__cr3;