[BUG] broken ebba638ae723d8a8fc2f7abce5ec18b688b791d7

[BUG] broken ebba638ae723d8a8fc2f7abce5ec18b688b791d7

[matthieu castet]

Hi,

ebba638ae723d8a8fc2f7abce5ec18b688b791d7 x86, cpu: Call verify_cpu during 32bit CPU startup look buggy.

It add a call to verify_cpu, but we never set the stack before (I check with qemu + gdbserver that sp is random
when doing cpu hotplug).
This mean do randomly corrupt the memory.

Matthieu

Re: [BUG] broken ebba638ae723d8a8fc2f7abce5ec18b688b791d7

[Kees Cook]

Hi matthieu,

On Thu, Jan 27, 2011 at 10:49:33PM +0100, matthieu castet wrote:
> ebba638ae723d8a8fc2f7abce5ec18b688b791d7 x86, cpu: Call verify_cpu during 32bit CPU startup look buggy.
> 
> It add a call to verify_cpu, but we never set the stack before (I check with qemu + gdbserver that sp is random
> when doing cpu hotplug).
> This mean do randomly corrupt the memory.

Yikes, good catch.

arch/x86/kernel/trampoline_64.S uses:
        movw    $(trampoline_stack_end - r_base), %sp

arch/x86/boot/compressed/head_64.S uses:
        movl    $boot_stack_end, %eax
        addl    %ebp, %eax
        movl    %eax, %esp

what would be safe for arch/x86/kernel/head_32.S ? It uses "stack_start",
but later after paging set-up. Is the following sane to solve this?


diff --git a/arch/x86/kernel/head_32.S b/arch/x86/kernel/head_32.S
index fc293dc..8ddd0e4 100644
--- a/arch/x86/kernel/head_32.S
+++ b/arch/x86/kernel/head_32.S
@@ -284,6 +284,8 @@ ENTRY(startup_32_smp)
 	movl %eax,%gs
 #endif /* CONFIG_SMP */
 default_entry:
