[PATCH][GIT PULL][v3.3] x86: Test saved %rip in NMI to determine nested NMI

[PATCH][GIT PULL][v3.3] x86: Test saved %rip in NMI to determine nested NMI

[Steven Rostedt]

Ingo,

I found that it is possible for userspace to prevent an NMI from
triggering while it is running by setting its stack pointer to that of
the NMI stack. This tricks the NMI nested algorithm in thinking that the
NMI is nested. The easy solution to this is to test the %rip to make
sure that the NMI happened in kernel mode before testing for nesting.

I've tested a program that exhibits the missing NMIs and this patch
corrects that behavior.

-- Steve

Please pull the latest tip/perf/urgent tree, which can be found at:

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
tip/perf/urgent

Head SHA1: b80ddc7b1636474297815d47fbfed7552f9b8f2c


Steven Rostedt (1):
      x86: Test saved %rip in NMI to determine nested NMI

----
 arch/x86/kernel/entry_64.S |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)
---------------------------
commit b80ddc7b1636474297815d47fbfed7552f9b8f2c
Author: Steven Rostedt <srostedt@redhat.com>
Date:   Sat Feb 18 20:26:52 2012 -0500

    x86: Test saved %rip in NMI to determine nested NMI
    
    Currently, the NMI handler tests if it is nested by checking the
    special variable saved no the stack (set during NMI handling) and
    whether the saved stack is the NMI stack as well (to prevent the race
    when the variable is set to zero). But userspace may set their %rsp
    to any value as long as the do not derefence it, and it may make it
    point to the NMI stack, which will prevent NMIs from triggering while
    the userspace app is running. (I tested this, and it is indeed the case)
    
    Add another check to determine nested NMIs by looking at the saved
    %rip and making sure that it is a kernel pointer (negative).
    
    Cc: H. Peter Anvin <hpa@zytor.com>
    Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
index 3fe8239..7c35a7a 100644
--- a/arch/x86/kernel/entry_64.S
+++ b/arch/x86/kernel/entry_64.S
@@ -1532,6 +1532,14 @@ ENTRY(nmi)
 	pushq_cfi %rdx
 
 	/*
+	 * If the RIP is not negative then we are in userspace where this is not
+	 * a nested NMI. 
+	 */
+	movq 8(%rsp), %rdx
+	testq %rdx, %rdx
+	jns first_nmi
+
+	/*
 	 * Check the special variable on the stack to see if NMIs are
 	 * executing.
 	 */

Re: [PATCH][GIT PULL][v3.3] x86: Test saved %rip in NMI to determine nested NMI

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> Ingo,
> 
> I found that it is possible for userspace to prevent an NMI 
> from triggering while it is running by setting its stack 
> pointer to that of the NMI stack. This tricks the NMI nested 
> algorithm in thinking that the NMI is nested. The easy 
> solution to this is to test the %rip to make sure that the NMI 
> happened in kernel mode before testing for nesting.

Ouch...

> I've tested a program that exhibits the missing NMIs and this 
> patch corrects that behavior.

Does it need a -stable tag?

> Please pull the latest tip/perf/urgent tree, which can be 
> found at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
> tip/perf/urgent
> 
> Head SHA1: b80ddc7b1636474297815d47fbfed7552f9b8f2c
> 
> 
> Steven Rostedt (1):
>       x86: Test saved %rip in NMI to determine nested NMI
> 
> ----
>  arch/x86/kernel/entry_64.S |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> ---------------------------
> commit b80ddc7b1636474297815d47fbfed7552f9b8f2c
> Author: Steven Rostedt <srostedt@redhat.com>
> Date:   Sat Feb 18 20:26:52 2012 -0500
> 
>     x86: Test saved %rip in NMI to determine nested NMI
>     
>     Currently, the NMI handler tests if it is nested by checking the
>     special variable saved no the stack (set during NMI handling) and
>     whether the saved stack is the NMI stack as well (to prevent the race
>     when the variable is set to zero). But userspace may set their %rsp
>     to any value as long as the do not derefence it, and it may make it
>     point to the NMI stack, which will prevent NMIs from triggering while
>     the userspace app is running. (I tested this, and it is indeed the case)
>     
>     Add another check to determine nested NMIs by looking at the saved
>     %rip and making sure that it is a kernel pointer (negative).
>     
>     Cc: H. Peter Anvin <hpa@zytor.com>
>     Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> 
> diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
> index 3fe8239..7c35a7a 100644
> --- a/arch/x86/kernel/entry_64.S
> +++ b/arch/x86/kernel/entry_64.S
> @@ -1532,6 +1532,14 @@ ENTRY(nmi)
>  	pushq_cfi %rdx
>  
>  	/*
> +	 * If the RIP is not negative then we are in userspace where this is not
> +	 * a nested NMI. 
> +	 */
> +	movq 8(%rsp), %rdx
> +	testq %rdx, %rdx
> +	jns first_nmi

