From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753067Ab1BECYq (ORCPT ); Fri, 4 Feb 2011 21:24:46 -0500 Received: from smtp.outflux.net ([198.145.64.163]:55007 "EHLO smtp.outflux.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752536Ab1BECYp (ORCPT ); Fri, 4 Feb 2011 21:24:45 -0500 Date: Fri, 4 Feb 2011 18:19:22 -0800 From: Kees Cook To: mingo@redhat.com, hpa@zytor.com, linux-kernel@vger.kernel.org, castet.matthieu@free.fr, tglx@linutronix.de, hpa@linux.intel.com Subject: Re: [tip:x86/urgent] x86-32: Make sure the stack is set up before we use it Message-ID: <20110205021922.GP5503@outflux.net> References: <4D41E86D.8060205@free.fr> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Organization: Canonical X-HELO: www.outflux.net Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Peter, This works for me doing CPU hotplugging on ia32, but on x86_64 it hangs. (Both boot, though.) Thanks, -Kees On Sat, Feb 05, 2011 at 12:45:54AM +0000, tip-bot for H. Peter Anvin wrote: > Commit-ID: 43d79a250bdb10e30b08a79e906befc691c3f5ce > Gitweb: http://git.kernel.org/tip/43d79a250bdb10e30b08a79e906befc691c3f5ce > Author: H. Peter Anvin > AuthorDate: Fri, 4 Feb 2011 16:14:11 -0800 > Committer: H. Peter Anvin > CommitDate: Fri, 4 Feb 2011 16:38:35 -0800 > > x86-32: Make sure the stack is set up before we use it > > Since checkin ebba638ae723d8a8fc2f7abce5ec18b688b791d7 we call > verify_cpu even in 32-bit mode. Unfortunately, calling a function > means using the stack, and the stack pointer was not initialized in > the 32-bit setup code! This code initializes the stack pointer, and > simplifies the interface slightly since it is easier to rely on just a > pointer value rather than a descriptor; we need to have different > values for the segment register anyway. > > This retains start_stack as a virtual address, even though a physical > address would be more convenient for 32 bits; the 64-bit code wants > the other way around... > > Reported-by: Matthieu Castet > LKML-Reference: <4D41E86D.8060205@free.fr> > Cc: Kees Cook > Signed-off-by: H. Peter Anvin > --- > arch/x86/include/asm/smp.h | 5 +---- > arch/x86/kernel/acpi/sleep.c | 2 +- > arch/x86/kernel/head_32.S | 30 +++++++++++++----------------- > arch/x86/kernel/head_64.S | 1 - > arch/x86/kernel/smpboot.c | 4 ++-- > 5 files changed, 17 insertions(+), 25 deletions(-) > > diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h > index 4c2f63c..1f46951 100644 > --- a/arch/x86/include/asm/smp.h > +++ b/arch/x86/include/asm/smp.h > @@ -40,10 +40,7 @@ DECLARE_EARLY_PER_CPU(u16, x86_cpu_to_apicid); > DECLARE_EARLY_PER_CPU(u16, x86_bios_cpu_apicid); > > /* Static state in head.S used to set up a CPU */ > -extern struct { > - void *sp; > - unsigned short ss; > -} stack_start; > +extern unsigned long stack_start; /* Initial stack pointer address */ > > struct smp_ops { > void (*smp_prepare_boot_cpu)(void); > diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c > index 69fd72a..4d9ebba 100644 > --- a/arch/x86/kernel/acpi/sleep.c > +++ b/arch/x86/kernel/acpi/sleep.c > @@ -100,7 +100,7 @@ int acpi_save_state_mem(void) > #else /* CONFIG_64BIT */ > header->trampoline_segment = setup_trampoline() >> 4; > #ifdef CONFIG_SMP > - stack_start.sp = temp_stack + sizeof(temp_stack); > + stack_start = (unsigned long)temp_stack + sizeof(temp_stack); > early_gdt_descr.address = > (unsigned long)get_cpu_gdt_table(smp_processor_id()); > initial_gs = per_cpu_offset(smp_processor_id()); > diff --git a/arch/x86/kernel/head_32.S b/arch/x86/kernel/head_32.S > index fc293dc..767d6c4 100644 > --- a/arch/x86/kernel/head_32.S > +++ b/arch/x86/kernel/head_32.S > @@ -85,6 +85,8 @@ RESERVE_BRK(pagetables, INIT_MAP_SIZE) > */ > __HEAD > ENTRY(startup_32) > + movl pa(stack_start),%ecx > + > /* test KEEP_SEGMENTS flag to see if the bootloader is asking > us to not reload segments */ > testb $(1<<6), BP_loadflags(%esi) > @@ -99,7 +101,9 @@ ENTRY(startup_32) > movl %eax,%es > movl %eax,%fs > movl %eax,%gs > + movl %eax,%ss > 2: > + leal -__PAGE_OFFSET(%ecx),%esp > > /* > * Clear BSS first so that there are no surprises... > @@ -145,8 +149,6 @@ ENTRY(startup_32) > * _brk_end is set up to point to the first "safe" location. > * Mappings are created both at virtual address 0 (identity mapping) > * and PAGE_OFFSET for up to _end. > - * > - * Note that the stack is not yet set up! > */ > #ifdef CONFIG_X86_PAE > > @@ -282,6 +284,9 @@ ENTRY(startup_32_smp) > movl %eax,%es > movl %eax,%fs > movl %eax,%gs > + movl pa(stack_start),%ecx > + movl %eax,%ss > + leal -__PAGE_OFFSET(%ecx),%esp > #endif /* CONFIG_SMP */ > default_entry: > > @@ -347,8 +352,8 @@ default_entry: > movl %eax,%cr0 /* ..and set paging (PG) bit */ > ljmp $__BOOT_CS,$1f /* Clear prefetch and normalize %eip */ > 1: > - /* Set up the stack pointer */ > - lss stack_start,%esp > + /* Shift the stack pointer to a virtual address */ > + addl $__PAGE_OFFSET, %esp > > /* > * Initialize eflags. Some BIOS's leave bits like NT set. This would > @@ -360,9 +365,7 @@ default_entry: > > #ifdef CONFIG_SMP > cmpb $0, ready > - jz 1f /* Initial CPU cleans BSS */ > - jmp checkCPUtype > -1: > + jnz checkCPUtype > #endif /* CONFIG_SMP */ > > /* > @@ -470,14 +473,7 @@ is386: movl $2,%ecx # set MP > > cld # gcc2 wants the direction flag cleared at all times > pushl $0 # fake return address for unwinder > -#ifdef CONFIG_SMP > - movb ready, %cl > movb $1, ready > - cmpb $0,%cl # the first CPU calls start_kernel > - je 1f > - movl (stack_start), %esp > -1: > -#endif /* CONFIG_SMP */ > jmp *(initial_code) > > /* > @@ -670,15 +666,15 @@ ENTRY(initial_page_table) > #endif > > .data > +.balign 4 > ENTRY(stack_start) > .long init_thread_union+THREAD_SIZE > - .long __BOOT_DS > - > -ready: .byte 0 > > early_recursion_flag: > .long 0 > > +ready: .byte 0 > + > int_msg: > .asciz "Unknown interrupt or fault at: %p %p %p\n" > > diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S > index 239046b..1ee8493 100644 > --- a/arch/x86/kernel/head_64.S > +++ b/arch/x86/kernel/head_64.S > @@ -264,7 +264,6 @@ ENTRY(secondary_startup_64) > > ENTRY(stack_start) > .quad init_thread_union+THREAD_SIZE-8 > - .word 0 > __FINITDATA > > bad_address: > diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c > index 0cbe8c0..03273b6 100644 > --- a/arch/x86/kernel/smpboot.c > +++ b/arch/x86/kernel/smpboot.c > @@ -638,7 +638,7 @@ wakeup_secondary_cpu_via_init(int phys_apicid, unsigned long start_eip) > * target processor state. > */ > startup_ipi_hook(phys_apicid, (unsigned long) start_secondary, > - (unsigned long)stack_start.sp); > + stack_start); > > /* > * Run STARTUP IPI loop. > @@ -785,7 +785,7 @@ do_rest: > #endif > early_gdt_descr.address = (unsigned long)get_cpu_gdt_table(cpu); > initial_code = (unsigned long)start_secondary; > - stack_start.sp = (void *) c_idle.idle->thread.sp; > + stack_start = c_idle.idle->thread.sp; > > /* start_ip had better be page-aligned! */ > start_ip = setup_trampoline(); -- Kees Cook Ubuntu Security Team