LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Andy Lutomirski <luto@amacapital.net>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: "H. Peter Anvin" <hpa@zytor.com>,
	Denys Vlasenko <dvlasenk@redhat.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	Ingo Molnar <mingo@kernel.org>, Borislav Petkov <bp@alien8.de>,
	Oleg Nesterov <oleg@redhat.com>,
	Frederic Weisbecker <fweisbec@gmail.com>,
	Alexei Starovoitov <ast@plumgrid.com>,
	Will Drewry <wad@chromium.org>, Kees Cook <keescook@chromium.org>,
	"the arch/x86 maintainers" <x86@kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] x86: entry_32.S: change ESPFIX test to not touch PT_OLDSS(%esp)
Date: Mon, 9 Mar 2015 11:04:55 -0700	[thread overview]
Message-ID: <CALCETrVt5AnrdHrj=HcWrLRHT7TN1ZoFb3cqj0VoktCn4c0W6A@mail.gmail.com> (raw)
In-Reply-To: <CA+55aFz2HOnPg9i4WSeL4rQgSJDUXbiWjbqnODib=vyWrPuJEg@mail.gmail.com>

On Mon, Mar 9, 2015 at 10:59 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Mon, Mar 9, 2015 at 10:45 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>>
>> If sp0 is set to the very top of the stack, then an NMI immediately
>> after sysenter will have OLDSS off the top of the stack, and reading
>> it can crash.  This is why 32-bit kernels have a (buggy!) 8 byte
>> offset in sp0.
>
> So I think that for sysenter, we *should* have that 8-byte buffer.
>
> Not in general for sp0, but for MSR_IA32_SYSENTER_ESP (which is sp1, afaik).

Bah.  I'm slightly wrong here.  It's not NMI immediately after
sysenter (that's handled by the emergency stack gunk, although we
might need to fix that too).  It's this:

ENTRY(ia32_sysenter_target)
    CFI_STARTPROC simple
    CFI_SIGNAL_FRAME
    CFI_DEF_CFA esp, 0
    CFI_REGISTER esp, ebp
    movl TSS_sysenter_sp0(%esp),%esp
sysenter_past_esp:
    /*
     * Interrupts are disabled here, but we can't trace it until
     * enough kernel state to call TRACE_IRQS_OFF can be called - but
     * we immediately enable interrupts at that point anyway.
     */

--> NMI here

    pushl_cfi $__USER_DS

--> or here

    /*CFI_REL_OFFSET ss, 0*/
    pushl_cfi %ebp

We could fix that locally by loading sp0 - 8 atomically (w/ percpu
help) and populating the top two words with mov instead of push.

One option would be to change the NMI entry code to move itself down 8
bytes if this happens (came from kernel mode or sp == sp0 - 12,
perhaps).  Or we could suck it up and keep that 8 byte offset (and fix
the cpu_tss initializer bug that threw me off in the first place).

--Andy

>
> Just make the rule be that you can never ever have a kernel stack
> frame that doesn't contain room for ss/sp at the top.
>
> We have various code that looks at and touches "pt_regs" anyway, and
> accesses things out for debugging/oopsing/tracing etc. Let's not make
> the rule be that you cannot look at regs->ss without checking various
> random other fields first.
>
>                          Linus



-- 
Andy Lutomirski
AMA Capital Management, LLC

  reply	other threads:[~2015-03-09 18:05 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-09 14:05 Denys Vlasenko
2015-03-09 14:18 ` Andy Lutomirski
2015-03-09 15:00   ` Denys Vlasenko
2015-03-09 15:09     ` Andy Lutomirski
2015-03-09 19:31       ` Denys Vlasenko
2015-03-09 15:13     ` Ingo Molnar
2015-03-09 15:18       ` Andy Lutomirski
2015-03-09 15:47       ` Steven Rostedt
2015-03-09 15:54         ` Ingo Molnar
2015-03-09 16:08 ` Linus Torvalds
2015-03-09 16:28   ` Denys Vlasenko
2015-03-09 16:44     ` Linus Torvalds
2015-03-09 17:44       ` H. Peter Anvin
2015-03-09 19:13         ` Andy Lutomirski
2015-03-09 19:26           ` H. Peter Anvin
2015-03-09 19:51             ` Andy Lutomirski
2015-03-09 17:42   ` H. Peter Anvin
2015-03-09 17:45     ` Andy Lutomirski
2015-03-09 17:59       ` Linus Torvalds
2015-03-09 18:04         ` Andy Lutomirski [this message]
2015-03-09 18:16           ` Linus Torvalds
2015-03-09 18:32             ` Denys Vlasenko
2015-03-09 18:36             ` Andy Lutomirski
2015-03-10  6:25               ` Ingo Molnar

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='CALCETrVt5AnrdHrj=HcWrLRHT7TN1ZoFb3cqj0VoktCn4c0W6A@mail.gmail.com' \
    --to=luto@amacapital.net \
    --cc=ast@plumgrid.com \
    --cc=bp@alien8.de \
    --cc=dvlasenk@redhat.com \
    --cc=fweisbec@gmail.com \
    --cc=hpa@zytor.com \
    --cc=keescook@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@kernel.org \
    --cc=oleg@redhat.com \
    --cc=rostedt@goodmis.org \
    --cc=torvalds@linux-foundation.org \
    --cc=wad@chromium.org \
    --cc=x86@kernel.org \
    --subject='Re: [PATCH] x86: entry_32.S: change ESPFIX test to not touch PT_OLDSS(%esp)' \
    /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).