Does this do the right thing for the vDSO as well? It is in 
negative addresses:

ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0                  [vsyscall]

Thanks,

	Ingo

Re: [PATCH][GIT PULL][v3.3] x86: Test saved %rip in NMI to determine nested NMI

[hpanvin@gmail.com]

Vsyscall page, not vdso...

Ingo Molnar <mingo@elte.hu> wrote:

>
>* Steven Rostedt <rostedt@goodmis.org> wrote:
>
>> Ingo,
>> 
>> I found that it is possible for userspace to prevent an NMI 
>> from triggering while it is running by setting its stack 
>> pointer to that of the NMI stack. This tricks the NMI nested 
>> algorithm in thinking that the NMI is nested. The easy 
>> solution to this is to test the %rip to make sure that the NMI 
>> happened in kernel mode before testing for nesting.
>
>Ouch...
>
>> I've tested a program that exhibits the missing NMIs and this 
>> patch corrects that behavior.
>
>Does it need a -stable tag?
>
>> Please pull the latest tip/perf/urgent tree, which can be 
>> found at:
>> 
>>  
>git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
>> tip/perf/urgent
>> 
>> Head SHA1: b80ddc7b1636474297815d47fbfed7552f9b8f2c
>> 
>> 
>> Steven Rostedt (1):
>>       x86: Test saved %rip in NMI to determine nested NMI
>> 
>> ----
>>  arch/x86/kernel/entry_64.S |    8 ++++++++
>>  1 files changed, 8 insertions(+), 0 deletions(-)
>> ---------------------------
>> commit b80ddc7b1636474297815d47fbfed7552f9b8f2c
>> Author: Steven Rostedt <srostedt@redhat.com>
>> Date:   Sat Feb 18 20:26:52 2012 -0500
>> 
>>     x86: Test saved %rip in NMI to determine nested NMI
>>     
>>     Currently, the NMI handler tests if it is nested by checking the
>>     special variable saved no the stack (set during NMI handling) and
>>     whether the saved stack is the NMI stack as well (to prevent the
>race
>>     when the variable is set to zero). But userspace may set their
>%rsp
>>     to any value as long as the do not derefence it, and it may make
>it
>>     point to the NMI stack, which will prevent NMIs from triggering
>while
>>     the userspace app is running. (I tested this, and it is indeed
>the case)
>>     
>>     Add another check to determine nested NMIs by looking at the
>saved
>>     %rip and making sure that it is a kernel pointer (negative).
>>     
>>     Cc: H. Peter Anvin <hpa@zytor.com>
>>     Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
>> 
>> diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
>> index 3fe8239..7c35a7a 100644
>> --- a/arch/x86/kernel/entry_64.S
>> +++ b/arch/x86/kernel/entry_64.S
>> @@ -1532,6 +1532,14 @@ ENTRY(nmi)
>>  	pushq_cfi %rdx
>>  
>>  	/*
>> +	 * If the RIP is not negative then we are in userspace where this
>is not
>> +	 * a nested NMI. 
>> +	 */
>> +	movq 8(%rsp), %rdx
>> +	testq %rdx, %rdx
>> +	jns first_nmi
>
>Does this do the right thing for the vDSO as well? It is in 
>negative addresses:
>
>ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0                
> [vsyscall]
>
>Thanks,
>
>	Ingo

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

