Re: [PATCH v2 10/18] x86/asm: Remap the TSS into the cpu entry area

[Borislav Petkov <bp@suse.de>]

On Tue, Nov 21, 2017 at 08:44:05PM -0800, Andy Lutomirski wrote:
> This has a secondary purpose: it puts the entry stack into a region
> with a well-controlled layout.  A subsequent patch will take
> advantage of this to streamline the SYSCALL entry code to be able to
> find it more easily.
> 
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  arch/x86/entry/entry_32.S     |  6 ++++--
>  arch/x86/include/asm/fixmap.h |  7 +++++++
>  arch/x86/kernel/asm-offsets.c |  3 +++
>  arch/x86/kernel/cpu/common.c  | 40 ++++++++++++++++++++++++++++++++++------
>  arch/x86/kernel/dumpstack.c   |  3 ++-
>  arch/x86/power/cpu.c          | 11 ++++++-----
>  6 files changed, 56 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
> index 4838037f97f6..0ab316c46806 100644
> --- a/arch/x86/entry/entry_32.S
> +++ b/arch/x86/entry/entry_32.S
> @@ -941,7 +941,8 @@ ENTRY(debug)
>  	movl	%esp, %eax			# pt_regs pointer
>  
>  	/* Are we currently on the SYSENTER stack? */
> -	PER_CPU(cpu_tss + CPU_TSS_SYSENTER_stack + SIZEOF_SYSENTER_stack, %ecx)
> +	movl	PER_CPU_VAR(cpu_entry_area), %ecx
> +	addl	$CPU_ENTRY_AREA_tss + CPU_TSS_SYSENTER_stack + SIZEOF_SYSENTER_stack, %ecx
>  	subl	%eax, %ecx	/* ecx = (end of SYSENTER_stack) - esp */

Fixup that comment while you're at it to be either ontop or be of the
#-type. The cool thing about the # comments is that gas puts them in the
.s output too, so you can find them again.

>  	cmpl	$SIZEOF_SYSENTER_stack, %ecx
>  	jb	.Ldebug_from_sysenter_stack
> @@ -984,7 +985,8 @@ ENTRY(nmi)
>  	movl	%esp, %eax			# pt_regs pointer
>  
>  	/* Are we currently on the SYSENTER stack? */
> -	PER_CPU(cpu_tss + CPU_TSS_SYSENTER_stack + SIZEOF_SYSENTER_stack, %ecx)
> +	movl	PER_CPU_VAR(cpu_entry_area), %ecx
> +	addl	$CPU_ENTRY_AREA_tss + CPU_TSS_SYSENTER_stack + SIZEOF_SYSENTER_stack, %ecx
>  	subl	%eax, %ecx	/* ecx = (end of SYSENTER_stack) - esp */

Ditto.

>  	cmpl	$SIZEOF_SYSENTER_stack, %ecx
>  	jb	.Lnmi_from_sysenter_stack

...

> diff --git a/arch/x86/kernel/asm-offsets.c b/arch/x86/kernel/asm-offsets.c
> index b275863128eb..55858b277cf6 100644
> --- a/arch/x86/kernel/asm-offsets.c
> +++ b/arch/x86/kernel/asm-offsets.c
> @@ -98,4 +98,7 @@ void common(void) {
>  	OFFSET(CPU_TSS_SYSENTER_stack, tss_struct, SYSENTER_stack);
>  	/* Size of SYSENTER_stack */
>  	DEFINE(SIZEOF_SYSENTER_stack, sizeof(((struct tss_struct *)0)->SYSENTER_stack));
> +
> +	/* Layout info for cpu_entry_area */
> +	OFFSET(CPU_ENTRY_AREA_tss, cpu_entry_area, tss);

The naming convention of those looks really strange. The beginning is in
all caps and then small. Can we choose a convention pls.

And the comments are not really needed - the variable names already say
what they are.

...

> @@ -1281,7 +1304,8 @@ void enable_sep_cpu(void)
>  	wrmsr(MSR_IA32_SYSENTER_CS, tss->x86_tss.ss1, 0);
>  
>  	wrmsr(MSR_IA32_SYSENTER_ESP,
> -	      (unsigned long)tss + offsetofend(struct tss_struct, SYSENTER_stack),
> +	      (unsigned long)&get_cpu_entry_area(cpu)->tss +
> +	      offsetofend(struct tss_struct, SYSENTER_stack),
>  	      0);

Use a local var:

	unsigned long se_esp;

	...

	se_esp = (unsigned long)&get_cpu_entry_area(cpu)->tss + offsetofend(struct tss_struct, SYSENTER_stack);

	wrmsr(MSR_IA32_SYSENTER_ESP, se_esp, 0);

>  
>  	wrmsr(MSR_IA32_SYSENTER_EIP, (unsigned long)entry_SYSENTER_32, 0);
> @@ -1394,6 +1418,8 @@ static DEFINE_PER_CPU_PAGE_ALIGNED(char, exception_stacks
>  /* May not be marked __init: used by software suspend */
>  void syscall_init(void)
>  {
> +	int cpu = smp_processor_id();
> +
>  	wrmsr(MSR_STAR, 0, (__USER32_CS << 16) | __KERNEL_CS);
>  	wrmsrl(MSR_LSTAR, (unsigned long)entry_SYSCALL_64);
>  
> @@ -1407,7 +1433,7 @@ void syscall_init(void)
>  	 */
>  	wrmsrl_safe(MSR_IA32_SYSENTER_CS, (u64)__KERNEL_CS);
>  	wrmsrl_safe(MSR_IA32_SYSENTER_ESP,
> -		    (unsigned long)this_cpu_ptr(&cpu_tss) +
> +		    (unsigned long)&get_cpu_entry_area(cpu)->tss +
>  		    offsetofend(struct tss_struct, SYSENTER_stack));

Ditto.

>  	wrmsrl_safe(MSR_IA32_SYSENTER_EIP, (u64)entry_SYSENTER_compat);
>  #else

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--