Skip to contentSkip to conversation

x86: execve and sigreturn syscalls must return via iret

PATCH In review Brian Gerst 4 participants 5 messages Mar 21, 2015 – Mar 23, 2015

Subsystems (2)
x86 ptrace support
lore
Share & tools
lore

Brian Gerst

author
#0 lore raw

PATCH 1/1 x86: execve and sigreturn syscalls must return via iret

Both the execve and sigreturn family of syscalls have the ability to change registers in ways that may not be compatabile with the syscall path they were called from. In particular, sysret and sysexit can't handle non-default %cs and %ss, and some bits in eflags. These syscalls have stubs that are hardcoded to jump to the iret path, and not return to the original syscall path. Commit 76f5df43cab5e765c0bd42289103e8f625813ae1 (Always allocate a complete "struct pt_regs" on the kernel stack) recently changed this for some 32-bit compat syscalls, but introduced a bug where execve from a 32-bit program to a 64-bit program would fail because it still returned via sysretl. This caused Wine to fail when built for both 32-bit and 64-bit.

This patch sets TIF_NOTIFY_RESUME for execve and sigreturn so that the iret path is always taken on exit to userspace.

Signed-off-by Brian Gerst <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Denys Vlasenko <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc H. Peter Anvin <[email protected]>
Cc Linus Torvalds <[email protected]>
---
 arch/x86/ia32/ia32_signal.c        | 2 ++
 arch/x86/include/asm/ptrace.h      | 2 +-
 arch/x86/include/asm/thread_info.h | 7 +++++++
 arch/x86/kernel/process_32.c       | 6 +-----
 arch/x86/kernel/process_64.c       | 1 +
 arch/x86/kernel/signal.c           | 2 ++
 6 files changed, 14 insertions(+), 6 deletions(-)
arch/x86/ia32/ia32_signal.c +2 -0
index d0165c9..1f5e2b0 100644
@@ -203,6 +203,8 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
 
 	err |= restore_xstate_sig(buf, 1);
 
+	force_iret();
+
 	return err;
 }
arch/x86/include/asm/ptrace.h +1 -1
index 74bb2e0..83b874d 100644
@@ -251,7 +251,7 @@ static inline unsigned long regs_get_kernel_stack_nth(struct pt_regs *regs,
  */
 #define arch_ptrace_stop_needed(code, info)				\
 ({									\
-	set_thread_flag(TIF_NOTIFY_RESUME);				\
+	force_iret();							\
 	false;								\
 })
arch/x86/include/asm/thread_info.h +7 -0
index ba115eb..861c1dd 100644
@@ -260,6 +260,13 @@ static inline bool is_ia32_task(void)
 #endif
 	return false;
 }
+
+/*
+ * force syscall return via iret by making it look as if there was
+ * some work pending.
+*/
+#define force_iret() set_thread_flag(TIF_NOTIFY_RESUME)
+
 #endif	/* !__ASSEMBLY__ */
 
 #ifndef __ASSEMBLY__
arch/x86/kernel/process_32.c +1 -5
index 1b9963f..26c596d 100644
@@ -206,11 +206,7 @@ start_thread(struct pt_regs *regs, unsigned long new_ip, unsigned long new_sp)
 	regs->ip		= new_ip;
 	regs->sp		= new_sp;
 	regs->flags		= X86_EFLAGS_IF;
-	/*
-	 * force it to the iret return path by making it look as if there was
-	 * some work pending.
-	 */
-	set_thread_flag(TIF_NOTIFY_RESUME);
+	force_iret();
 }
 EXPORT_SYMBOL_GPL(start_thread);
arch/x86/kernel/process_64.c +1 -0
index 97f5658..da8b745 100644
@@ -239,6 +239,7 @@ start_thread_common(struct pt_regs *regs, unsigned long new_ip,
 	regs->cs		= _cs;
 	regs->ss		= _ss;
 	regs->flags		= X86_EFLAGS_IF;
+	force_iret();
 }
 
 void
arch/x86/kernel/signal.c +2 -0
index edcb862..eaa2c5e 100644
@@ -108,6 +108,8 @@ int restore_sigcontext(struct pt_regs *regs, struct sigcontext __user *sc,
 
 	err |= restore_xstate_sig(buf, config_enabled(CONFIG_X86_32));
 
+	force_iret();
+
 	return err;
 }
Signature · 1 line
2.1.0
#1 lore raw
Quoted 21 lines · Brian Gerst