Re: [PATCH][GIT PULL][v3.3] x86: Test saved %rip in NMI to determine nested NMI

[Ingo Molnar]

* hpanvin@gmail.com <hpa@zytor.com> wrote:

> Vsyscall page, not vdso...

yes - same general purpose: user-space execution happens in the 
kernel address range.

Thanks,

	Ingo

Re: [PATCH][GIT PULL][v3.3] x86: Test saved %rip in NMI to determine nested NMI

[Steven Rostedt]

On Sun, 2012-02-19 at 13:56 +0100, Ingo Molnar wrote:
> * Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > Ingo,
> > 
> > I found that it is possible for userspace to prevent an NMI 
> > from triggering while it is running by setting its stack 
> > pointer to that of the NMI stack. This tricks the NMI nested 
> > algorithm in thinking that the NMI is nested. The easy 
> > solution to this is to test the %rip to make sure that the NMI 
> > happened in kernel mode before testing for nesting.
> 
> Ouch...

Note, it does not seem to cause any destruction, but screw up profiling.
It doesn't change how timer interrupts, or any other interrupts work.
I'm not sure what problems this can cause on a security level. But it
needs to be fixed regardless.

> 
> > I've tested a program that exhibits the missing NMIs and this 
> > patch corrects that behavior.
> 
> Does it need a -stable tag?

The nesting code was introduced in 3.3 so this does not affect any major
released version of Linux.


> Does this do the right thing for the vDSO as well? It is in 
> negative addresses:
> 
> ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0                  [vsyscall]
> 

Hmm, I thought about this as well, Can that code run with a bad stack?
It would require a process to call this code using the NMI stack
pointer, and things like push and pop would cause the code to crash
immediately with a SEGFAULT.

I could add more checks in the nested NMI path, as that is the slow path
because nested NMIs are extremely rare.

-- Steve

Re: [PATCH][GIT PULL][v3.3] x86: Test saved %rip in NMI to determine nested NMI

[Steven Rostedt]

On Sun, 2012-02-19 at 05:46 -0800, hpanvin@gmail.com wrote:
> Vsyscall page, not vdso...
> 

Peter,

My original patch was to check the %cs register against __KERNEL_CS, but
IIRC, you said that userspace can change that register to anything it
wanted before doing a long jump or something. Is this true for x86_64 as
well? I guess it would be because x86_64 can support 32bit apps.

Anyway, I'll add a check that makes sure that the RIP is less than the
FIXADDR sections as well.

-- Steve

Re: [PATCH][GIT PULL][v3.3] x86: Test saved %rip in NMI to determine nested NMI

[H. Peter Anvin]

On 02/19/2012 12:34 PM, Steven Rostedt wrote:
> On Sun, 2012-02-19 at 05:46 -0800, hpanvin@gmail.com wrote:
>> Vsyscall page, not vdso...
>
> Peter,
>
> My original patch was to check the %cs register against __KERNEL_CS, but
> IIRC, you said that userspace can change that register to anything it
> wanted before doing a long jump or something. Is this true for x86_64 as
> well? I guess it would be because x86_64 can support 32bit apps.
>
> Anyway, I'll add a check that makes sure that the RIP is less than the
> FIXADDR sections as well.
>

User space can change %cs, but it can never set it to __KERNEL_CS; 
specifically user space can never set the bottom two bits in CS to zero.

So this should be a better test.

(Now, doing the test that way plays havoc with the braindamage knows as 
Xen paravirt, but I don't think it's subject to this particular NMI code.)

	-hpa

Re: [PATCH][GIT PULL][v3.3] x86: Test saved %rip in NMI to determine nested NMI

[Steven Rostedt]

On Sun, 2012-02-19 at 12:45 -0800, H. Peter Anvin wrote:

> User space can change %cs, but it can never set it to __KERNEL_CS; 
> specifically user space can never set the bottom two bits in CS to zero.
> 
> So this should be a better test.

Great! That means that my original patch (the one never submitted, but
tested) would be the best.

Ingo,

