[PATCH] x86: make 32-bit "emergency stack" better documented

[PATCH] x86: make 32-bit "emergency stack" better documented

[Denys Vlasenko]

Before the patch, tss.stack field was not referenced anywhere.
It was used only by setting sysenter's stack to point after
last byte of tss, thus the trailing field, stack[64], was used.
But grep would not know it. You can comment it out, compile,
and kernel will even run until an unlucky NMI corrupts
iomap[] (which is also not easily detectable).

This patch changes code so the purpose and usage of this field
is not mysterious anymore, and can be easily grepped for.

This does change generated code, for a subtle reason:
since tss_struct is ____cacheline_aligned, there happen to be
5 longs of padding at the end. Old code was using the padding too;
new code will strictly use only SYSENTER_stack[].

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
CC: Linus Torvalds <torvalds@linux-foundation.org>
CC: Steven Rostedt <rostedt@goodmis.org>
CC: Ingo Molnar <mingo@kernel.org>
CC: Borislav Petkov <bp@alien8.de>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Andy Lutomirski <luto@amacapital.net>
CC: Oleg Nesterov <oleg@redhat.com>
CC: Frederic Weisbecker <fweisbec@gmail.com>
CC: Alexei Starovoitov <ast@plumgrid.com>
CC: Will Drewry <wad@chromium.org>
CC: Kees Cook <keescook@chromium.org>
CC: x86@kernel.org
CC: linux-kernel@vger.kernel.org

---
 arch/x86/include/asm/processor.h | 5 +++--
 arch/x86/kernel/asm-offsets_32.c | 2 +-
 arch/x86/kernel/cpu/common.c     | 2 +-
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 48a61c1..4172241 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -276,9 +276,10 @@ struct tss_struct {
 	unsigned long		io_bitmap[IO_BITMAP_LONGS + 1];
 
 	/*
-	 * .. and then another 0x100 bytes for the emergency kernel stack:
+	 * and then space for temporary SYSENTER stack:
 	 */
-	unsigned long		stack[64];
+	unsigned long		SYSENTER_stack[63];
+	unsigned long		SYSENTER_stack_last_word;
 
 } ____cacheline_aligned;
 
diff --git a/arch/x86/kernel/asm-offsets_32.c b/arch/x86/kernel/asm-offsets_32.c
index 3b3b9d3..4a3cbd0 100644
--- a/arch/x86/kernel/asm-offsets_32.c
+++ b/arch/x86/kernel/asm-offsets_32.c
@@ -68,7 +68,7 @@ void foo(void)
 
 	/* Offset from the sysenter stack to tss.sp0 */
 	DEFINE(TSS_sysenter_sp0, offsetof(struct tss_struct, x86_tss.sp0) -
-		 sizeof(struct tss_struct));
+		(offsetof(struct tss_struct, SYSENTER_stack_last_word) + 4));
 
 #if defined(CONFIG_LGUEST) || defined(CONFIG_LGUEST_GUEST) || defined(CONFIG_LGUEST_MODULE)
 	BLANK();
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 7634833..8180e0b 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -987,7 +987,7 @@ void enable_sep_cpu(void)
 	}
 
 	tss->x86_tss.ss1 = __KERNEL_CS;
-	tss->x86_tss.sp1 = sizeof(struct tss_struct) + (unsigned long) tss;
+	tss->x86_tss.sp1 = ((unsigned long) &tss->SYSENTER_stack_last_word) + 4;
 	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);
-- 
1.8.1.4

Re: [PATCH] x86: make 32-bit "emergency stack" better documented

[Andy Lutomirski]