Both the execve and sigreturn family of syscalls have the ability to change registers in ways that may not be compatabile with the syscall path they were called from. In particular, sysret and sysexit can't handle non-default %cs and %ss, and some bits in eflags. These syscalls have stubs that are hardcoded to jump to the iret path, and not return to the original syscall path. Commit 76f5df43cab5e765c0bd42289103e8f625813ae1 (Always allocate a complete "struct pt_regs" on the kernel stack) recently changed this for some 32-bit compat syscalls, but introduced a bug where execve from a 32-bit program to a 64-bit program would fail because it still returned via sysretl. This caused Wine to fail when built for both 32-bit and 64-bit.

This patch sets TIF_NOTIFY_RESUME for execve and sigreturn so that the iret path is always taken on exit to userspace.

Signed-off-by Brian Gerst <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Denys Vlasenko <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc H. Peter Anvin <[email protected]>
Cc Linus Torvalds <[email protected]>
Quoted 8 lines
---
 arch/x86/ia32/ia32_signal.c        | 2 ++
 arch/x86/include/asm/ptrace.h      | 2 +-
 arch/x86/include/asm/thread_info.h | 7 +++++++
 arch/x86/kernel/process_32.c       | 6 +-----
 arch/x86/kernel/process_64.c       | 1 +
 arch/x86/kernel/signal.c           | 2 ++
 6 files changed, 14 insertions(+), 6 deletions(-)

Applied the fix to tip:x86/asm, thanks Brian!

arch/x86/include/asm/thread_info.h static inline bool is_ia32_task(void) · line 268

Quoted 6 lines
arch/x86/include/asm/thread_info.h +6 -0
+
+/*
+ * force syscall return via iret by making it look as if there was
+ * some work pending.
+*/
+#define force_iret() set_thread_flag(TIF_NOTIFY_RESUME)

I extended this comment to:

/*
* Force syscall return via IRET by making it look as if there was
* some work pending. IRET is our most capable (but slowest) syscall
* return path, which is able to restore modified SS, CS and certain
* EFLAGS values that other (fast) syscall return instructions
* are not able to restore properly.
*/
#define force_iret() set_thread_flag(TIF_NOTIFY_RESUME)

Just to preserve the underlying reason for force_iret() for the future and such.

Btw., it might be a worthwile optimization to detect non-standard SS, CS and EFLAGS values and only force_iret() in that case, that will speed up 99.9999% of execve() and sigreturn() syscalls and only force the 'weird' process startup modes into the slow return path.

Quoted 1 line

From an access security POV it should be a relatively safe

optimization: if we get it wrong then we don't allow certain ABI angles, but we won't make the kernel unsafe AFAICS.

Thanks,

Ingo

author
#2 lore raw
Quoted 53 lines · Ingo Molnar
Quoted 21 lines · Brian Gerst

Both the execve and sigreturn family of syscalls have the ability to change registers in ways that may not be compatabile with the syscall path they were called from. In particular, sysret and sysexit can't handle non-default %cs and %ss, and some bits in eflags. These syscalls have stubs that are hardcoded to jump to the iret path, and not return to the original syscall path. Commit 76f5df43cab5e765c0bd42289103e8f625813ae1 (Always allocate a complete "struct pt_regs" on the kernel stack) recently changed this for some 32-bit compat syscalls, but introduced a bug where execve from a 32-bit program to a 64-bit program would fail because it still returned via sysretl. This caused Wine to fail when built for both 32-bit and 64-bit.

This patch sets TIF_NOTIFY_RESUME for execve and sigreturn so that the iret path is always taken on exit to userspace.

Signed-off-by Brian Gerst <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Denys Vlasenko <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc H. Peter Anvin <[email protected]>
Cc Linus Torvalds <[email protected]>
Quoted 8 lines
---
 arch/x86/ia32/ia32_signal.c        | 2 ++
 arch/x86/include/asm/ptrace.h      | 2 +-
 arch/x86/include/asm/thread_info.h | 7 +++++++
 arch/x86/kernel/process_32.c       | 6 +-----
 arch/x86/kernel/process_64.c       | 1 +
 arch/x86/kernel/signal.c           | 2 ++
 6 files changed, 14 insertions(+), 6 deletions(-)

Applied the fix to tip:x86/asm, thanks Brian!

arch/x86/include/asm/thread_info.h static inline bool is_ia32_task(void) · line 268

Quoted 6 lines
arch/x86/include/asm/thread_info.h +6 -0
+
+/*
+ * force syscall return via iret by making it look as if there was
+ * some work pending.
+*/
+#define force_iret() set_thread_flag(TIF_NOTIFY_RESUME)

I extended this comment to:

/*
* Force syscall return via IRET by making it look as if there was
* some work pending. IRET is our most capable (but slowest) syscall
* return path, which is able to restore modified SS, CS and certain
* EFLAGS values that other (fast) syscall return instructions
* are not able to restore properly.
*/
#define force_iret() set_thread_flag(TIF_NOTIFY_RESUME)

Quoted 7 lines

Just to preserve the underlying reason for force_iret() for the future and such.

Btw., it might be a worthwile optimization to detect non-standard SS, CS and EFLAGS values and only force_iret() in that case, that will speed up 99.9999% of execve() and sigreturn() syscalls and only force the 'weird' process startup modes into the slow return path.

sysret/sysexit also can't restore rcx and r11/rdx. This would not work for execve, since it sets those registers to zero. It could possibly work for sigreturn if the signal interrupted a syscall. We already have the opportunistic sysret code for 64-bit returns.

--
Brian Gerst

#3 lore raw
Quoted 62 lines · Brian Gerst
Quoted 53 lines · Ingo Molnar
Quoted 21 lines · Brian Gerst

Both the execve and sigreturn family of syscalls have the ability to change registers in ways that may not be compatabile with the syscall path they were called from. In particular, sysret and sysexit can't handle non-default %cs and %ss, and some bits in eflags. These syscalls have stubs that are hardcoded to jump to the iret path, and not return to the original syscall path. Commit 76f5df43cab5e765c0bd42289103e8f625813ae1 (Always allocate a complete "struct pt_regs" on the kernel stack) recently changed this for some 32-bit compat syscalls, but introduced a bug where execve from a 32-bit program to a 64-bit program would fail because it still returned via sysretl. This caused Wine to fail when built for both 32-bit and 64-bit.

This patch sets TIF_NOTIFY_RESUME for execve and sigreturn so that the iret path is always taken on exit to userspace.

Signed-off-by Brian Gerst <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Denys Vlasenko <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc H. Peter Anvin <[email protected]>
Cc Linus Torvalds <[email protected]>
Quoted 8 lines
---
 arch/x86/ia32/ia32_signal.c        | 2 ++
 arch/x86/include/asm/ptrace.h      | 2 +-
 arch/x86/include/asm/thread_info.h | 7 +++++++
 arch/x86/kernel/process_32.c       | 6 +-----
 arch/x86/kernel/process_64.c       | 1 +
 arch/x86/kernel/signal.c           | 2 ++
 6 files changed, 14 insertions(+), 6 deletions(-)

Applied the fix to tip:x86/asm, thanks Brian!

arch/x86/include/asm/thread_info.h static inline bool is_ia32_task(void) · line 268

Quoted 6 lines
arch/x86/include/asm/thread_info.h +6 -0
+
+/*
+ * force syscall return via iret by making it look as if there was
+ * some work pending.
+*/
+#define force_iret() set_thread_flag(TIF_NOTIFY_RESUME)

I extended this comment to:

/*
* Force syscall return via IRET by making it look as if there was
* some work pending. IRET is our most capable (but slowest) syscall
* return path, which is able to restore modified SS, CS and certain
* EFLAGS values that other (fast) syscall return instructions
* are not able to restore properly.
*/
#define force_iret() set_thread_flag(TIF_NOTIFY_RESUME)

Quoted 7 lines

Just to preserve the underlying reason for force_iret() for the future and such.

Btw., it might be a worthwile optimization to detect non-standard SS, CS and EFLAGS values and only force_iret() in that case, that will speed up 99.9999% of execve() and sigreturn() syscalls and only force the 'weird' process startup modes into the slow return path.

Quoted 4 lines

sysret/sysexit also can't restore rcx and r11/rdx. This would not work for execve, since it sets those registers to zero. It could possibly work for sigreturn if the signal interrupted a syscall. We already have the opportunistic sysret code for 64-bit returns.

I'd like to extend opportunistic sysret to 32-bit, as either only opportunistic sysretl or perhaps opportunistic sysexitl *and* sysretl depending on regs.

Why only sysretl as a possibility? I think that Intel CPUs allow sysretl despite not having the matching syscall instruction. If we could make everything work without using sysexit, the code would be simplified.

We could add helpers like opportunistic_clobber_rcx_r11(regs) and opportunistic_clobber_ecx_edx(regs) that would set us use it reasonably cleanly.

--Andy

merge bot
#4 lore raw

[tip:x86/asm] x86/asm/entry: Fix execve() and sigreturn() syscalls to always return via IRET

In reply to Brian Gerst
x86/asm/entry: Fix execve() and sigreturn() syscalls to always return via IRET
Commit-ID:  1daeaa315164c60b937f56fe3848d4328c358eba
Gitweb:     http://git.kernel.org/tip/1daeaa315164c60b937f56fe3848d4328c358eba
Author:     Brian Gerst <[email protected]>
AuthorDate: Sat, 21 Mar 2015 18:54:21 -0400
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 23 Mar 2015 08:52:46 +0100