I'll send out another pull request with the old patch. I'll still run it
through tests just to be sure, so it may take a day before I send it.

(I hope I still have it)

> 
> (Now, doing the test that way plays havoc with the braindamage knows as 
> Xen paravirt, but I don't think it's subject to this particular NMI code.)

Xen paravirt shouldn't be doing nested NMIs. I hope :-)

-- Steve

[PATCH v2][GIT PULL][v3.3] x86: Test saved %cs in NMI to determine nested NMI

[Steven Rostedt]

Ingo,

I rebased the same branch so you may need to wait a little for the
kernel.org mirrors to update.

Please pull the latest tip/perf/urgent tree, which can be found at:

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
tip/perf/urgent

Head SHA1: 38f53faa02c0976ed0a6f0290771a4cbe6dc655b


Steven Rostedt (1):
      x86: Test saved %cs in NMI to determine nested NMI

----
 arch/x86/kernel/entry_64.S |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)
---------------------------
commit 38f53faa02c0976ed0a6f0290771a4cbe6dc655b
Author: Steven Rostedt <srostedt@redhat.com>
Date:   Sat Feb 18 20:26:52 2012 -0500

    x86: Test saved %cs in NMI to determine nested NMI
    
    Currently, the NMI handler tests if it is nested by checking the
    special variable saved on the stack (set during NMI handling) and
    whether the saved stack is the NMI stack as well (to prevent the race
    when the variable is set to zero). But userspace may set their %rsp
    to any value as long as they do not derefence it, and it may make it
    point to the NMI stack, which will prevent NMIs from triggering while
    the userspace app is running. (I tested this, and it is indeed the case)
    
    Add another check to determine nested NMIs by looking at the saved
    %cs (code segment register) and making sure that it is the kernel code segment.
    
    Cc: H. Peter Anvin <hpa@zytor.com>
    Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
index 3fe8239..debd851 100644
--- a/arch/x86/kernel/entry_64.S
+++ b/arch/x86/kernel/entry_64.S
@@ -1532,6 +1532,13 @@ ENTRY(nmi)
 	pushq_cfi %rdx
 
 	/*
+	 * If %cs was not the kernel segment, then the NMI triggered in user
+	 * space, which means it is definitely not nested.
+	 */
+	cmp $__KERNEL_CS, 16(%rsp)
+	jne first_nmi
+
+	/*
 	 * Check the special variable on the stack to see if NMIs are
 	 * executing.
 	 */

Re: [PATCH][GIT PULL][v3.3] x86: Test saved %rip in NMI to determine nested NMI

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> On Sun, 2012-02-19 at 13:56 +0100, Ingo Molnar wrote:
> > * Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > > Ingo,
> > > 
> > > I found that it is possible for userspace to prevent an NMI 
> > > from triggering while it is running by setting its stack 
> > > pointer to that of the NMI stack. This tricks the NMI nested 
> > > algorithm in thinking that the NMI is nested. The easy 
> > > solution to this is to test the %rip to make sure that the NMI 
> > > happened in kernel mode before testing for nesting.
> > 
> > Ouch...
> 
> Note, it does not seem to cause any destruction, but screw up 
> profiling.

Except if the source of the NMI was not profiling, right?

Thanks,

	Ingo

[tip:x86/urgent] x86/nmi: Test saved %cs in NMI to determine nested NMI case

[tip-bot for Steven Rostedt]

Commit-ID:  45d5a1683c04be28abdf5c04c27b1417e0374486
Gitweb:     http://git.kernel.org/tip/45d5a1683c04be28abdf5c04c27b1417e0374486
Author:     Steven Rostedt <rostedt@goodmis.org>
AuthorDate: Sun, 19 Feb 2012 16:43:37 -0500
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Mon, 20 Feb 2012 09:09:57 +0100

x86/nmi: Test saved %cs in NMI to determine nested NMI case

Currently, the NMI handler tests if it is nested by checking the
special variable saved on the stack (set during NMI handling)
and whether the saved stack is the NMI stack as well (to prevent
the race when the variable is set to zero).