On Mon, Mar 9, 2015 at 6:28 AM, Denys Vlasenko <dvlasenk@redhat.com> wrote:
> Before the patch, tss.stack field was not referenced anywhere.
> It was used only by setting sysenter's stack to point after
> last byte of tss, thus the trailing field, stack[64], was used.
> But grep would not know it. You can comment it out, compile,
> and kernel will even run until an unlucky NMI corrupts
> iomap[] (which is also not easily detectable).
>
> This patch changes code so the purpose and usage of this field
> is not mysterious anymore, and can be easily grepped for.
>
> This does change generated code, for a subtle reason:
> since tss_struct is ____cacheline_aligned, there happen to be
> 5 longs of padding at the end. Old code was using the padding too;
> new code will strictly use only SYSENTER_stack[].
>
> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> CC: Linus Torvalds <torvalds@linux-foundation.org>
> CC: Steven Rostedt <rostedt@goodmis.org>
> CC: Ingo Molnar <mingo@kernel.org>
> CC: Borislav Petkov <bp@alien8.de>
> CC: "H. Peter Anvin" <hpa@zytor.com>
> CC: Andy Lutomirski <luto@amacapital.net>
> CC: Oleg Nesterov <oleg@redhat.com>
> CC: Frederic Weisbecker <fweisbec@gmail.com>
> CC: Alexei Starovoitov <ast@plumgrid.com>
> CC: Will Drewry <wad@chromium.org>
> CC: Kees Cook <keescook@chromium.org>
> CC: x86@kernel.org
> CC: linux-kernel@vger.kernel.org
>
> ---
>  arch/x86/include/asm/processor.h | 5 +++--
>  arch/x86/kernel/asm-offsets_32.c | 2 +-
>  arch/x86/kernel/cpu/common.c     | 2 +-
>  3 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index 48a61c1..4172241 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -276,9 +276,10 @@ struct tss_struct {
>         unsigned long           io_bitmap[IO_BITMAP_LONGS + 1];
>
>         /*
> -        * .. and then another 0x100 bytes for the emergency kernel stack:
> +        * and then space for temporary SYSENTER stack:
>          */
> -       unsigned long           stack[64];
> +       unsigned long           SYSENTER_stack[63];
> +       unsigned long           SYSENTER_stack_last_word;
>
>  } ____cacheline_aligned;
>
> diff --git a/arch/x86/kernel/asm-offsets_32.c b/arch/x86/kernel/asm-offsets_32.c
> index 3b3b9d3..4a3cbd0 100644
> --- a/arch/x86/kernel/asm-offsets_32.c
> +++ b/arch/x86/kernel/asm-offsets_32.c
> @@ -68,7 +68,7 @@ void foo(void)
>
>         /* Offset from the sysenter stack to tss.sp0 */
>         DEFINE(TSS_sysenter_sp0, offsetof(struct tss_struct, x86_tss.sp0) -
> -                sizeof(struct tss_struct));
> +               (offsetof(struct tss_struct, SYSENTER_stack_last_word) + 4));

Is there no way to do this using sizeof to avoid having a "last_word" member.

--Andy

Re: [PATCH] x86: make 32-bit "emergency stack" better documented

[Denys Vlasenko]

On Mon, Mar 9, 2015 at 2:43 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>>         /* Offset from the sysenter stack to tss.sp0 */
>>         DEFINE(TSS_sysenter_sp0, offsetof(struct tss_struct, x86_tss.sp0) -
>> -                sizeof(struct tss_struct));
>> +               (offsetof(struct tss_struct, SYSENTER_stack_last_word) + 4));
>
> Is there no way to do this using sizeof to avoid having a "last_word" member.

sizeof of a structure member? IIRC in C sizeof can't be used for that.
You need to concoct a fictitious object, dereference its member
and take sizeof of _that_.

It would look horrible. Something like:

offsetof(struct tss_struct, SYSENTER_stack) +
                sizeof(((struct tss_struct*)NULL)->SYSENTER_stack)

Re: [PATCH] x86: make 32-bit "emergency stack" better documented

[Andy Lutomirski]

On Mon, Mar 9, 2015 at 7:14 AM, Denys Vlasenko <vda.linux@googlemail.com> wrote:
> On Mon, Mar 9, 2015 at 2:43 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>>>         /* Offset from the sysenter stack to tss.sp0 */
>>>         DEFINE(TSS_sysenter_sp0, offsetof(struct tss_struct, x86_tss.sp0) -
>>> -                sizeof(struct tss_struct));
>>> +               (offsetof(struct tss_struct, SYSENTER_stack_last_word) + 4));
>>
>> Is there no way to do this using sizeof to avoid having a "last_word" member.
>
> sizeof of a structure member? IIRC in C sizeof can't be used for that.
> You need to concoct a fictitious object, dereference its member
> and take sizeof of _that_.
>
> It would look horrible. Something like:
>
> offsetof(struct tss_struct, SYSENTER_stack) +
>                 sizeof(((struct tss_struct*)NULL)->SYSENTER_stack)

vfio.h has:

/**
 * offsetofend(TYPE, MEMBER)
 *
 * @TYPE: The type of the structure
 * @MEMBER: The member within the structure to get the end offset of
 *
 * Simple helper macro for dealing with variable sized structures passed
 * from user space.  This allows us to easily determine if the provided
 * structure is sized to include various fields.
 */
#define offsetofend(TYPE, MEMBER) \
        (offsetof(TYPE, MEMBER) + sizeof(((TYPE *)0)->MEMBER))

we could stick that in stdef.h and use it.

--Andy