LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "Rafael J. Wysocki" <rjw@sisk.pl>
To: matthieu castet <castet.matthieu@free.fr>, Ingo Molnar <mingo@elte.hu>
Cc: Kees Cook <kees.cook@canonical.com>,
	"H. Peter Anvin" <hpa@zytor.com>,
	Linux Kernel list <linux-kernel@vger.kernel.org>,
	Jeremy Fitzhardinge <jeremy@goop.org>
Subject: Re: [BUG] broken ebba638ae723d8a8fc2f7abce5ec18b688b791d7
Date: Tue, 1 Feb 2011 00:17:40 +0100	[thread overview]
Message-ID: <201102010017.40986.rjw@sisk.pl> (raw)
In-Reply-To: <4D474194.2000709@free.fr>

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

  reply	other threads:[~2011-01-31 23:18 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-01-27 21:49 matthieu castet
2011-01-27 23:00 ` Kees Cook
2011-01-28  2:24   ` H. Peter Anvin
2011-01-28  3:38     ` H. Peter Anvin
2011-01-28 16:58       ` Jeremy Fitzhardinge
2011-02-02 22:48         ` H. Peter Anvin
2011-02-03  1:19           ` Rusty Russell
2011-02-03  2:00             ` Rusty Russell
2011-02-03  2:35               ` H. Peter Anvin
2011-02-03 10:02                 ` Rusty Russell
2011-02-03 17:11                   ` H. Peter Anvin
2011-01-31 21:38     ` Kees Cook
2011-01-31 23:11       ` matthieu castet
2011-01-31 23:17         ` Rafael J. Wysocki [this message]
2011-02-01 13:07           ` castet.matthieu
2011-02-01 18:50             ` Rafael J. Wysocki
2011-01-31 23:52         ` Kees Cook
2011-02-01  1:10           ` H. Peter Anvin
2011-02-02 20:40             ` Kees Cook
2011-02-04  5:47               ` H. Peter Anvin
2011-01-31 23:12       ` matthieu castet
2011-02-05  0:34 ` [tip:x86/urgent] x86-32: Make sure the stack is set up before we use it tip-bot for H. Peter Anvin
2011-02-05  0:45 ` tip-bot for H. Peter Anvin
2011-02-05  2:19   ` Kees Cook
2011-02-05  4:37     ` H. Peter Anvin
2011-02-05  5:37       ` Kees Cook
2011-02-05  6:26         ` H. Peter Anvin
2011-02-05  6:31 ` tip-bot for H. Peter Anvin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=201102010017.40986.rjw@sisk.pl \
    --to=rjw@sisk.pl \
    --cc=castet.matthieu@free.fr \
    --cc=hpa@zytor.com \
    --cc=jeremy@goop.org \
    --cc=kees.cook@canonical.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@elte.hu \
    --subject='Re: [BUG] broken ebba638ae723d8a8fc2f7abce5ec18b688b791d7' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).