+	/* Set up the stack pointer */
+	lss stack_start,%esp
 
 /*
  *	New page tables may be in 4Mbyte page mode and may
@@ -347,8 +349,6 @@ 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
 
 /*
  * Initialize eflags.  Some BIOS's leave bits like NT set.  This would

-Kees


-- 
Kees Cook
Ubuntu Security Team

Re: [BUG] broken ebba638ae723d8a8fc2f7abce5ec18b688b791d7

[H. Peter Anvin]

On 01/27/2011 03:00 PM, Kees Cook wrote:
>
> Yikes, good catch.
>
> arch/x86/kernel/trampoline_64.S uses:
>          movw    $(trampoline_stack_end - r_base), %sp
>
> arch/x86/boot/compressed/head_64.S uses:
>          movl    $boot_stack_end, %eax
>          addl    %ebp, %eax
>          movl    %eax, %esp
>
> what would be safe for arch/x86/kernel/head_32.S ? It uses "stack_start",
> but later after paging set-up. Is the following sane to solve this?
>

To run it before paging is set up, you can't use stack, start; you have 
to use a pointer based on physical address.  You have two problems with 
using stack_start: you're using a linear address to access stack_start, 
and stack_start itself contains a linear address.

It's not entirely clear to me why we don't initialize %ss to __BOOT_DS 
with the other segment registers, but it would make most sense to me:

diff --git a/arch/x86/kernel/head_32.S b/arch/x86/kernel/head_32.S
index fc293dc..c10f9ba 100644
--- a/arch/x86/kernel/head_32.S
+++ b/arch/x86/kernel/head_32.S
@@ -99,7 +99,12 @@ ENTRY(startup_32)
         movl %eax,%es
         movl %eax,%fs
         movl %eax,%gs
+       movl %eax,%ss
  2:
+/*
+ * Set up an initial stack
+ */
+       movl $pa(init_thread_union+THREAD_SIZE), %esp

  /*
   * Clear BSS first so that there are no surprises...

	-hpa

Re: [BUG] broken ebba638ae723d8a8fc2f7abce5ec18b688b791d7

[H. Peter Anvin]

[Adding Jeremy]

Jeremy, would this break Xen?  As far as I know, Xen is the main user of 
skipping segment register initialization.

	-hpa


On 01/27/2011 06:24 PM, H. Peter Anvin wrote:
> On 01/27/2011 03:00 PM, Kees Cook wrote:
>>
>> Yikes, good catch.
>>
>> arch/x86/kernel/trampoline_64.S uses:
>> movw $(trampoline_stack_end - r_base), %sp
>>
>> arch/x86/boot/compressed/head_64.S uses:
>> movl $boot_stack_end, %eax
>> addl %ebp, %eax
>> movl %eax, %esp
>>
>> what would be safe for arch/x86/kernel/head_32.S ? It uses "stack_start",
>> but later after paging set-up. Is the following sane to solve this?
>>
>
> To run it before paging is set up, you can't use stack, start; you have
> to use a pointer based on physical address. You have two problems with
> using stack_start: you're using a linear address to access stack_start,
> and stack_start itself contains a linear address.
>
> It's not entirely clear to me why we don't initialize %ss to __BOOT_DS
> with the other segment registers, but it would make most sense to me:
>
> diff --git a/arch/x86/kernel/head_32.S b/arch/x86/kernel/head_32.S
> index fc293dc..c10f9ba 100644
> --- a/arch/x86/kernel/head_32.S
> +++ b/arch/x86/kernel/head_32.S
> @@ -99,7 +99,12 @@ ENTRY(startup_32)
> movl %eax,%es
> movl %eax,%fs
> movl %eax,%gs
> + movl %eax,%ss
> 2:
> +/*
> + * Set up an initial stack
> + */
> + movl $pa(init_thread_union+THREAD_SIZE), %esp
>
> /*
> * Clear BSS first so that there are no surprises...
>
> -hpa

Re: [BUG] broken ebba638ae723d8a8fc2f7abce5ec18b688b791d7

[Jeremy Fitzhardinge]

On 01/27/2011 07:38 PM, H. Peter Anvin wrote:
> [Adding Jeremy]
>
> Jeremy, would this break Xen?  As far as I know, Xen is the main user
> of skipping segment register initialization.

Xen doesn't go through head_*.S at all.

    J

>
>     -hpa
>
>
> On 01/27/2011 06:24 PM, H. Peter Anvin wrote:
>> On 01/27/2011 03:00 PM, Kees Cook wrote:
>>>
>>> Yikes, good catch.
>>>
>>> arch/x86/kernel/trampoline_64.S uses:
>>> movw $(trampoline_stack_end - r_base), %sp
>>>
>>> arch/x86/boot/compressed/head_64.S uses:
>>> movl $boot_stack_end, %eax
>>> addl %ebp, %eax
>>> movl %eax, %esp
>>>
>>> what would be safe for arch/x86/kernel/head_32.S ? It uses
>>> "stack_start",
>>> but later after paging set-up. Is the following sane to solve this?
>>>
>>
>> To run it before paging is set up, you can't use stack, start; you have
>> to use a pointer based on physical address. You have two problems with
>> using stack_start: you're using a linear address to access stack_start,
>> and stack_start itself contains a linear address.
>>
>> It's not entirely clear to me why we don't initialize %ss to __BOOT_DS
>> with the other segment registers, but it would make most sense to me:
>>
>> diff --git a/arch/x86/kernel/head_32.S b/arch/x86/kernel/head_32.S
>> index fc293dc..c10f9ba 100644
>> --- a/arch/x86/kernel/head_32.S
>> +++ b/arch/x86/kernel/head_32.S
>> @@ -99,7 +99,12 @@ ENTRY(startup_32)
>> movl %eax,%es
>> movl %eax,%fs
>> movl %eax,%gs
>> + movl %eax,%ss
>> 2:
>> +/*
>> + * Set up an initial stack
>> + */
>> + movl $pa(init_thread_union+THREAD_SIZE), %esp
>>
>> /*
>> * Clear BSS first so that there are no surprises...
>>
>> -hpa
>

Re: [BUG] broken ebba638ae723d8a8fc2f7abce5ec18b688b791d7

[H. Peter Anvin]

On 01/28/2011 08:58 AM, Jeremy Fitzhardinge wrote:
> On 01/27/2011 07:38 PM, H. Peter Anvin wrote:
>> [Adding Jeremy]
>>
>> Jeremy, would this break Xen?  As far as I know, Xen is the main user
>> of skipping segment register initialization.
> 
> Xen doesn't go through head_*.S at all.
> 

OK, I guess it's actually lguest which needs this feature?

	-hpa

Re: [BUG] broken ebba638ae723d8a8fc2f7abce5ec18b688b791d7

[Rusty Russell]

On Thu, 3 Feb 2011 09:18:21 am H. Peter Anvin wrote:
> On 01/28/2011 08:58 AM, Jeremy Fitzhardinge wrote:
> > On 01/27/2011 07:38 PM, H. Peter Anvin wrote:
> >> [Adding Jeremy]
> >>
> >> Jeremy, would this break Xen?  As far as I know, Xen is the main user
> >> of skipping segment register initialization.
> > 
> > Xen doesn't go through head_*.S at all.
> > 
> 
> OK, I guess it's actually lguest which needs this feature?

Well, we already move __BOOT_DS into every other segment reg, so I don't think
%ss will break...

Checking...
Rusty.

Re: [BUG] broken ebba638ae723d8a8fc2f7abce5ec18b688b791d7

[Rusty Russell]

On Thu, 3 Feb 2011 11:49:36 am Rusty Russell wrote:
> On Thu, 3 Feb 2011 09:18:21 am H. Peter Anvin wrote:
> > On 01/28/2011 08:58 AM, Jeremy Fitzhardinge wrote:
> > > On 01/27/2011 07:38 PM, H. Peter Anvin wrote:
> > >> [Adding Jeremy]
> > >>
> > >> Jeremy, would this break Xen?  As far as I know, Xen is the main user
> > >> of skipping segment register initialization.
> > > 
> > > Xen doesn't go through head_*.S at all.
> > > 
> > 
> > OK, I guess it's actually lguest which needs this feature?
> 
> Well, we already move __BOOT_DS into every other segment reg, so I don't think
> %ss will break...
> 
> Checking...

Fine here.

Thanks,
Rusty.
PS.  In general, I don't want lguest to interfere with cleanups: I'd rather
     fix up lguest as required until it gets too hard, then kill it.

Re: [BUG] broken ebba638ae723d8a8fc2f7abce5ec18b688b791d7

[H. Peter Anvin]

Can we completely kill the "don't reload segments" flag, then?

"Rusty Russell" <rusty@rustcorp.com.au> wrote:

>On Thu, 3 Feb 2011 11:49:36 am Rusty Russell wrote:
>> On Thu, 3 Feb 2011 09:18:21 am H. Peter Anvin wrote:
>> > On 01/28/2011 08:58 AM, Jeremy Fitzhardinge wrote:
>> > > On 01/27/2011 07:38 PM, H. Peter Anvin wrote:
>> > >> [Adding Jeremy]
>> > >>
>> > >> Jeremy, would this break Xen?  As far as I know, Xen is the main
>user
>> > >> of skipping segment register initialization.
>> > > 
>> > > Xen doesn't go through head_*.S at all.
>> > > 
>> > 
>> > OK, I guess it's actually lguest which needs this feature?
>> 
>> Well, we already move __BOOT_DS into every other segment reg, so I
>don't think
>> %ss will break...
>> 
>> Checking...
>
>Fine here.
>
>Thanks,
>Rusty.
>PS.  In general, I don't want lguest to interfere with cleanups: I'd
>rather
>     fix up lguest as required until it gets too hard, then kill it.

-- 
Sent from my mobile phone.  Please pardon any lack of formatting.

Re: [BUG] broken ebba638ae723d8a8fc2f7abce5ec18b688b791d7

[Rusty Russell]

On Thu, 3 Feb 2011 01:05:44 pm H. Peter Anvin wrote:
> Can we completely kill the "don't reload segments" flag, then?

We can as far as *I* am concerned, but of course it's a separate issue...

Thanks,
Rusty.

Re: [BUG] broken ebba638ae723d8a8fc2f7abce5ec18b688b791d7

[H. Peter Anvin]

On 02/03/2011 02:02 AM, Rusty Russell wrote:
> On Thu, 3 Feb 2011 01:05:44 pm H. Peter Anvin wrote:
>> Can we completely kill the "don't reload segments" flag, then?
> 
> We can as far as *I* am concerned, but of course it's a separate issue...

I'm trying to figure out if there is any actual user of it.  I thought
it was Xen which needed it, but Jeremy says it doesn't; I don't know
what else would possible need it.  If it is is something that isn't used
then it would be better to get rid of it.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [BUG] broken ebba638ae723d8a8fc2f7abce5ec18b688b791d7

[Kees Cook]

Hi,

On Thu, Jan 27, 2011 at 06:24:14PM -0800, H. Peter Anvin wrote:
> On 01/27/2011 03:00 PM, Kees Cook wrote:
> >
> >Yikes, good catch.
> >
> >arch/x86/kernel/trampoline_64.S uses:
> >         movw    $(trampoline_stack_end - r_base), %sp
> >
> >arch/x86/boot/compressed/head_64.S uses:
> >         movl    $boot_stack_end, %eax
> >         addl    %ebp, %eax
> >         movl    %eax, %esp
> >
> >what would be safe for arch/x86/kernel/head_32.S ? It uses "stack_start",
> >but later after paging set-up. Is the following sane to solve this?
> >
> 
> To run it before paging is set up, you can't use stack, start; you
> have to use a pointer based on physical address.  You have two
> problems with using stack_start: you're using a linear address to
> access stack_start, and stack_start itself contains a linear
> address.
> 
> It's not entirely clear to me why we don't initialize %ss to
> __BOOT_DS with the other segment registers, but it would make most
> sense to me:
> 
> diff --git a/arch/x86/kernel/head_32.S b/arch/x86/kernel/head_32.S
> index fc293dc..c10f9ba 100644
> --- a/arch/x86/kernel/head_32.S
> +++ b/arch/x86/kernel/head_32.S
> @@ -99,7 +99,12 @@ ENTRY(startup_32)
>         movl %eax,%es
>         movl %eax,%fs
>         movl %eax,%gs
> +       movl %eax,%ss
>  2:
> +/*
> + * Set up an initial stack
> + */
> +       movl $pa(init_thread_union+THREAD_SIZE), %esp
> 
>  /*
>   * Clear BSS first so that there are no surprises...

This doesn't appear to work for me. While I can boot fine, doing CPU
hotplugging hangs the system. :(

-Kees

-- 
Kees Cook
Ubuntu Security Team

Re: [BUG] broken ebba638ae723d8a8fc2f7abce5ec18b688b791d7

[matthieu castet]

[-- Attachment #1: Type: text/plain, Size: 1809 bytes --]

Kees Cook a écrit :
> Hi,
> 
> On Thu, Jan 27, 2011 at 06:24:14PM -0800, H. Peter Anvin wrote:
>> On 01/27/2011 03:00 PM, Kees Cook wrote:
>>> Yikes, good catch.
>>>
>>> arch/x86/kernel/trampoline_64.S uses:
>>>         movw    $(trampoline_stack_end - r_base), %sp
>>>
>>> arch/x86/boot/compressed/head_64.S uses:
>>>         movl    $boot_stack_end, %eax
>>>         addl    %ebp, %eax
>>>         movl    %eax, %esp
>>>
>>> what would be safe for arch/x86/kernel/head_32.S ? It uses "stack_start",
>>> but later after paging set-up. Is the following sane to solve this?
>>>
>> To run it before paging is set up, you can't use stack, start; you
>> have to use a pointer based on physical address.  You have two
>> problems with using stack_start: you're using a linear address to
>> access stack_start, and stack_start itself contains a linear
>> address.
>>
>> It's not entirely clear to me why we don't initialize %ss to
>> __BOOT_DS with the other segment registers, but it would make most
>> sense to me:
>>
>> diff --git a/arch/x86/kernel/head_32.S b/arch/x86/kernel/head_32.S
>> index fc293dc..c10f9ba 100644
>> --- a/arch/x86/kernel/head_32.S
>> +++ b/arch/x86/kernel/head_32.S
>> @@ -99,7 +99,12 @@ ENTRY(startup_32)
>>         movl %eax,%es
>>         movl %eax,%fs
>>         movl %eax,%gs
>> +       movl %eax,%ss
>>  2:
>> +/*
>> + * Set up an initial stack
>> + */
>> +       movl $pa(init_thread_union+THREAD_SIZE), %esp
>>
>>  /*
>>   * Clear BSS first so that there are no surprises...
> 
> This doesn't appear to work for me. While I can boot fine, doing CPU
> hotplugging hangs the system. :(
> 
This is weird because the patch only touch first cpu (startup_32 entry) and cpu hotplug go to 
startup_32_smp.

Here a untested patch that move the stack setup in the common path.


Matthieu



[-- Attachment #2: diff --]
[-- Type: text/plain, Size: 451 bytes --]

diff --git a/arch/x86/kernel/head_32.S b/arch/x86/kernel/head_32.S
index fc293dc..5df3432 100644
--- a/arch/x86/kernel/head_32.S
+++ b/arch/x86/kernel/head_32.S
@@ -284,6 +284,12 @@ ENTRY(startup_32_smp)
 	movl %eax,%gs
 #endif /* CONFIG_SMP */
 default_entry:
+	/*
+	 * Set up an initial stack
+	 */
+	movl $(__BOOT_DS),%eax
+	movl %eax,%ss
+	movl $pa(init_thread_union+THREAD_SIZE), %esp
 
 /*
  *	New page tables may be in 4Mbyte page mode and may

Re: [BUG] broken ebba638ae723d8a8fc2f7abce5ec18b688b791d7

[Rafael J. Wysocki]

On Tuesday, February 01, 2011, matthieu castet wrote:
> Kees Cook a écrit :
> > Hi,
> > 
> > On Thu, Jan 27, 2011 at 06:24:14PM -0800, H. Peter Anvin wrote:
> >> On 01/27/2011 03:00 PM, Kees Cook wrote:
> >>> Yikes, good catch.
> >>>
> >>> arch/x86/kernel/trampoline_64.S uses:
> >>>         movw    $(trampoline_stack_end - r_base), %sp
> >>>
> >>> arch/x86/boot/compressed/head_64.S uses:
> >>>         movl    $boot_stack_end, %eax
> >>>         addl    %ebp, %eax
> >>>         movl    %eax, %esp
> >>>
> >>> what would be safe for arch/x86/kernel/head_32.S ? It uses "stack_start",
> >>> but later after paging set-up. Is the following sane to solve this?
> >>>
> >> To run it before paging is set up, you can't use stack, start; you
> >> have to use a pointer based on physical address.  You have two
> >> problems with using stack_start: you're using a linear address to
> >> access stack_start, and stack_start itself contains a linear
> >> address.
> >>
> >> It's not entirely clear to me why we don't initialize %ss to
> >> __BOOT_DS with the other segment registers, but it would make most
> >> sense to me:
> >>
> >> diff --git a/arch/x86/kernel/head_32.S b/arch/x86/kernel/head_32.S
> >> index fc293dc..c10f9ba 100644
> >> --- a/arch/x86/kernel/head_32.S
> >> +++ b/arch/x86/kernel/head_32.S
> >> @@ -99,7 +99,12 @@ ENTRY(startup_32)
> >>         movl %eax,%es
> >>         movl %eax,%fs
> >>         movl %eax,%gs
> >> +       movl %eax,%ss
> >>  2:
> >> +/*
> >> + * Set up an initial stack
> >> + */
> >> +       movl $pa(init_thread_union+THREAD_SIZE), %esp
> >>
> >>  /*
> >>   * Clear BSS first so that there are no surprises...
> > 
> > This doesn't appear to work for me. While I can boot fine, doing CPU
> > hotplugging hangs the system. :(
> > 
> This is weird because the patch only touch first cpu (startup_32 entry) and cpu hotplug go to 
> startup_32_smp.
> 
> Here a untested patch that move the stack setup in the common path.

Well, in my not so humble opinion the amount of random fixes required by the
entire NX protection of kernel "data" pages is simply unacceptable.

Please revert the whole thing and do it once again from scratch and possibly
without breaking stuff left and right.  I guess you've learnt enough so far
that this should be doable for 2.6.39 without introducing major issues like
_boot_ problems (let alone the broken resume).

Thanks,
Rafael

Re: [BUG] broken ebba638ae723d8a8fc2f7abce5ec18b688b791d7

[castet.matthieu]

Quoting "Rafael J. Wysocki" <rjw@sisk.pl>:

> On Tuesday, February 01, 2011, matthieu castet wrote:
> Well, in my not so humble opinion the amount of random fixes required by the
> entire NX protection of kernel "data" pages is simply unacceptable.

What's the relation between this bug and NX protection of kernel "data" ?

Matthieu

Re: [BUG] broken ebba638ae723d8a8fc2f7abce5ec18b688b791d7

[Rafael J. Wysocki]

On Tuesday, February 01, 2011, castet.matthieu@free.fr wrote:
> Quoting "Rafael J. Wysocki" <rjw@sisk.pl>:
> 
> > On Tuesday, February 01, 2011, matthieu castet wrote:
> > Well, in my not so humble opinion the amount of random fixes required by the
> > entire NX protection of kernel "data" pages is simply unacceptable.
> 
> What's the relation between this bug and NX protection of kernel "data" ?

Sorry, I confused threads.

Rafael

Re: [BUG] broken ebba638ae723d8a8fc2f7abce5ec18b688b791d7

[Kees Cook]

Hi Matthieu,

On Tue, Feb 01, 2011 at 12:11:16AM +0100, matthieu castet wrote:
> Kees Cook a écrit :
> >On Thu, Jan 27, 2011 at 06:24:14PM -0800, H. Peter Anvin wrote:
> >>On 01/27/2011 03:00 PM, Kees Cook wrote:
> >>>Yikes, good catch.
> >>>
> >>>arch/x86/kernel/trampoline_64.S uses:
> >>>        movw    $(trampoline_stack_end - r_base), %sp
> >>>
> >>>arch/x86/boot/compressed/head_64.S uses:
> >>>        movl    $boot_stack_end, %eax
> >>>        addl    %ebp, %eax
> >>>        movl    %eax, %esp
> >>>
> >>>what would be safe for arch/x86/kernel/head_32.S ? It uses "stack_start",
> >>>but later after paging set-up. Is the following sane to solve this?
> >>>
> >>To run it before paging is set up, you can't use stack, start; you
> >>have to use a pointer based on physical address.  You have two
> >>problems with using stack_start: you're using a linear address to
> >>access stack_start, and stack_start itself contains a linear
> >>address.
> >>
> >>It's not entirely clear to me why we don't initialize %ss to
> >>__BOOT_DS with the other segment registers, but it would make most
> >>sense to me:
> >>
> >>diff --git a/arch/x86/kernel/head_32.S b/arch/x86/kernel/head_32.S
> >>index fc293dc..c10f9ba 100644
> >>--- a/arch/x86/kernel/head_32.S
> >>+++ b/arch/x86/kernel/head_32.S
> >>@@ -99,7 +99,12 @@ ENTRY(startup_32)
> >>        movl %eax,%es
> >>        movl %eax,%fs
> >>        movl %eax,%gs
> >>+       movl %eax,%ss
> >> 2:
> >>+/*
> >>+ * Set up an initial stack
> >>+ */
> >>+       movl $pa(init_thread_union+THREAD_SIZE), %esp
> >>
> >> /*
> >>  * Clear BSS first so that there are no surprises...
> >
> >This doesn't appear to work for me. While I can boot fine, doing CPU
> >hotplugging hangs the system. :(
> >
> This is weird because the patch only touch first cpu (startup_32
> entry) and cpu hotplug go to startup_32_smp.
> 
> Here a untested patch that move the stack setup in the common path.
> 
> diff --git a/arch/x86/kernel/head_32.S b/arch/x86/kernel/head_32.S
> index fc293dc..5df3432 100644
> --- a/arch/x86/kernel/head_32.S
> +++ b/arch/x86/kernel/head_32.S
> @@ -284,6 +284,12 @@ ENTRY(startup_32_smp)
>  	movl %eax,%gs
>  #endif /* CONFIG_SMP */
>  default_entry:
> +	/*
> +	 * Set up an initial stack
> +	 */
> +	movl $(__BOOT_DS),%eax
> +	movl %eax,%ss
> +	movl $pa(init_thread_union+THREAD_SIZE), %esp
>  
>  /*
>   *	New page tables may be in 4Mbyte page mode and may

This worked, thanks! If this tests cleanly for you in qemu, we should get
this committed.

-Kees

-- 
Kees Cook
Ubuntu Security Team

Re: [BUG] broken ebba638ae723d8a8fc2f7abce5ec18b688b791d7

[H. Peter Anvin]

On 01/31/2011 03:52 PM, Kees Cook wrote:
>
> This worked, thanks! If this tests cleanly for you in qemu, we should get
> this committed.
>

This is wrong for at least one reason; possibly two:

a) it ignores the control to not reload the segment registers (not sure 
if anything still uses them, but...)

b) I'm not sure that init_thread_union is safe for the non-BSP CPU here.

	-hpa

Re: [BUG] broken ebba638ae723d8a8fc2f7abce5ec18b688b791d7

[Kees Cook]

On Mon, Jan 31, 2011 at 05:10:03PM -0800, H. Peter Anvin wrote:
> On 01/31/2011 03:52 PM, Kees Cook wrote:
> >
> >This worked, thanks! If this tests cleanly for you in qemu, we should get
> >this committed.
> >
> 
> This is wrong for at least one reason; possibly two:
> 
> a) it ignores the control to not reload the segment registers (not
> sure if anything still uses them, but...)
> 
> b) I'm not sure that init_thread_union is safe for the non-BSP CPU here.

What's the best way to move forward? Or, how can we find answers to these
questions?

-Kees

-- 
Kees Cook
Ubuntu Security Team

Re: [BUG] broken ebba638ae723d8a8fc2f7abce5ec18b688b791d7

[H. Peter Anvin]

[-- Attachment #1: Type: text/plain, Size: 802 bytes --]

On 02/02/2011 12:40 PM, Kees Cook wrote:
> On Mon, Jan 31, 2011 at 05:10:03PM -0800, H. Peter Anvin wrote:
>> On 01/31/2011 03:52 PM, Kees Cook wrote:
>>>
>>> This worked, thanks! If this tests cleanly for you in qemu, we should get
>>> this committed.
>>>
>>
>> This is wrong for at least one reason; possibly two:
>>
>> a) it ignores the control to not reload the segment registers (not
>> sure if anything still uses them, but...)
>>
>> b) I'm not sure that init_thread_union is safe for the non-BSP CPU here.
> 
> What's the best way to move forward? Or, how can we find answers to these
> questions?
> 
> -Kees
> 


Can someone test out the attached patch and verify that it works?

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.


[-- Attachment #2: diff --]
[-- Type: text/plain, Size: 1005 bytes --]

diff --git a/arch/x86/kernel/head_32.S b/arch/x86/kernel/head_32.S
index fc293dc..2aee594 100644
--- a/arch/x86/kernel/head_32.S
+++ b/arch/x86/kernel/head_32.S
@@ -26,6 +26,11 @@
 #define pa(X) ((X) - __PAGE_OFFSET)
 
 /*
+ * Initial stack pointer for the boot processor
+ */
+initial_stack_pointer = init_thread_union+THREAD_SIZE
+
+/*
  * References to members of the new_cpu_data structure.
  */
 