But userspace may set their %rsp to any value as long as they do
not derefence it, and it may make it point to the NMI stack,
which will prevent NMIs from triggering while the userspace app
is running. (I tested this, and it is indeed the case)

Add another check to determine nested NMIs by looking at the
saved %cs (code segment register) and making sure that it is the
kernel code segment.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: <stable@kernel.org>
Link: http://lkml.kernel.org/r/1329687817.1561.27.camel@acer.local.home
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/entry_64.S |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
index 3fe8239..debd851 100644
--- a/arch/x86/kernel/entry_64.S
+++ b/arch/x86/kernel/entry_64.S
@@ -1532,6 +1532,13 @@ ENTRY(nmi)
 	pushq_cfi %rdx
 
 	/*
+	 * If %cs was not the kernel segment, then the NMI triggered in user
+	 * space, which means it is definitely not nested.
+	 */
+	cmp $__KERNEL_CS, 16(%rsp)
+	jne first_nmi
+
+	/*
 	 * Check the special variable on the stack to see if NMIs are
 	 * executing.
 	 */

Re: [PATCH][GIT PULL][v3.3] x86: Test saved %rip in NMI to determine nested NMI

[Steven Rostedt]

On Mon, 2012-02-20 at 09:10 +0100, Ingo Molnar wrote:
> > 
> > Note, it does not seem to cause any destruction, but screw up 
> > profiling.
> 
> Except if the source of the NMI was not profiling, right?

Well, what else are NMIs used for? All I can think of is profiling and
watchdogs. I'm not sure how much damage a watchdog NMI being missed in
userspace will hurt it. Unless it takes a watchdog to set off the next
watchdog. That is, if watchdogs are enabled in a one shot mode. Then a
missed NMI could cause the watchdog to shut off. This is all theory.

The test that %cs is __KERNEL_CS before considering the NMI nested seems
to fix this bug. And if userspace really can't change the %cs to
__KERNEL_CS than we are safe. The bug only exists in the 3.3-rc releases
and by fixing it now we don't need to worry about what problems the
original bug can cause.

Thanks,

-- Steve

Re: [PATCH][GIT PULL][v3.3] x86: Test saved %rip in NMI to determine nested NMI

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> On Mon, 2012-02-20 at 09:10 +0100, Ingo Molnar wrote:
> > > 
> > > Note, it does not seem to cause any destruction, but screw up 
> > > profiling.
> > 
> > Except if the source of the NMI was not profiling, right?
> 
> Well, what else are NMIs used for? [...]

There's various (legacy) diagnostics NMIs on various x86 
platforms, mostly signalling some sort of deep trouble
with the hw.

Thanks,

	Ingo

Re: [tip:x86/urgent] x86/nmi: Test saved %cs in NMI to determine nested NMI case

[Steven Rostedt]

On Mon, 2012-02-20 at 03:47 -0800, tip-bot for Steven Rostedt wrote:
> Commit-ID:  45d5a1683c04be28abdf5c04c27b1417e0374486
> Gitweb:     http://git.kernel.org/tip/45d5a1683c04be28abdf5c04c27b1417e0374486
> Author:     Steven Rostedt <rostedt@goodmis.org>
> AuthorDate: Sun, 19 Feb 2012 16:43:37 -0500
> Committer:  Ingo Molnar <mingo@elte.hu>
> CommitDate: Mon, 20 Feb 2012 09:09:57 +0100
> 
> x86/nmi: Test saved %cs in NMI to determine nested NMI case
> 
> Currently, the NMI handler tests if it is nested by checking the
> special variable saved on the stack (set during NMI handling)
> and whether the saved stack is the NMI stack as well (to prevent
> the race when the variable is set to zero).
> 
> But userspace may set their %rsp to any value as long as they do
> not derefence it, and it may make it point to the NMI stack,
> which will prevent NMIs from triggering while the userspace app
> is running. (I tested this, and it is indeed the case)
> 
> Add another check to determine nested NMIs by looking at the
> saved %cs (code segment register) and making sure that it is the
> kernel code segment.
> 
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: <stable@kernel.org>

Just so that we do not confuse Greg, the bug was introduced in the 3.3
merge window. It does not exist in 3.2 or earlier.

