[PATCH 1/2 v2] x86/asm/entry/64: Enable interrupts *after* we fetch PER_CPU_VAR(old_rsp)

[PATCH 1/2 v2] x86/asm/entry/64: Enable interrupts *after* we fetch PER_CPU_VAR(old_rsp)

[Denys Vlasenko]

Without this change, it is still not possible to get rid of
PER_CPU_VAR(old_rsp) usage in switch_to: if preemption happens
while we did not fetch PER_CPU_VAR(old_rsp) and stored it in pt_regs->sp,
PER_CPU_VAR(old_rsp) gets corrupted by other task's user sp.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
CC: Linus Torvalds <torvalds@linux-foundation.org>
CC: Steven Rostedt <rostedt@goodmis.org>
CC: Ingo Molnar <mingo@kernel.org>
CC: Borislav Petkov <bp@alien8.de>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Andy Lutomirski <luto@amacapital.net>
CC: Oleg Nesterov <oleg@redhat.com>
CC: Frederic Weisbecker <fweisbec@gmail.com>
CC: Alexei Starovoitov <ast@plumgrid.com>
CC: Will Drewry <wad@chromium.org>
CC: Kees Cook <keescook@chromium.org>
CC: x86@kernel.org
CC: linux-kernel@vger.kernel.org
---

Changes since v1: don't try to be clever and use CLBR_RAX

 arch/x86/kernel/entry_64.S | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
index d86788c..3054a9d 100644
--- a/arch/x86/kernel/entry_64.S
+++ b/arch/x86/kernel/entry_64.S
@@ -241,17 +241,17 @@ GLOBAL(system_call_after_swapgs)
 	movq	%rsp,PER_CPU_VAR(old_rsp)
 	/* kernel_stack is set so that 5 slots (iret frame) are preallocated */
 	movq	PER_CPU_VAR(kernel_stack),%rsp
-	/*
-	 * No need to follow this irqs off/on section - it's straight
-	 * and short:
-	 */
-	ENABLE_INTERRUPTS(CLBR_NONE)
 	ALLOC_PT_GPREGS_ON_STACK 8		/* +8: space for orig_ax */
 	movq	%rcx,RIP(%rsp)
 	movq	PER_CPU_VAR(old_rsp),%rcx
 	movq	%r11,EFLAGS(%rsp)
 	movq	%rcx,RSP(%rsp)
+	/*
+	 * No need to follow this irqs off/on section - it's straight
+	 * and short:
+	 */
+	ENABLE_INTERRUPTS(CLBR_NONE)
 	movq_cfi rax,ORIG_RAX
 	SAVE_C_REGS_EXCEPT_RAX_RCX_R11
 	movq	$-ENOSYS,RAX(%rsp)
 	CFI_REL_OFFSET rip,RIP
-- 
1.8.1.4

Re: [PATCH 1/2 v2] x86/asm/entry/64: Enable interrupts *after* we fetch PER_CPU_VAR(old_rsp)

[Borislav Petkov]

On Tue, Mar 17, 2015 at 02:52:24PM +0100, Denys Vlasenko wrote:
> Without this change, it is still not possible to get rid of
> PER_CPU_VAR(old_rsp) usage in switch_to: if preemption happens
> while we did not fetch PER_CPU_VAR(old_rsp) and stored it in pt_regs->sp,
> PER_CPU_VAR(old_rsp) gets corrupted by other task's user sp.
> 
> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> CC: Linus Torvalds <torvalds@linux-foundation.org>
> CC: Steven Rostedt <rostedt@goodmis.org>
> CC: Ingo Molnar <mingo@kernel.org>
> CC: Borislav Petkov <bp@alien8.de>
> CC: "H. Peter Anvin" <hpa@zytor.com>
> CC: Andy Lutomirski <luto@amacapital.net>
> CC: Oleg Nesterov <oleg@redhat.com>
> CC: Frederic Weisbecker <fweisbec@gmail.com>
> CC: Alexei Starovoitov <ast@plumgrid.com>
> CC: Will Drewry <wad@chromium.org>
> CC: Kees Cook <keescook@chromium.org>
> CC: x86@kernel.org
> CC: linux-kernel@vger.kernel.org
> ---
> 
> Changes since v1: don't try to be clever and use CLBR_RAX
> 
>  arch/x86/kernel/entry_64.S | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

Reported-and-tested-by: Borislav Petkov <bp@suse.de>

Thanks Denys!

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH 1/2 v2] x86/asm/entry/64: Enable interrupts *after* we fetch PER_CPU_VAR(old_rsp)

[Ingo Molnar]

* Denys Vlasenko <dvlasenk@redhat.com> wrote:

> Without this change, it is still not possible to get rid of
> PER_CPU_VAR(old_rsp) usage in switch_to: if preemption happens
> while we did not fetch PER_CPU_VAR(old_rsp) and stored it in pt_regs->sp,
> PER_CPU_VAR(old_rsp) gets corrupted by other task's user sp.

Well, wouldn't this be a much clearer explanation:

"We want to use PER_CPU_VAR(old_rsp) as a simple temporary register, 
 to shuffle user-space RSP into (and from) when we set up the system 
 call stack frame. At that point we cannot shuffle values into general 
 purpose registers, because we have not saved them yet.

 To be able to do this shuffling into a memory location, we must be 
 atomic and must not be preempted while we do the shuffling, otherwise 
 the 'temporary' register gets overwritten by some other task's 
 temporary register contents ..."

Agreed?

Thanks,

	Ingo

Re: [PATCH 1/2 v2] x86/asm/entry/64: Enable interrupts *after* we fetch PER_CPU_VAR(old_rsp)

[Denys Vlasenko]

On 03/17/2015 03:36 PM, Ingo Molnar wrote:
> 
> * Denys Vlasenko <dvlasenk@redhat.com> wrote:
> 
>> Without this change, it is still not possible to get rid of
>> PER_CPU_VAR(old_rsp) usage in switch_to: if preemption happens
>> while we did not fetch PER_CPU_VAR(old_rsp) and stored it in pt_regs->sp,
>> PER_CPU_VAR(old_rsp) gets corrupted by other task's user sp.
> 
> Well, wouldn't this be a much clearer explanation:
> 
> "We want to use PER_CPU_VAR(old_rsp) as a simple temporary register, 
>  to shuffle user-space RSP into (and from) when we set up the system 
>  call stack frame. At that point we cannot shuffle values into general 
>  purpose registers, because we have not saved them yet.
> 
>  To be able to do this shuffling into a memory location, we must be 
>  atomic and must not be preempted while we do the shuffling, otherwise 
>  the 'temporary' register gets overwritten by some other task's 
>  temporary register contents ..."
> 
> Agreed?


Sounds good to me.

[tip:x86/asm] x86/asm/entry/64: Enable interrupts *after* we fetch PER_CPU_VAR(old_rsp)

[tip-bot for Denys Vlasenko]

Commit-ID:  33db1fd48ac3d90385b412b41a8a6525096ac6d5
Gitweb:     http://git.kernel.org/tip/33db1fd48ac3d90385b412b41a8a6525096ac6d5
Author:     Denys Vlasenko <dvlasenk@redhat.com>
AuthorDate: Tue, 17 Mar 2015 14:52:24 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 17 Mar 2015 16:01:40 +0100

x86/asm/entry/64: Enable interrupts *after* we fetch PER_CPU_VAR(old_rsp)

We want to use PER_CPU_VAR(old_rsp) as a simple temporary register,
to shuffle user-space RSP into (and from) when we set up the system
call stack frame. At that point we cannot shuffle values into general
purpose registers, because we have not saved them yet.

To be able to do this shuffling into a memory location, we must be
atomic and must not be preempted while we do the shuffling, otherwise
the 'temporary' register gets overwritten by some other task's
temporary register contents ...

Tested-by: Borislav Petkov <bp@alien8.de>
Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
Acked-by: Borislav Petkov <bp@alien8.de>
Cc: Alexei Starovoitov <ast@plumgrid.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Will Drewry <wad@chromium.org>
Link: http://lkml.kernel.org/r/1426600344-8254-1-git-send-email-dvlasenk@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/entry_64.S | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
index d86788c..aed3f11 100644
--- a/arch/x86/kernel/entry_64.S
+++ b/arch/x86/kernel/entry_64.S
@@ -241,16 +241,16 @@ GLOBAL(system_call_after_swapgs)
 	movq	%rsp,PER_CPU_VAR(old_rsp)
 	/* kernel_stack is set so that 5 slots (iret frame) are preallocated */
 	movq	PER_CPU_VAR(kernel_stack),%rsp
-	/*
-	 * No need to follow this irqs off/on section - it's straight
-	 * and short:
-	 */
-	ENABLE_INTERRUPTS(CLBR_NONE)
 	ALLOC_PT_GPREGS_ON_STACK 8		/* +8: space for orig_ax */
 	movq	%rcx,RIP(%rsp)
 	movq	PER_CPU_VAR(old_rsp),%rcx
 	movq	%r11,EFLAGS(%rsp)
 	movq	%rcx,RSP(%rsp)
+	/*
+	 * No need to follow this irqs off/on section - it's straight
+	 * and short:
+	 */
+	ENABLE_INTERRUPTS(CLBR_NONE)
 	movq_cfi rax,ORIG_RAX
 	SAVE_C_REGS_EXCEPT_RAX_RCX_R11
 	movq	$-ENOSYS,RAX(%rsp)