@@ -99,8 +104,10 @@ ENTRY(startup_32)
 	movl %eax,%es
 	movl %eax,%fs
 	movl %eax,%gs
+	movl %eax,%ss
 2:
-
+	movl $pa(initial_stack_pointer), %esp
+	
 /*
  * Clear BSS first so that there are no surprises...
  */
@@ -282,6 +289,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:
 
@@ -671,7 +681,7 @@ ENTRY(initial_page_table)
 
 .data
 ENTRY(stack_start)
-	.long init_thread_union+THREAD_SIZE
+	.long initial_stack_pointer
 	.long __BOOT_DS
 
 ready:	.byte 0

Re: [BUG] broken ebba638ae723d8a8fc2f7abce5ec18b688b791d7

[matthieu castet]

Kees Cook a écrit :
> Hi,
> 
> On Thu, Jan 27, 2011 at 06:24:14PM -0800, H. Peter Anvin wrote:
>> On 01/27/2011 03:00 PM, Kees Cook wrote:
>>> Yikes, good catch.
>>>
>>> arch/x86/kernel/trampoline_64.S uses:
>>>         movw    $(trampoline_stack_end - r_base), %sp
>>>
>>> arch/x86/boot/compressed/head_64.S uses:
>>>         movl    $boot_stack_end, %eax
>>>         addl    %ebp, %eax
>>>         movl    %eax, %esp
>>>
>>> what would be safe for arch/x86/kernel/head_32.S ? It uses "stack_start",
>>> but later after paging set-up. Is the following sane to solve this?
>>>
>> To run it before paging is set up, you can't use stack, start; you
>> have to use a pointer based on physical address.  You have two
>> problems with using stack_start: you're using a linear address to
>> access stack_start, and stack_start itself contains a linear
>> address.
>>
>> It's not entirely clear to me why we don't initialize %ss to
>> __BOOT_DS with the other segment registers, but it would make most
>> sense to me:
>>
>> diff --git a/arch/x86/kernel/head_32.S b/arch/x86/kernel/head_32.S
>> index fc293dc..c10f9ba 100644
>> --- a/arch/x86/kernel/head_32.S
>> +++ b/arch/x86/kernel/head_32.S
>> @@ -99,7 +99,12 @@ ENTRY(startup_32)
>>         movl %eax,%es
>>         movl %eax,%fs
>>         movl %eax,%gs
>> +       movl %eax,%ss
>>  2:
>> +/*
>> + * Set up an initial stack
>> + */
>> +       movl $pa(init_thread_union+THREAD_SIZE), %esp
>>
>>  /*
>>   * Clear BSS first so that there are no surprises...
> 
> This doesn't appear to work for me. While I can boot fine, doing CPU
> hotplugging hangs the system. :(
> 
That's because of NX :)

The stack is marked NX, 

[tip:x86/urgent] x86-32: Make sure the stack is set up before we use it

[tip-bot for H. Peter Anvin]

Commit-ID:  990957d4076354d449921b598cc8de28764cf7ed
Gitweb:     http://git.kernel.org/tip/990957d4076354d449921b598cc8de28764cf7ed
Author:     H. Peter Anvin <hpa@linux.intel.com>
AuthorDate: Fri, 4 Feb 2011 16:14:11 -0800
Committer:  H. Peter Anvin <hpa@linux.intel.com>
CommitDate: Fri, 4 Feb 2011 16:27:21 -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 <castet.matthieu@free.fr>
LKML-Reference: <4D41E86D.8060205@free.fr>
Cc: Kees Cook <kees.cook@canonical.com>
Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
---
 arch/x86/include/asm/smp.h   |    5 +----
 arch/x86/kernel/acpi/sleep.c |    2 +-
 arch/x86/kernel/head_32.S    |   20 ++++++++++++--------
 arch/x86/kernel/head_64.S    |    1 -
 arch/x86/kernel/smpboot.c    |    4 ++--
 5 files changed, 16 insertions(+), 16 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..10a6381 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
@@ -475,7 +480,6 @@ is386:	movl $2,%ecx		# set MP
 	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 +674,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();

[tip:x86/urgent] x86-32: Make sure the stack is set up before we use it

[tip-bot for H. Peter Anvin]

Commit-ID:  43d79a250bdb10e30b08a79e906befc691c3f5ce
Gitweb:     http://git.kernel.org/tip/43d79a250bdb10e30b08a79e906befc691c3f5ce
Author:     H. Peter Anvin <hpa@linux.intel.com>
AuthorDate: Fri, 4 Feb 2011 16:14:11 -0800
Committer:  H. Peter Anvin <hpa@linux.intel.com>
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 <castet.matthieu@free.fr>
LKML-Reference: <4D41E86D.8060205@free.fr>
Cc: Kees Cook <kees.cook@canonical.com>
Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
---
 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();

Re: [tip:x86/urgent] x86-32: Make sure the stack is set up before we use it

[Kees Cook]

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 <hpa@linux.intel.com>
> AuthorDate: Fri, 4 Feb 2011 16:14:11 -0800
> Committer:  H. Peter Anvin <hpa@linux.intel.com>
> 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 <castet.matthieu@free.fr>
> LKML-Reference: <4D41E86D.8060205@free.fr>
> Cc: Kees Cook <kees.cook@canonical.com>
> Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
> ---
>  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

Re: [tip:x86/urgent] x86-32: Make sure the stack is set up before we use it

[H. Peter Anvin]

On 02/04/2011 06:19 PM, Kees Cook wrote:
> Hi Peter,
> 
> This works for me doing CPU hotplugging on ia32, but on x86_64 it hangs.
> (Both boot, though.)
> 
> Thanks,
> 
> -Kees

I can't reproduce the failure here, and it's hard to see how this patch
would make a significant change on 64 bits.  Could you try reverting
*only* the head_64.S hunk and try it on your platform again?

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [tip:x86/urgent] x86-32: Make sure the stack is set up before we use it

[Kees Cook]

On Fri, Feb 04, 2011 at 08:37:37PM -0800, H. Peter Anvin wrote:
> On 02/04/2011 06:19 PM, Kees Cook wrote:
> > This works for me doing CPU hotplugging on ia32, but on x86_64 it hangs.
> > (Both boot, though.)
> 
> I can't reproduce the failure here, and it's hard to see how this patch
> would make a significant change on 64 bits.  Could you try reverting
> *only* the head_64.S hunk and try it on your platform again?

With the head_64.S hunk dropped, it runs fine. My highly fine-tuned
test-case is:

  while :; do ls -la / -R >/dev/null 2>/dev/null; done &
  while :; do echo 0 > /sys/devices/system/cpu/cpu1/online ; echo 1 > /sys/devices/system/cpu/cpu1/online ; done &

Looks good from this end. Thanks!

-Kees

-- 
Kees Cook
Ubuntu Security Team

Re: [tip:x86/urgent] x86-32: Make sure the stack is set up before we use it

[H. Peter Anvin]

On 02/04/2011 09:37 PM, Kees Cook wrote:
> On Fri, Feb 04, 2011 at 08:37:37PM -0800, H. Peter Anvin wrote:
>> On 02/04/2011 06:19 PM, Kees Cook wrote:
>>> This works for me doing CPU hotplugging on ia32, but on x86_64 it hangs.
>>> (Both boot, though.)
>>
>> I can't reproduce the failure here, and it's hard to see how this patch
>> would make a significant change on 64 bits.  Could you try reverting
>> *only* the head_64.S hunk and try it on your platform again?
> 
> With the head_64.S hunk dropped, it runs fine. My highly fine-tuned
> test-case is:
> 
>   while :; do ls -la / -R >/dev/null 2>/dev/null; done &
>   while :; do echo 0 > /sys/devices/system/cpu/cpu1/online ; echo 1 > /sys/devices/system/cpu/cpu1/online ; done &
> 
> Looks good from this end. Thanks!
> 

OK, that bothers me, because it would seem that something is accessing
data it shouldn't, but ok, it fixes it for now.  I'll update it in -tip.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

[tip:x86/urgent] x86-32: Make sure the stack is set up before we use it

[tip-bot for H. Peter Anvin]

Commit-ID:  11d4c3f9b671720e80353dd7e433ff2bf65e9500
Gitweb:     http://git.kernel.org/tip/11d4c3f9b671720e80353dd7e433ff2bf65e9500
Author:     H. Peter Anvin <hpa@linux.intel.com>
AuthorDate: Fri, 4 Feb 2011 16:14:11 -0800
Committer:  H. Peter Anvin <hpa@zytor.com>
CommitDate: Fri, 4 Feb 2011 22:27:28 -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 <castet.matthieu@free.fr>
LKML-Reference: <4D41E86D.8060205@free.fr>
Tested-by: Kees Cook <kees.cook@canonical.com>
Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
---
 arch/x86/include/asm/smp.h   |    5 +----
 arch/x86/kernel/acpi/sleep.c |    2 +-
 arch/x86/kernel/head_32.S    |   30 +++++++++++++-----------------
 arch/x86/kernel/smpboot.c    |    4 ++--
 4 files changed, 17 insertions(+), 24 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/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();