-- Steve



> Link: http://lkml.kernel.org/r/1329687817.1561.27.camel@acer.local.home
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> ---
>  arch/x86/kernel/entry_64.S |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
> index 3fe8239..debd851 100644
> --- a/arch/x86/kernel/entry_64.S
> +++ b/arch/x86/kernel/entry_64.S
> @@ -1532,6 +1532,13 @@ ENTRY(nmi)
>  	pushq_cfi %rdx
>  
>  	/*
> +	 * If %cs was not the kernel segment, then the NMI triggered in user
> +	 * space, which means it is definitely not nested.
> +	 */
> +	cmp $__KERNEL_CS, 16(%rsp)
> +	jne first_nmi
> +
> +	/*
>  	 * Check the special variable on the stack to see if NMIs are
>  	 * executing.
>  	 */

Re: [PATCH][GIT PULL][v3.3] x86: Test saved %rip in NMI to determine nested NMI

[H. Peter Anvin]

Not even all that legacy.

Ingo Molnar <mingo@elte.hu> wrote:

>
>* Steven Rostedt <rostedt@goodmis.org> wrote:
>
>> On Mon, 2012-02-20 at 09:10 +0100, Ingo Molnar wrote:
>> > > 
>> > > Note, it does not seem to cause any destruction, but screw up 
>> > > profiling.
>> > 
>> > Except if the source of the NMI was not profiling, right?
>> 
>> Well, what else are NMIs used for? [...]
>
>There's various (legacy) diagnostics NMIs on various x86 
>platforms, mostly signalling some sort of deep trouble
>with the hw.
>
>Thanks,
>
>	Ingo

-- 
Sent from my mobile phone. Please excuse my brevity and lack of formatting.

Re: [tip:x86/urgent] x86/nmi: Test saved %cs in NMI to determine nested NMI case

[Linus Torvalds]

On Mon, Feb 20, 2012 at 3:47 AM, tip-bot for Steven Rostedt
<rostedt@goodmis.org> wrote:
>        /*
> +        * If %cs was not the kernel segment, then the NMI triggered in user
> +        * space, which means it is definitely not nested.
> +        */
> +       cmp $__KERNEL_CS, 16(%rsp)
> +       jne first_nmi

I don't like how you wrote 'cmp' without a size, especially with none
of the arguments then giving a size either. Usually there's a register
name or something that gives you the size, but not here.

I guess there is some default size that gas will use, but at least
some versions of gas have traditionally errored out instead of
guessing on sizes. Maybe x86-64 doesn't use those gas versions any
more, but still..

                                        Linus

Re: [tip:x86/urgent] x86/nmi: Test saved %cs in NMI to determine nested NMI case

[Steven Rostedt]

On Mon, 2012-02-20 at 10:03 -0800, Linus Torvalds wrote:
> On Mon, Feb 20, 2012 at 3:47 AM, tip-bot for Steven Rostedt
> <rostedt@goodmis.org> wrote:
> >        /*
> > +        * If %cs was not the kernel segment, then the NMI triggered in user
> > +        * space, which means it is definitely not nested.
> > +        */
> > +       cmp $__KERNEL_CS, 16(%rsp)
> > +       jne first_nmi
> 
> I don't like how you wrote 'cmp' without a size, especially with none
> of the arguments then giving a size either. Usually there's a register
> name or something that gives you the size, but not here.
> 
> I guess there is some default size that gas will use, but at least
> some versions of gas have traditionally errored out instead of
> guessing on sizes. Maybe x86-64 doesn't use those gas versions any
> more, but still..

OK, I'll send an update fix to use cmpq.

>From HPA's email:

"User space can change %cs, but it can never set it to __KERNEL_CS; 
specifically user space can never set the bottom two bits in CS to
zero."


I'm not sure it matters, but I'll change it anyway since we are
comparing an 8 byte word on the stack.

-- Steve

[tip:x86/urgent] x86: Specify a size for the cmp in the NMI handler

[tip-bot for Steven Rostedt]