x86/asm/entry: Fix execve() and sigreturn() syscalls to always return via IRET

Both the execve() and sigreturn() family of syscalls have the ability to change registers in ways that may not be compatabile with the syscall path they were called from.

In particular, SYSRET and SYSEXIT can't handle non-default %cs and %ss, and some bits in eflags.

These syscalls have stubs that are hardcoded to jump to the IRET path, and not return to the original syscall path.

The following commit:

76f5df43cab5e76 ("Always allocate a complete "struct pt_regs" on the kernel stack")

recently changed this for some 32-bit compat syscalls, but introduced a bug where execve from a 32-bit program to a 64-bit program would fail because it still returned via SYSRETL. This caused Wine to fail when built for both 32-bit and 64-bit.

This patch sets TIF_NOTIFY_RESUME for execve() and sigreturn() so that the IRET path is always taken on exit to userspace.

Signed-off-by Brian Gerst <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Denys Vlasenko <[email protected]>
Cc H. Peter Anvin <[email protected]>
Cc Linus Torvalds <[email protected]>
Link http://lkml.kernel.org/r/[email protected] open here
[ Improved the changelog and comments. ]
Signed-off-by Ingo Molnar <[email protected]>
---
 arch/x86/ia32/ia32_signal.c        |  2 ++
 arch/x86/include/asm/ptrace.h      |  2 +-
 arch/x86/include/asm/thread_info.h | 10 ++++++++++
 arch/x86/kernel/process_32.c       |  6 +-----
 arch/x86/kernel/process_64.c       |  1 +
 arch/x86/kernel/signal.c           |  2 ++
 6 files changed, 17 insertions(+), 6 deletions(-)
arch/x86/ia32/ia32_signal.c +2 -0
index d0165c9..1f5e2b0 100644
@@ -203,6 +203,8 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
 
 	err |= restore_xstate_sig(buf, 1);
 
+	force_iret();
+
 	return err;
 }
arch/x86/include/asm/ptrace.h +1 -1
index 74bb2e0..83b874d 100644
@@ -251,7 +251,7 @@ static inline unsigned long regs_get_kernel_stack_nth(struct pt_regs *regs,
  */
 #define arch_ptrace_stop_needed(code, info)				\
 ({									\
-	set_thread_flag(TIF_NOTIFY_RESUME);				\
+	force_iret();							\
 	false;								\
 })
arch/x86/include/asm/thread_info.h +10 -0
index ba115eb..0abf7ab 100644
@@ -260,6 +260,16 @@ static inline bool is_ia32_task(void)
 #endif
 	return false;
 }
+
+/*
+ * Force syscall return via IRET by making it look as if there was
+ * some work pending. IRET is our most capable (but slowest) syscall
+ * return path, which is able to restore modified SS, CS and certain
+ * EFLAGS values that other (fast) syscall return instructions
+ * are not able to restore properly.
+ */
+#define force_iret() set_thread_flag(TIF_NOTIFY_RESUME)
+
 #endif	/* !__ASSEMBLY__ */
 
 #ifndef __ASSEMBLY__
arch/x86/kernel/process_32.c +1 -5
index 1b9963f..26c596d 100644
@@ -206,11 +206,7 @@ start_thread(struct pt_regs *regs, unsigned long new_ip, unsigned long new_sp)
 	regs->ip		= new_ip;
 	regs->sp		= new_sp;
 	regs->flags		= X86_EFLAGS_IF;
-	/*
-	 * force it to the iret return path by making it look as if there was
-	 * some work pending.
-	 */
-	set_thread_flag(TIF_NOTIFY_RESUME);
+	force_iret();
 }
 EXPORT_SYMBOL_GPL(start_thread);
arch/x86/kernel/process_64.c +1 -0
index 97f5658..da8b745 100644
@@ -239,6 +239,7 @@ start_thread_common(struct pt_regs *regs, unsigned long new_ip,
 	regs->cs		= _cs;
 	regs->ss		= _ss;
 	regs->flags		= X86_EFLAGS_IF;
+	force_iret();
 }
 
 void
arch/x86/kernel/signal.c +2 -0
index edcb862..eaa2c5e 100644
@@ -108,6 +108,8 @@ int restore_sigcontext(struct pt_regs *regs, struct sigcontext __user *sc,
 
 	err |= restore_xstate_sig(buf, config_enabled(CONFIG_X86_32));
 
+	force_iret();
+
 	return err;
 }