Commit-ID:  a38449ef596b345e13a8f9b7d5cd9fedb8fcf921
Gitweb:     http://git.kernel.org/tip/a38449ef596b345e13a8f9b7d5cd9fedb8fcf921
Author:     Steven Rostedt <srostedt@redhat.com>
AuthorDate: Mon, 20 Feb 2012 15:29:34 -0500
Committer:  Steven Rostedt <rostedt@goodmis.org>
CommitDate: Mon, 20 Feb 2012 19:45:26 -0500

x86: Specify a size for the cmp in the NMI handler

Linus noticed that the cmp used to check if the code segment is
__KERNEL_CS or not did not specify a size. Perhaps it does not matter
as H. Peter Anvin noted that user space can not set the bottom two
bits of the %cs register. But it's best not to let the assembly choose
and change things between different versions of gas, but instead just
pick the size.

Four bytes are used to compare the saved code segment against
__KERNEL_CS. Perhaps this might mess up Xen, but we can fix that when
the time comes.

Also I noticed that there was another non-specified cmp that checks
the special stack variable if it is 1 or 0. This too probably doesn't
matter what cmp is used, but this patch uses cmpl just to make it non
ambiguous.

Link: http://lkml.kernel.org/r/CA+55aFxfAn9MWRgS3O5k2tqN5ys1XrhSFVO5_9ZAoZKDVgNfGA@mail.gmail.com

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: H. Peter Anvin <hpa@zytor.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 arch/x86/kernel/entry_64.S |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
index debd851..1333d98 100644
--- a/arch/x86/kernel/entry_64.S
+++ b/arch/x86/kernel/entry_64.S
@@ -1535,14 +1535,14 @@ ENTRY(nmi)
 	 * If %cs was not the kernel segment, then the NMI triggered in user
 	 * space, which means it is definitely not nested.
 	 */
-	cmp $__KERNEL_CS, 16(%rsp)
+	cmpl $__KERNEL_CS, 16(%rsp)
 	jne first_nmi
 
 	/*
 	 * Check the special variable on the stack to see if NMIs are
 	 * executing.
 	 */
-	cmp $1, -8(%rsp)
+	cmpl $1, -8(%rsp)
 	je nested_nmi
 
 	/*

Re: [tip:x86/urgent] x86/nmi: Test saved %cs in NMI to determine nested NMI case

[Greg KH]

On Mon, Feb 20, 2012 at 11:51:41AM -0500, Steven Rostedt wrote:
> On Mon, 2012-02-20 at 03:47 -0800, tip-bot for Steven Rostedt wrote:
> > Commit-ID:  45d5a1683c04be28abdf5c04c27b1417e0374486
> > Gitweb:     http://git.kernel.org/tip/45d5a1683c04be28abdf5c04c27b1417e0374486
> > Author:     Steven Rostedt <rostedt@goodmis.org>
> > AuthorDate: Sun, 19 Feb 2012 16:43:37 -0500
> > Committer:  Ingo Molnar <mingo@elte.hu>
> > CommitDate: Mon, 20 Feb 2012 09:09:57 +0100
> > 
> > x86/nmi: Test saved %cs in NMI to determine nested NMI case
> > 
> > Currently, the NMI handler tests if it is nested by checking the
> > special variable saved on the stack (set during NMI handling)
> > and whether the saved stack is the NMI stack as well (to prevent
> > the race when the variable is set to zero).
> > 
> > But userspace may set their %rsp to any value as long as they do
> > not derefence it, and it may make it point to the NMI stack,
> > which will prevent NMIs from triggering while the userspace app
> > is running. (I tested this, and it is indeed the case)
> > 
> > Add another check to determine nested NMIs by looking at the
> > saved %cs (code segment register) and making sure that it is the
> > kernel code segment.
> > 
> > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> > Cc: H. Peter Anvin <hpa@zytor.com>
> > Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > Cc: Linus Torvalds <torvalds@linux-foundation.org>
> > Cc: <stable@kernel.org>
> 
> Just so that we do not confuse Greg, the bug was introduced in the 3.3
> merge window. It does not exist in 3.2 or earlier.

Thanks for letting me know, I would have been confused :)

greg k-h