exit cleanups

I do not see panic calling rewind_stack_do_exit anywhere, nor can I find anywhere in the history where doublefault_shim has called rewind_stack_do_exit. So I don't think this comment was ever actually correct.

Cc Andy Lutomirski <[email protected]>

Fixes 7d8d8cfdee9a ("x86/doublefault/32: Rewrite the x86_32 #DF handler and unify with 64-bit")

Signed-off-by "Eric W. Biederman" <[email protected]>

On nds32, openrisc, s390, sh, and xtensa the function die never returns. Mark die __noreturn so that no one expects die to return. Remove the do_exit calls after die as they will never be reached.

Quoted 3 lines · Eric W. Biederman

On nds32, openrisc, s390, sh, and xtensa the function die never returns. Mark die __noreturn so that no one expects die to return. Remove the do_exit calls after die as they will never be reached.

Maybe note that the "bust_spinlocks" calls are also redundant, since they're in die(). I note that is a "mismatch" between the do_kill() in die() (SIGSEGV) and after die() (SIGKILL). This patch makes no behavioral change (the first caller would "win"), but I thought I'd note it in case some architecture would prefer a different signal.

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 kernel/reboot.c | 1 -
 1 file changed, 1 deletion(-)

kernel/reboot.c +0 -0

index f7440c0c7e43..d6e0f9fb7f04 100644

kernel/reboot.c +0 -0

 	case LINUX_REBOOT_CMD_POWER_OFF:
 		kernel_power_off();

Signature · 1 line

2.20.1

The call to do_exit in do_sparc_fault immediately follows a call to unhandled_fault. The function unhandled_fault never returns. This means the call to do_exit can never be reached.

Cc David Miller <[email protected]>

Cc [email protected]

Fixes 2.3.41

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 arch/sparc/mm/fault_32.c | 1 -
 1 file changed, 1 deletion(-)

arch/sparc/mm/fault_32.c +0 -1

index fa858626b85b..90dc4ae315c8 100644
@@ -248,7 +248,6 @@ asmlinkage void do_sparc_fault(struct pt_regs *regs, int text_fault, int write,
 	}
 
 	unhandled_fault(address, tsk, regs);
-	do_exit(SIGKILL);
 
 /*
  * We ran out of memory, or some other thing happened to us that made

Signature · 1 line

2.20.1

When an instruction to save or restore a register from the stack fails in _save_fp_context or _restore_fp_context return with -EFAULT. This change was made to r2300_fpu.S[1] but it looks like it got lost with the introduction of EX2[2]. This is also what the other implementation of _save_fp_context and _restore_fp_context in r4k_fpu.S does, and what is needed for the callers to be able to handle the error.

Furthermore calling do_exit(SIGSEGV) from bad_stack is wrong because it does not terminate the entire process it just terminates a single thread.

As the changed code was the only caller of arch/mips/kernel/syscall.c:bad_stack remove the problematic and now unused helper function.

Cc: Thomas Bogendoerfer <[email protected]>
Cc: Maciej Rozycki <[email protected]>
Cc: [email protected]
[1] 35938a00ba86 ("MIPS: Fix ISA I FP sigcontext access violation handling")
[2] f92722dc4545 ("MIPS: Correct MIPS I FP sigcontext layout")

Fixes f92722dc4545 ("MIPS: Correct MIPS I FP sigcontext layout")

Signed-off-by "Eric W. Biederman" <[email protected]>

When an instruction to save or restore a register from the stack fails in _save_fp_context or _restore_fp_context return with -EFAULT. This change was made to r2300_fpu.S[1] but it looks like it got lost with the introduction of EX2[2]. This is also what the other implementation of _save_fp_context and _restore_fp_context in r4k_fpu.S does, and what is needed for the callers to be able to handle the error.

Quoted 6 lines · Eric W. Biederman

When an instruction to save or restore a register from the stack fails in _save_fp_context or _restore_fp_context return with -EFAULT. This change was made to r2300_fpu.S[1] but it looks like it got lost with the introduction of EX2[2]. This is also what the other implementation of _save_fp_context and _restore_fp_context in r4k_fpu.S does, and what is needed for the callers to be able to handle the error.

Umm, right, good catch, thanks! I think this ought to be backported.

Acked-by Maciej W. Rozycki <[email protected]>

When an instruction to save or restore a register from the stack fails in _save_fp_context or _restore_fp_context return with -EFAULT. This change was made to r2300_fpu.S[1] but it looks like it got lost with the introduction of EX2[2]. This is also what the other implementation of _save_fp_context and _restore_fp_context in r4k_fpu.S does, and what is needed for the callers to be able to handle the error.

Furthermore calling do_exit(SIGSEGV) from bad_stack is wrong because it does not terminate the entire process it just terminates a single thread.

As the changed code was the only caller of arch/mips/kernel/syscall.c:bad_stack remove the problematic and now unused helper function.

Cc: Thomas Bogendoerfer <[email protected]>
Cc: Maciej Rozycki <[email protected]>
Cc: [email protected]
[1] 35938a00ba86 ("MIPS: Fix ISA I FP sigcontext access violation handling")
[2] f92722dc4545 ("MIPS: Correct MIPS I FP sigcontext layout")

Fixes f92722dc4545 ("MIPS: Correct MIPS I FP sigcontext layout")

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 arch/mips/kernel/r2300_fpu.S | 4 ++--
 arch/mips/kernel/syscall.c   | 9 ---------
 2 files changed, 2 insertions(+), 11 deletions(-)

+ force_sig(SIGKILL);

Quoted 2 lines · Eric W. Biederman

+ force_sig(SIGKILL);

I wonder if SIGFPE would be a more intuitive thing.

Doesn't really matter, this is a "doesn't happen" event anyway, but that was just my reaction to reading the patch.

Today the sh code allocates memory the first time a process uses the fpu. If that memory allocation fails, kill the affected task with force_sig(SIGKILL) rather than do_group_exit(SIGKILL).

Calling do_group_exit from an exception handler can potentially lead to dead locks as do_group_exit is not designed to be called from interrupt context. Instead use force_sig(SIGKILL) to kill the userspace process. Sending signals in general and force_sig in particular has been tested from interrupt context so there should be no problems.

Cc Yoshinori Sato <[email protected]>

Cc Rich Felker <[email protected]>

Cc [email protected]

Fixes 0ea820cf9bf5 ("sh: Move over to dynamically allocated FPU context.")

Signed-off-by "Eric W. Biederman" <[email protected]>

If the register state may be partial and corrupted instead of calling do_exit, call force_sigsegv(SIGSEGV). Which properly kills the process with SIGSEGV and does not let any more userspace code execute, instead of just killing one thread of the process and potentially confusing everything.

Cc: Michael Ellerman <[email protected]>
Cc: Benjamin Herrenschmidt <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: [email protected]
History-tree: git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git

Fixes 756f1ae8a44e ("PPC32: Rework signal code and add a swapcontext system call.")

Fixes 04879b04bf50 ("[PATCH] ppc64: VMX (Altivec) support & signal32 rework, from Ben Herrenschmidt")

Signed-off-by "Eric W. Biederman" <[email protected]>

The function setup_tsb_params has exactly one caller tsb_grow. The function tsb_grow passes in a tsb_bytes value that is between 8192 and 1048576 inclusive, and is guaranteed to be a power of 2. The function setup_tsb_params verifies this property with a switch statement and then prints an error and causes the task to exit if this is not true.

In practice that print statement can never be reached because tsb_grow never passes in a bad tsb_size. So if tsb_size ever gets a bad value that is a kernel bug.

So replace the do_exit which is effectively an open coded version of BUG() with an actuall call to BUG(). Making it clearer that this is a case that can never, and should never happen.

Cc David Miller <[email protected]>

Cc [email protected]

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 arch/sparc/mm/tsb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/sparc/mm/tsb.c +0 -0

index 0dce4b7ff73e..912205787161 100644

The function save_v86_state is only called when userspace was operating in vm86 mode before entering the kernel. Not having vm86 state in the task_struct should never happen. So transform the hand rolled BUG_ON into an actual BUG_ON to make it clear what is happening.

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc [email protected]

Cc H Peter Anvin <[email protected]>

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 arch/x86/kernel/vm86_32.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

arch/x86/kernel/vm86_32.c +2 -4

index e5a7a10a0164..63486da77272 100644
@@ -106,10 +106,8 @@ void save_v86_state(struct kernel_vm86_regs *regs, int retval)
 	 */
 	local_irq_enable();
 
-	if (!vm86 || !vm86->user_vm86) {
-		pr_alert("no user_vm86: BAD\n");
-		do_exit(SIGSEGV);
-	}
+	BUG_ON(!vm86 || !vm86->user_vm86);
+
 	set_flags(regs->pt.flags, VEFLAGS, X86_EFLAGS_VIF | vm86->veflags_mask);
 	user = vm86->user_vm86;
 

Signature · 1 line

2.20.1

The function save_v86_state is only called when userspace was operating in vm86 mode before entering the kernel. Not having vm86 state in the task_struct should never happen. So transform the hand rolled BUG_ON into an actual BUG_ON to make it clear what is happening.

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc [email protected]

Cc H Peter Anvin <[email protected]>

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 arch/x86/kernel/vm86_32.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

arch/x86/kernel/vm86_32.c +2 -4

index e5a7a10a0164..63486da77272 100644
@@ -106,10 +106,8 @@ void save_v86_state(struct kernel_vm86_regs *regs, int retval)
 	 */
 	local_irq_enable();
 
-	if (!vm86 || !vm86->user_vm86) {
-		pr_alert("no user_vm86: BAD\n");
-		do_exit(SIGSEGV);
-	}
+	BUG_ON(!vm86 || !vm86->user_vm86);
+
 	set_flags(regs->pt.flags, VEFLAGS, X86_EFLAGS_VIF | vm86->veflags_mask);
 	user = vm86->user_vm86;

Quoted 5 lines · Eric W. Biederman

The function save_v86_state is only called when userspace was operating in vm86 mode before entering the kernel. Not having vm86 state in the task_struct should never happen. So transform the hand rolled BUG_ON into an actual BUG_ON to make it clear what is happening.

Now that this change has been merged into Linus' tree I have a report that it is possible to trigger this new BUG_ON. Which obviously is not good.

We could revert the change but I think that would just be shooting the messenger.

Does anyone have an idea where to start to track down what is going on?

A very quick skim through the code suggests that the only code path that calls save_v86_state that has not already accessed is current->thread.vm86 is handle_signal.

Another quick look suggests that the only place where X86_VM_MASK gets set in eflags is in do_sys_vm86. So it appears do_sys_vm86 must be called before v8086_mode returns true in handle_signal.

Which seems to suggest that the bug on can't trigger.

But that is obviously wrong.

I will keep digging but if anyone has some ideas that would be appreciated.

Eric

Quoted 32 lines · Eric W. Biederman

Quoted 5 lines · Eric W. Biederman

The function save_v86_state is only called when userspace was operating in vm86 mode before entering the kernel. Not having vm86 state in the task_struct should never happen. So transform the hand rolled BUG_ON into an actual BUG_ON to make it clear what is happening.

Now that this change has been merged into Linus' tree I have a report that it is possible to trigger this new BUG_ON. Which obviously is not good.

We could revert the change but I think that would just be shooting the messenger.

Does anyone have an idea where to start to track down what is going on?

A very quick skim through the code suggests that the only code path that calls save_v86_state that has not already accessed is current->thread.vm86 is handle_signal.

Another quick look suggests that the only place where X86_VM_MASK gets set in eflags is in do_sys_vm86. So it appears do_sys_vm86 must be called before v8086_mode returns true in handle_signal.

Which seems to suggest that the bug on can't trigger.

But that is obviously wrong.

I will keep digging but if anyone has some ideas that would be appreciated.

Quoted 1 line

Eric

It's possible that a null pointer was passed to the vm86 syscall. Since vm86 mode usually requires memory to be mapped at address 0 this wouldn't trigger a fault when reading the vm86_struct data. It should be fine to remove !vm86->user_vm86 from the BUG_ON(), since the write to userspace can handle a fault.

Still user space would have had to have mapped address 0 to get that value set in do_sys_vm86.

Quoted 3 lines · Eric W. Biederman

Still user space would have had to have mapped address 0 to get that value set in do_sys_vm86.

You have to map address 0 anyway just to get vm86 mode to work.

vm86 mode fundamentally requires the low 1MB of virtual memory to be mapped, since there is no virtual memory offset in the vm86 model.

Still if we are going to check for userspace being silly that it should be in do_sys_vm86.

But I could see some quick test hack doing it - the IVT at boot is actually not at zero, but at fffxxxxx. 8086 is magic.

Greeting,

FYI, we noticed the following commit (built with gcc-9):

commit: 1a4d21a23c4ca7467726be7db9ae8077a62b2c62 ("signal/vm86_32: Replace open coded BUG_ON with an actual BUG_ON")
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master

in testcase: trinity
version: trinity-static-i386-x86_64-1c734c75-1_2020-01-06 with following parameters:

[ 70.645554][ T3747] kernel BUG at arch/x86/kernel/vm86_32.c:109!
[ 70.646185][ T3747] invalid opcode: 0000 [#1] SMP
[ 70.646682][ T3747] CPU: 0 PID: 3747 Comm: trinity-c6 Not tainted 5.15.0-rc1-00009-g1a4d21a23c4c #1
[ 70.647598][ T3747] EIP: save_v86_state (arch/x86/kernel/vm86_32.c:109 (discriminator 3))
[ 70.648113][ T3747] Code: 89 c3 64 8b 35 60 b8 25 c2 83 ec 08 89 55 f0 8b 96 10 19 00 00 89 55 ec e8 c6 2d 0c 00 fb 8b 55 ec 85 d2 74 05 83 3a 00 75 02 <0f> 0b 8b 86 10 19 00 00 8b 4b 38 8b 78 48 31 cf 89 f8 8b 7a 4c 81
[ 70.650136][ T3747] EAX: 00000001 EBX: f5f49fac ECX: 0000000b EDX: f610b600
[ 70.650852][ T3747] ESI: f5f79cc0 EDI: f5f79cc0 EBP: f5f49f04 ESP: f5f49ef0
[ 70.651593][ T3747] DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068 EFLAGS: 00010246
[ 70.652413][ T3747] CR0: 80050033 CR2: 00004000 CR3: 35fc7000 CR4: 000406d0
[ 70.653169][ T3747] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
[ 70.653897][ T3747] DR6: fffe0ff0 DR7: 00000400

[ 70.654382][ T3747] Call Trace:
[ 70.654719][ T3747] arch_do_signal_or_restart (arch/x86/kernel/signal.c:792 arch/x86/kernel/signal.c:867)
[ 70.655288][ T3747] exit_to_user_mode_prepare (kernel/entry/common.c:174 kernel/entry/common.c:209)
[ 70.655854][ T3747] irqentry_exit_to_user_mode (kernel/entry/common.c:126 kernel/entry/common.c:317)
[ 70.656450][ T3747] irqentry_exit (kernel/entry/common.c:406)
[ 70.656897][ T3747] exc_page_fault (arch/x86/mm/fault.c:1535)
[ 70.657369][ T3747] ? sysvec_kvm_asyncpf_interrupt (arch/x86/mm/fault.c:1488)
[ 70.657989][ T3747] handle_exception (arch/x86/entry/entry_32.S:1085)

Quoted 3 lines · Linus Torvalds

But I could see some quick test hack doing it - the IVT at boot is actually not at zero, but at fffxxxxx. 8086 is magic.

.. and it's been too long, and I'm too lazy to check - it may be that vm86 mode doesn't even do that magic boot-time address thing.

It's not like we really care about vm86 mode any more, since pretty much nobody users it.

Instead of pretending to send SIGSEGV by calling do_exit(SIGSEGV) call force_sigsegv(SIGSEGV) to force the process to take a SIGSEGV and terminate.

Update handle_signal to return immediately when save_v86_state fails and kills the process. Returning immediately without doing anything except killing the process with SIGSEGV is also what signal_setup_done does when setup_rt_frame fails. Plus it is always ok to return immediately without delivering a signal to a userspace handler when a fatal signal has killed the current process.

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc [email protected]

Cc H Peter Anvin <[email protected]>

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 arch/x86/kernel/signal.c  | 6 +++++-
 arch/x86/kernel/vm86_32.c | 2 +-
 2 files changed, 6 insertions(+), 2 deletions(-)

arch/x86/kernel/signal.c +5 -1

index f4d21e470083..25a230f705c1 100644
@@ -785,8 +785,12 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 	bool stepping, failed;
 	struct fpu *fpu = &current->thread.fpu;
 
-	if (v8086_mode(regs))
+	if (v8086_mode(regs)) {
 		save_v86_state((struct kernel_vm86_regs *) regs, VM86_SIGNAL);
+		/* Has save_v86_state failed and killed the process? */
+		if (fatal_signal_pending(current))
+			return;
+	}
 
 	/* Are we from a system call? */
 	if (syscall_get_nr(current, regs) != -1) {

arch/x86/kernel/vm86_32.c +1 -1

index 63486da77272..040fd01be8b3 100644
@@ -159,7 +159,7 @@ void save_v86_state(struct kernel_vm86_regs *regs, int retval)
 	user_access_end();
 Efault:
 	pr_alert("could not access userspace vm86 info\n");
-	do_exit(SIGSEGV);
+	force_sigsegv(SIGSEGV);
 }
 
 static int do_vm86_irq_handling(int subfunction, int irqnumber);

Signature · 1 line

2.20.1

Quoted 10 lines · Eric W. Biederman

Instead of pretending to send SIGSEGV by calling do_exit(SIGSEGV) call force_sigsegv(SIGSEGV) to force the process to take a SIGSEGV and terminate.

Update handle_signal to return immediately when save_v86_state fails and kills the process. Returning immediately without doing anything except killing the process with SIGSEGV is also what signal_setup_done does when setup_rt_frame fails. Plus it is always ok to return immediately without delivering a signal to a userspace handler when a fatal signal has killed the current process.

Do the tools/testing/selftests/x86 tests all pass after these changes? I know Andy has a bunch of weird corner cases in there.

Quoted 12 lines · Kees Cook

Quoted 10 lines · Eric W. Biederman

Instead of pretending to send SIGSEGV by calling do_exit(SIGSEGV) call force_sigsegv(SIGSEGV) to force the process to take a SIGSEGV and terminate.

Update handle_signal to return immediately when save_v86_state fails and kills the process. Returning immediately without doing anything except killing the process with SIGSEGV is also what signal_setup_done does when setup_rt_frame fails. Plus it is always ok to return immediately without delivering a signal to a userspace handler when a fatal signal has killed the current process.

Quoted 2 lines

Do the tools/testing/selftests/x86 tests all pass after these changes? I know Andy has a bunch of weird corner cases in there.

That would require a 32bit userspace wouldn't it?

It is a good idea so I will see if I can dig such a box up, but I unfortunately don't have an up-to-date 32bit box handy, or even an up-to-date box with a 32bit userspace.

It has been about 20 years since I have done much with 32bit x86.

How hard is it to run the tests under tools/testing/selftests/... Last time I tried it was a royal pain. I am hoping it is better this round.

Instead of pretending to send SIGSEGV by calling do_exit(SIGSEGV) call force_sigsegv(SIGSEGV) to force the process to take a SIGSEGV and terminate.

Update handle_signal to return immediately when save_v86_state fails and kills the process. Returning immediately without doing anything except killing the process with SIGSEGV is also what signal_setup_done does when setup_rt_frame fails. Plus it is always ok to return immediately without delivering a signal to a userspace handler when a fatal signal has killed the current process.

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc [email protected]

Cc H Peter Anvin <[email protected]>

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 arch/x86/kernel/signal.c  | 6 +++++-
 arch/x86/kernel/vm86_32.c | 2 +-
 2 files changed, 6 insertions(+), 2 deletions(-)

arch/x86/kernel/signal.c +0 -0

index f4d21e470083..25a230f705c1 100644

arch/x86/kernel/signal.c +1 -0

+	}
 
 	/* Are we from a system call? */
 	if (syscall_get_nr(current, regs) != -1) {

arch/x86/kernel/vm86_32.c +0 -0

index 63486da77272..040fd01be8b3 100644

Quoted 3 lines · Eric W. Biederman

Instead of pretending to send SIGSEGV by calling do_exit(SIGSEGV) call force_sigsegv(SIGSEGV) to force the process to take a SIGSEGV and terminate.

Why? I realize it's more polite, but is this useful enough to justify the need for testing and potential security impacts?

Quoted 7 lines

Update handle_signal to return immediately when save_v86_state fails and kills the process. Returning immediately without doing anything except killing the process with SIGSEGV is also what signal_setup_done does when setup_rt_frame fails. Plus it is always ok to return immediately without delivering a signal to a userspace handler when a fatal signal has killed the current process.

I can mostly understand the individual sentences, but I don't understand what you're getting it. If a fatal signal has killed the current process and we are guaranteed not to hit the exit-to-usermode path, then, sure, it's safe to return unless we're worried that the core dump code will explode.

But, unless it's fixed elsewhere in your series, force_sigsegv() is itself quite racy, or at least looks racy -- it can race against another thread calling sigaction() and changing the action to something other than SIG_DFL. So it does not appear to actually reliably kill the caller, especially if exposed to a malicious user program.

Quoted 19 lines

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc [email protected]

Cc H Peter Anvin <[email protected]>

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 arch/x86/kernel/signal.c  | 6 +++++-
 arch/x86/kernel/vm86_32.c | 2 +-
 2 files changed, 6 insertions(+), 2 deletions(-)

arch/x86/kernel/signal.c +0 -0

index f4d21e470083..25a230f705c1 100644
@@ -785,8 +785,12 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 	bool stepping, failed;
 	struct fpu *fpu = &current->thread.fpu;
 

arch/x86/kernel/signal.c handle_signal(struct ksignal *ksig, struct pt_regs *regs) · line 792

Quoted 6 lines

arch/x86/kernel/signal.c +4 -1

-	if (v8086_mode(regs))
+	if (v8086_mode(regs)) {
 		save_v86_state((struct kernel_vm86_regs *) regs, VM86_SIGNAL);
+		/* Has save_v86_state failed and killed the process? */
+		if (fatal_signal_pending(current))
+			return;

This might be an ABI break, or at least it could be if anyone cared about vm86. Imagine this wasn't guarded by if (v8086_mode) and was just if (fatal_signal_pending(current)) return; Then all the other processing gets skipped if a fatal signal is pending (e.g. from a concurrent kill), which could cause visible oddities in a core dump, I think. Maybe it's minor.

Quoted 13 lines

arch/x86/kernel/signal.c +1 -0

+	}
 
 	/* Are we from a system call? */
 	if (syscall_get_nr(current, regs) != -1) {

arch/x86/kernel/vm86_32.c +0 -0

index 63486da77272..040fd01be8b3 100644
@@ -159,7 +159,7 @@ void save_v86_state(struct kernel_vm86_regs *regs, 
 int retval)
 	user_access_end();
 Efault:
 	pr_alert("could not access userspace vm86 info\n");

arch/x86/kernel/vm86_32.c void save_v86_state(struct kernel_vm86_regs *regs, int retval) · line 162

Quoted 2 lines

arch/x86/kernel/vm86_32.c +1 -1

-	do_exit(SIGSEGV);
+	force_sigsegv(SIGSEGV);

This causes us to run unwitting kernel code with the vm86 garbage still loaded into the relevant architectural areas (see the chunk if save_v86_state that's inside preempt_disable()). So NAK, especially since the aforementioned race might cause the exit-to-usermode path to actually run with who-knows-what consequences.

If you really want to make this change, please arrange for save_v86_state() to switch out of vm86 mode *before* anything that might fail so that it's guaranteed to at least put the task in a sane state. And write an explicit test case that tests it. I could help with the latter if you do the former.

Update save_v86_state to always complete all of it's work except possibly some of the copies to userspace even if save_v86_state takes a fault. This ensures that the kernel is always in a sane state, even if userspace has done something silly.

Quoted 5 lines · Eric W. Biederman

Update save_v86_state to always complete all of it's work except possibly some of the copies to userspace even if save_v86_state takes a fault. This ensures that the kernel is always in a sane state, even if userspace has done something silly.

Well, honestly, with this change, you might as well replace the force_sigsegv() with just a plain "force_sig()", and make it something the process can catch.

The only thing that "force_sigsgv()" does is to make SIGSEGV uncatchable. In contrast, a plain "force_sig()" just means that it can't be ignored - but it can be caught, and it is fatal only when not caught.

And with the "always complete the non-vm86 state restore" part change, there's really no reason for it to not be caught.

Of course, the other case (where we have no state information for the "enter vm86 mode" case) is still fatal, and is a "this should never happen". But the "cannot write to the vm86 save state" thing isn't technically fatal.

It should even be possible to write a test for it: passing a read-only pointer to the vm86() system call. The vm86 entry will work (because it only reads the vm86 state from it), but then at vm86 exit, writing the state back will fail.

Anybody?

Update save_v86_state to always complete all of it's work except possibly some of the copies to userspace even if save_v86_state takes a fault. This ensures that the kernel is always in a sane state, even if userspace has done something silly.

When save_v86_state takes a fault update it to force userspace to take a SIGSEGV and terminate the userspace application.

As Andy pointed out in review of the first version of this change there are races between sigaction and the application terinating. Now that the code has been modified to always perform all save_v86_state's work (except possibly copying to userspace) those races do not matter from a kernel perspective.

Forcing the userspace application to terminate (by resetting it's handler to SIGDFL) is there to keep everything as close to the current behavior as possible while removing the unique (and difficult to maintain) use of do_exit.

If this new SIGSEGV happens during handle_signal the next time around the exit_to_user_mode_loop, SIGSEGV will be delivered to userspace.

All of the callers of handle_vm86_trap and handle_vm86_fault run the exit_to_user_mode_loop before they return to userspace any signal sent to the current task during their execution will be delivered to the current task before that tasks exits to usermode.

Cc: Andy Lutomirski <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: [email protected]
Cc: H Peter Anvin <[email protected]>
v1: https://lkml.kernel.org/r/[email protected]

Signed-off-by Eric W. Biederman <[email protected]>

---
  arch/x86/kernel/vm86_32.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

Any does this look better?

I think by just completing all of the work that isn't copying to userspace this makes save_v86_state much more robust.

arch/x86/kernel/vm86_32.c +1 -0

index 63486da77272..933cafab7832 100644
@@ -140,6 +140,7 @@ void save_v86_state(struct kernel_vm86_regs *regs, int retval)
 
  	user_access_end();
 
+exit_vm86:
  	preempt_disable();
  	tsk->thread.sp0 = vm86->saved_sp0;
  	tsk->thread.sysenter_cs = __KERNEL_CS;

I think the result would be nicer if, instead of adding an extra goto, you just literally moved all the cleanup under the unsafe_put_user()s above them. Unless I missed something, none of the put_user stuff reads any state that is written by the cleanup code.

Quoted 5 lines · Andy Lutomirski

I think the result would be nicer if, instead of adding an extra goto, you just literally moved all the cleanup under the unsafe_put_user()s above them. Unless I missed something, none of the put_user stuff reads any state that is written by the cleanup code.

Sure it does:

        memcpy(&regs->pt, &vm86->regs32, sizeof(struct pt_regs));

is very much part of the cleanup code, and overwrites that regs->pt thing.

Which is exactly what we're writing back to user space in that unsafe_put_user() thing.

That said, thinking more about this, and looking at it again, I take back my statement that we could just make it a catchable SIGSEGV instead.

If we can't write the vm86 state to user space, we will have fundamentally lost it, and while it's not fatal to the kernel, and while we've recovered the original 32-bit state, it's not something that user space can sanely recover from because the register state at the end of the vm86 work has now been irrecoverably thrown away.

So I think Eric's patch is fine.

Except, as mentioned as part of the other patch, the "force_sigsegv()" conversion to use "force_fatal_sig()" was broken, because that function wasn't actually fatal at all.

Linus

Reading the history it is unclear why default_trap_handler calls do_exit. It is not even menthioned in the commit where the change happened. My best guess is that because it is unknown why the exception happened it was desired to guarantee the process never returned to userspace.

Using do_exit(SIGSEGV) has the problem that it will only terminate one thread of a process, leaving the process in an undefined state.

Use force_sigsegv(SIGSEGV) instead which effectively has the same behavior except that is uses the ordinary signal mechanism and terminates all threads of a process and is generally well defined.

Cc Heiko Carstens <[email protected]>

Cc Vasily Gorbik <[email protected]>

Cc Christian Borntraeger <[email protected]>

Cc [email protected]

Fixes ca2ab03237ec ("[PATCH] s390: core changes")

History Tree: https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git

Signed-off-by "Eric W. Biederman" <[email protected]>

Reading the history it is unclear why default_trap_handler calls do_exit. It is not even menthioned in the commit where the change happened. My best guess is that because it is unknown why the exception happened it was desired to guarantee the process never returned to userspace.

Using do_exit(SIGSEGV) has the problem that it will only terminate one thread of a process, leaving the process in an undefined state.

Use force_sigsegv(SIGSEGV) instead which effectively has the same behavior except that is uses the ordinary signal mechanism and terminates all threads of a process and is generally well defined.

Cc Heiko Carstens <[email protected]>

Cc Vasily Gorbik <[email protected]>

Cc Christian Borntraeger <[email protected]>

Cc [email protected]

Fixes ca2ab03237ec ("[PATCH] s390: core changes")

History Tree: https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git

Signed-off-by "Eric W. Biederman" <[email protected]>

---
  arch/s390/kernel/traps.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

arch/s390/kernel/traps.c +1 -1

index bcefc2173de4..51729ea2cf8e 100644
@@ -84,7 +84,7 @@ static void default_trap_handler(struct pt_regs *regs)
  {
  	if (user_mode(regs)) {
  		report_user_fault(regs, SIGSEGV, 0);
-		do_exit(SIGSEGV);
+		force_sigsegv(SIGSEGV);
  	} else
  		die(regs, "Unknown program exception");
  }

Am 20.10.21 um 19:43 schrieb Eric W. Biederman:

Quoted 6 lines

Reading the history it is unclear why default_trap_handler calls do_exit. It is not even menthioned in the commit where the change happened. My best guess is that because it is unknown why the exception happened it was desired to guarantee the process never returned to userspace.

Quoted 6 lines

Using do_exit(SIGSEGV) has the problem that it will only terminate one thread of a process, leaving the process in an undefined state.

Use force_sigsegv(SIGSEGV) instead which effectively has the same behavior except that is uses the ordinary signal mechanism and terminates all threads of a process and is generally well defined.

Do I get that right, that programs can not block SIGSEGV from force_sigsegv with a signal handler? Thats how I read the code. If this is true then

Reviewed-by Christian Borntraeger <[email protected]>

Quoted 25 lines

Cc Heiko Carstens <[email protected]>

Cc Vasily Gorbik <[email protected]>

Cc Christian Borntraeger <[email protected]>

Cc [email protected]

Fixes ca2ab03237ec ("[PATCH] s390: core changes")

History Tree: https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git

Signed-off-by "Eric W. Biederman" <[email protected]>

---
  arch/s390/kernel/traps.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

arch/s390/kernel/traps.c +1 -1

index bcefc2173de4..51729ea2cf8e 100644
@@ -84,7 +84,7 @@ static void default_trap_handler(struct pt_regs *regs)
  {
  	if (user_mode(regs)) {
  		report_user_fault(regs, SIGSEGV, 0);
-		do_exit(SIGSEGV);
+		force_sigsegv(SIGSEGV);
  	} else
  		die(regs, "Unknown program exception");
  }

Quoted 18 lines · Christian Borntraeger

Am 20.10.21 um 19:43 schrieb Eric W. Biederman:

Quoted 6 lines

Reading the history it is unclear why default_trap_handler calls do_exit. It is not even menthioned in the commit where the change happened. My best guess is that because it is unknown why the exception happened it was desired to guarantee the process never returned to userspace.

Quoted 6 lines

Using do_exit(SIGSEGV) has the problem that it will only terminate one thread of a process, leaving the process in an undefined state.

Use force_sigsegv(SIGSEGV) instead which effectively has the same behavior except that is uses the ordinary signal mechanism and terminates all threads of a process and is generally well defined.

Do I get that right, that programs can not block SIGSEGV from force_sigsegv with a signal handler? Thats how I read the code. If this is true then

Quoted 1 line

Reviewed-by Christian Borntraeger <[email protected]>

99% true, and it is what force_sigsegv(SIGSEGV) intends to do.

Andy Lutormorski pointed at a race where a thread can call sigaction and change the signal handler after force_sigsegv has run but before the process dequeues the SIGSEGV.

Quoted 25 lines

Quoted 25 lines

Cc Heiko Carstens <[email protected]>

Cc Vasily Gorbik <[email protected]>

Cc Christian Borntraeger <[email protected]>

Cc [email protected]

Fixes ca2ab03237ec ("[PATCH] s390: core changes")

History Tree: https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git

Signed-off-by "Eric W. Biederman" <[email protected]>

---
  arch/s390/kernel/traps.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

arch/s390/kernel/traps.c +1 -1

index bcefc2173de4..51729ea2cf8e 100644
@@ -84,7 +84,7 @@ static void default_trap_handler(struct pt_regs *regs)
  {
  	if (user_mode(regs)) {
  		report_user_fault(regs, SIGSEGV, 0);
-		do_exit(SIGSEGV);
+		force_sigsegv(SIGSEGV);
  	} else
  		die(regs, "Unknown program exception");
  }

In 2009 Oleg reworked[1] the kernel threads so that it is not necessary to call do_exit if you are not using kthread_stop(). Remove the explicit calls of do_exit and complete_and_exit (with a NULL completion) that were previously necessary.

Quoted 4 lines · Eric W. Biederman

In 2009 Oleg reworked[1] the kernel threads so that it is not necessary to call do_exit if you are not using kthread_stop(). Remove the explicit calls of do_exit and complete_and_exit (with a NULL completion) that were previously necessary.

With this we should also be able to drop the export for do_exit.

In 2009 Oleg reworked[1] the kernel threads so that it is not necessary to call do_exit if you are not using kthread_stop(). Remove the explicit calls of do_exit and complete_and_exit (with a NULL completion) that were previously necessary.

[1] 63706172f332 ("kthreads: rework kthread_stop()")

Signed-off-by "Eric W. Biederman" <[email protected]>

Add a simple helper force_fatal_sig that causes a signal to be delivered to a process as if the signal handler was set to SIG_DFL.

Reimplement force_sigsegv based upon this new helper.

Quoted 5 lines · Eric W. Biederman

Add a simple helper force_fatal_sig that causes a signal to be delivered to a process as if the signal handler was set to SIG_DFL.

Reimplement force_sigsegv based upon this new helper.

Can you just make the old force_sigsegv() go away? The odd special casing of SIGSEGV was odd to begin with, I think everybody really just wanted this new "force_fatal_sig()" and allow any signal - not making SIGSEGV special.

Also, I think it should set SIGKILL in p->pending.signal or something like that - because we want this to trigger fatal_signal_pending(), don't we?

Right now fatal_signal_pending() is only true for SIGKILL, I think.

Quoted 4 lines · Eric W. Biederman

Add a simple helper force_fatal_sig that causes a signal to be delivered to a process as if the signal handler was set to SIG_DFL.

Quoted 1 line

Reimplement force_sigsegv based upon this new helper.

Can you just make the old force_sigsegv() go away? The odd special casing of SIGSEGV was odd to begin with, I think everybody really just wanted this new "force_fatal_sig()" and allow any signal - not making SIGSEGV special.

I'm rather nervous about all this, and I'm also nervous about the existing code. A quick skim is finding plenty of code paths that assume force_sigsegv (or a do_exit that this series touches) are genuinely unrecoverable.

Quoted 5 lines · Andy Lutomirski

I'm rather nervous about all this, and I'm also nervous about the existing code. A quick skim is finding plenty of code paths that assume force_sigsegv (or a do_exit that this series touches) are genuinely unrecoverable.

I was going to say "what are you talking about", because clearly Eric kept it all fatal.

But then looked at that patch a bit more before I claimed you were wrong.

And yeah, Eric's force_fatal_sig() is completely broken.

It claims to force a fatal signal, but doesn't actually do that at all, and is completely misnamed.

It just uses "force_sig_info_to_task()", which still allows user space to catch signals - so it's not "fatal" in the least. It only punches through SIG_IGN and blocked signals.

So yeah, that's broken.

I do still think that that could the behavior we possibly want for that "can't write updated vm86 state back" situation, but for something that is called "fatal", it really needs to be fatal.

Quoted 5 lines · Andy Lutomirski

I'm rather nervous about all this, and I'm also nervous about the existing code. A quick skim is finding plenty of code paths that assume force_sigsegv (or a do_exit that this series touches) are genuinely unrecoverable.

I was going to say "what are you talking about", because clearly Eric kept it all fatal.

But then looked at that patch a bit more before I claimed you were wrong.

And yeah, Eric's force_fatal_sig() is completely broken.

It claims to force a fatal signal, but doesn't actually do that at all, and is completely misnamed.

It just uses "force_sig_info_to_task()", which still allows user space to catch signals - so it's not "fatal" in the least. It only punches through SIG_IGN and blocked signals.

Rereading this I think you might be misreading something.

force_siginfo_to_task takes a sigdfl parameter which I am setting in force_fatal_signal.

Quoted 2 lines · Eric W. Biederman

Rereading this I think you might be misreading something.

Gaah. Yes, indeed.

Quoted 2 lines

force_siginfo_to_task takes a sigdfl parameter which I am setting in force_fatal_signal.

.. and I realized that the first time I read through it, but then when I read through it due to Andy saying it worries him, I missed it and thought the handler didn't get reset.

So the patch is fine.

Add a simple helper force_fatal_sig that causes a signal to be delivered to a process as if the signal handler was set to SIG_DFL.

Reimplement force_sigsegv based upon this new helper. This fixes force_sigsegv so that when it forces the default signal handler to be used the code now forces the signal to be unblocked as well.

Reusing the tested logic in force_sig_info_to_task that was built for force_sig_seccomp this makes the implementation trivial.

This is interesting both because it makes force_sigsegv simpler and because there are a couple of buggy places in the kernel that call do_exit(SIGILL) or do_exit(SIGSYS) because there is no straight forward way today for those places to simply force the exit of a process with the chosen signal. Creating force_fatal_sig allows those places to be implemented with normal signal exits.

Signed-off-by "Eric W. Biederman" <[email protected]>

Quoted 6 lines · Eric W. Biederman

This is interesting both because it makes force_sigsegv simpler and because there are a couple of buggy places in the kernel that call do_exit(SIGILL) or do_exit(SIGSYS) because there is no straight forward way today for those places to simply force the exit of a process with the chosen signal. Creating force_fatal_sig allows those places to be implemented with normal signal exits.

I assume this is talking about seccomp()? :) Should a patch be included in this series to change those?

Quoted 8 lines · Kees Cook

Quoted 6 lines · Eric W. Biederman

This is interesting both because it makes force_sigsegv simpler and because there are a couple of buggy places in the kernel that call do_exit(SIGILL) or do_exit(SIGSYS) because there is no straight forward way today for those places to simply force the exit of a process with the chosen signal. Creating force_fatal_sig allows those places to be implemented with normal signal exits.

Quoted 2 lines

I assume this is talking about seccomp()? :) Should a patch be included in this series to change those?

Actually it is not talking about seccomp. As far as I can tell seccomp is deliberately only killing a single thread when it calls do_exit.

I am thinking about places where we really want the entire process to die and not just a single thread. Please see the following changes where I actually use force_fatal_sig.

[email protected] (Eric W. Biederman) asked:

Quoted 6 lines

Why does do_syscal_user_dispatch call do_exit(SIGSEGV) and do_exit(SIGSYS) instead of force_sig(SIGSEGV) and force_sig(SIGSYS)?

Looking at the code these cases are not expected to happen, so I would be surprised if userspace depends on any particular behaviour on the failure path so I think we can change this.

Hi Eric,

There is not really a good reason, and the use case that originated the feature doesn't rely on it.

Unless I'm missing yet another problem and others correct me, I think it makes sense to change it as you described.

Quoted 1 line

Is using do_exit in this way something you copied from seccomp?

I'm not sure, its been a while, but I think it might be just that. The first prototype of SUD was implemented as a seccomp mode.

Use force_fatal_sig instead of calling do_exit directly. This ensures the ordinary signal handling path gets invoked, core dumps as appropriate get created, and for multi-threaded processes all of the threads are terminated not just a single thread.

When asked Gabriel Krisman Bertazi <[email protected]> said [1]:

Quoted 14 lines

[email protected] (Eric W. Biederman) asked:

Quoted 6 lines

Why does do_syscal_user_dispatch call do_exit(SIGSEGV) and do_exit(SIGSYS) instead of force_sig(SIGSEGV) and force_sig(SIGSYS)?

Looking at the code these cases are not expected to happen, so I would be surprised if userspace depends on any particular behaviour on the failure path so I think we can change this.

Hi Eric,

There is not really a good reason, and the use case that originated the feature doesn't rely on it.

Quoted 7 lines

Unless I'm missing yet another problem and others correct me, I think it makes sense to change it as you described.

Quoted 1 line

Is using do_exit in this way something you copied from seccomp?

I'm not sure, its been a while, but I think it might be just that. The first prototype of SUD was implemented as a seccomp mode.

If at some point it becomes interesting we could relax "force_fatal_sig(SIGSEGV)" to instead say "force_sig_fault(SIGSEGV, SEGV_MAPERR, sd->selector)".

I avoid doing that in this patch to avoid making it possible to catch currently uncatchable signals.

Cc: Gabriel Krisman Bertazi <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Andy Lutomirski <[email protected]>
[1] https://lkml.kernel.org/r/[email protected]

Signed-off-by "Eric W. Biederman" <[email protected]>

Quoted 4 lines · Eric W. Biederman

Use force_fatal_sig instead of calling do_exit directly. This ensures the ordinary signal handling path gets invoked, core dumps as appropriate get created, and for multi-threaded processes all of the threads are terminated not just a single thread.

Yeah, looks good. Should be no visible behavior change.

Quoted 6 lines · Kees Cook

Quoted 4 lines · Eric W. Biederman

Use force_fatal_sig instead of calling do_exit directly. This ensures the ordinary signal handling path gets invoked, core dumps as appropriate get created, and for multi-threaded processes all of the threads are terminated not just a single thread.

Quoted 1 line

Yeah, looks good. Should be no visible behavior change.

It is observable in that an entire multi-threaded process gets terminated instead of a single thread. But since these events should be handling of extra-ordinary events I don't expect there is anyone who wants to have a thread of their process survive.

Quoted 10 lines · Eric W. Biederman

Quoted 7 lines · Kees Cook

Quoted 4 lines · Eric W. Biederman

Use force_fatal_sig instead of calling do_exit directly. This ensures the ordinary signal handling path gets invoked, core dumps as appropriate get created, and for multi-threaded processes all of the threads are terminated not just a single thread.

Yeah, looks good. Should be no visible behavior change.

Quoted 4 lines

It is observable in that an entire multi-threaded process gets terminated instead of a single thread. But since these events should be handling of extra-ordinary events I don't expect there is anyone who wants to have a thread of their process survive.

Right -- sorry, I should have said that more clearly: "Besides the single thread death now taking the whole process, there's not behavior change (i.e. the signal delivery)." Still looks good to me.

Quoted 35 lines · Eric W. Biederman

Use force_fatal_sig instead of calling do_exit directly. This ensures the ordinary signal handling path gets invoked, core dumps as appropriate get created, and for multi-threaded processes all of the threads are terminated not just a single thread.

When asked Gabriel Krisman Bertazi <[email protected]> said [1]:

Quoted 14 lines

[email protected] (Eric W. Biederman) asked:

Quoted 6 lines

Why does do_syscal_user_dispatch call do_exit(SIGSEGV) and do_exit(SIGSYS) instead of force_sig(SIGSEGV) and force_sig(SIGSYS)?

Looking at the code these cases are not expected to happen, so I would be surprised if userspace depends on any particular behaviour on the failure path so I think we can change this.

Hi Eric,

There is not really a good reason, and the use case that originated the feature doesn't rely on it.

Quoted 7 lines

Unless I'm missing yet another problem and others correct me, I think it makes sense to change it as you described.

Quoted 1 line

Is using do_exit in this way something you copied from seccomp?

I'm not sure, its been a while, but I think it might be just that. The first prototype of SUD was implemented as a seccomp mode.

If at some point it becomes interesting we could relax "force_fatal_sig(SIGSEGV)" to instead say "force_sig_fault(SIGSEGV, SEGV_MAPERR, sd->selector)".

I avoid doing that in this patch to avoid making it possible to catch currently uncatchable signals.

Quoted 6 lines

Cc: Gabriel Krisman Bertazi <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Andy Lutomirski <[email protected]>
[1] https://lkml.kernel.org/r/[email protected]

Signed-off-by "Eric W. Biederman" <[email protected]>

Yeah, looks good. Should be no visible behavior change.

Reviewed-by Kees Cook <[email protected]>

Use force_fatal_sig instead of calling do_exit directly. This ensures the ordinary signal handling path gets invoked, core dumps as appropriate get created, and for multi-threaded processes all of the threads are terminated not just a single thread.

When asked Gabriel Krisman Bertazi <[email protected]> said [1]:

Quoted 14 lines

[email protected] (Eric W. Biederman) asked:

Quoted 6 lines

Why does do_syscal_user_dispatch call do_exit(SIGSEGV) and do_exit(SIGSYS) instead of force_sig(SIGSEGV) and force_sig(SIGSYS)?

Looking at the code these cases are not expected to happen, so I would be surprised if userspace depends on any particular behaviour on the failure path so I think we can change this.

Hi Eric,

There is not really a good reason, and the use case that originated the feature doesn't rely on it.

Quoted 7 lines

Unless I'm missing yet another problem and others correct me, I think it makes sense to change it as you described.

Quoted 1 line

Is using do_exit in this way something you copied from seccomp?

I'm not sure, its been a while, but I think it might be just that. The first prototype of SUD was implemented as a seccomp mode.

If at some point it becomes interesting we could relax "force_fatal_sig(SIGSEGV)" to instead say "force_sig_fault(SIGSEGV, SEGV_MAPERR, sd->selector)".

I avoid doing that in this patch to avoid making it possible to catch currently uncatchable signals.

Cc: Gabriel Krisman Bertazi <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Andy Lutomirski <[email protected]>
[1] https://lkml.kernel.org/r/[email protected]

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 kernel/entry/syscall_user_dispatch.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

The function try_to_clear_window_buffer is only called from rtrap_32.c. After it is called the signal pending state is retested,

and signals are handled if TIF_SIGPENDING is set. This allows try_to_clear_window_buffer to call force_fatal_signal and then rely on the signal being delivered to kill the process, without any danger of returning to userspace, or otherwise using possible corrupt state on failure.

Quoted 2 lines · Eric W. Biederman

The function try_to_clear_window_buffer is only called from rtrap_32.c. After it is called the signal pending state is retested,

nit: rtrap_32.S

Quoted 5 lines

and signals are handled if TIF_SIGPENDING is set. This allows try_to_clear_window_buffer to call force_fatal_signal and then rely on the signal being delivered to kill the process, without any danger of returning to userspace, or otherwise using possible corrupt state on failure.

The TIF_SIGPENDING test happens in do_notify_resume(), though I see other code before that:

...
call try_to_clear_window_buffer
add %sp, STACKFRAME_SZ, %o0

b signal_p
...
signal_p:
andcc %g2, _TIF_DO_NOTIFY_RESUME_MASK, %g0
bz,a ret_trap_continue
ld [%sp + STACKFRAME_SZ + PT_PSR], %t_psr

mov %g2, %o2
mov %l6, %o1
call do_notify_resume

Will the ret_trap_continue always be skipped?

Also I see the "tp->w_saved = 0" never happens due to the "return" in try_to_clear_window_buffer. Is that okay?

Only synchronize_user_stack()
uses it, and that could be called in do_sigreturn(). Should the "return" be removed?

Modify the 32bit version of setup_rt_frame and setup_frame to act similar to the 64bit version of setup_rt_frame and fail with a signal instead of calling do_exit.

Replacing do_exit(SIGILL) with force_fatal_signal(SIGILL) ensures that the process will be terminated cleanly when the stack frame is invalid, instead of just killing off a single thread and leaving the process is a weird state.

Cc David Miller <[email protected]>

Cc [email protected]

Signed-off-by Eric W. Biederman <[email protected]>

Directly calling do_exit with a signal number has the problem that all of the side effects of the signal don't happen, such as killing all of the threads of a process instead of just the calling thread.

So replace do_exit(SIGSYS) with force_fatal_sig(SIGSYS) which causes the signal handling to take it's normal path and work as expected.

Cc Andy Lutomirski <[email protected]>

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 arch/x86/entry/vsyscall/vsyscall_64.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

arch/x86/entry/vsyscall/vsyscall_64.c +0 -0

index 1b40b9297083..0b6b277ee050 100644

Every place thread_exit is called is at the end of a function started with kthread_run. The code in kthread_run has arranged things so a kernel thread can just return and do_exit will be called.

So just have the threads return instead of calling complete_and_exit.

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 drivers/staging/rtl8723bs/core/rtw_cmd.c                | 2 +-
 drivers/staging/rtl8723bs/core/rtw_xmit.c               | 2 +-
 drivers/staging/rtl8723bs/hal/rtl8723bs_xmit.c          | 2 +-
 drivers/staging/rtl8723bs/include/osdep_service_linux.h | 2 --
 4 files changed, 3 insertions(+), 5 deletions(-)

Quoted 6 lines · Eric W. Biederman

Every place thread_exit is called is at the end of a function started with kthread_run. The code in kthread_run has arranged things so a kernel thread can just return and do_exit will be called.

So just have the threads return instead of calling complete_and_exit.

Quoted 7 lines

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 drivers/staging/rtl8723bs/core/rtw_cmd.c                | 2 +-
 drivers/staging/rtl8723bs/core/rtw_xmit.c               | 2 +-
 drivers/staging/rtl8723bs/hal/rtl8723bs_xmit.c          | 2 +-
 drivers/staging/rtl8723bs/include/osdep_service_linux.h | 2 --
 4 files changed, 3 insertions(+), 5 deletions(-)

You "forgot" to cc: the linux-staging and the staging driver maintainer on these drivers/staging/ changes...

Anyway, they look fine to me, but you will get some conflicts with some of these changes based on cleanups already in my staging-next tree (in linux-next if you want to see them). But feel free to take these all in your tree if that makes it easier:

Acked-by Greg Kroah-Hartman <[email protected]>

Every place thread_exit is called is at the end of a function started with kthread_run. The code in kthread_run has arranged things so a kernel thread can just return and do_exit will be called.

So just have the threads return instead of calling complete_and_exit.

Signed-off-by "Eric W. Biederman" <[email protected]>

The macro thread_exit is called is at the end of a function started with kthread_run. The code in kthread_run has arranged things so a kernel thread can just return and do_exit will be called.

So just have the cmd_thread return instead of calling complete_and_exit.

Signed-off-by "Eric W. Biederman" <[email protected]>

The macro thread_exit is called is at the end of a function started with kthread_run. The code in kthread_run has arranged things so a kernel thread can just return and do_exit will be called.

So just have the cmd_thread return instead of calling complete_and_exit.

Signed-off-by "Eric W. Biederman" <[email protected]>

The macro thread_exit is called is at the end of functions started with kthread_run. The code in kthread_run has arranged things so a kernel thread can just return and do_exit will be called.

So just have rtw_cmd_thread and mp_xmit_packet_thread return instead of calling complete_and_exit.

Signed-off-by "Eric W. Biederman" <[email protected]>

The macro thread_exit is called is at the end of functions started with kthread_run. The code in kthread_run has arranged things so a kernel thread can just return and do_exit will be called.

So just have rtw_cmd_thread and mp_xmit_packet_thread return instead of calling complete_and_exit.

Signed-off-by "Eric W. Biederman" <[email protected]>

Now that force_fatal_sig exists it is unnecessary and a bit confusing to use force_sigsegv in cases where the simpler force_fatal_sig is wanted. So change every instance we can to make the code clearer.

Signed-off-by "Eric W. Biederman" <[email protected]>

 arch/m68k/kernel/traps.c        | 2 +-

Hi Eric,

Patch 21/20?

Quoted 7 lines · Eric W. Biederman

Now that force_fatal_sig exists it is unnecessary and a bit confusing to use force_sigsegv in cases where the simpler force_fatal_sig is wanted. So change every instance we can to make the code clearer.

Signed-off-by "Eric W. Biederman" <[email protected]>

 arch/m68k/kernel/traps.c        | 2 +-

Acked-by Geert Uytterhoeven <[email protected]>

Now that force_fatal_sig exists it is unnecessary and a bit confusing to use force_sigsegv in cases where the simpler force_fatal_sig is wanted. So change every instance we can to make the code clearer.

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 arch/arc/kernel/process.c       | 2 +-
 arch/m68k/kernel/traps.c        | 2 +-
 arch/powerpc/kernel/signal_32.c | 2 +-
 arch/powerpc/kernel/signal_64.c | 4 ++--
 arch/s390/kernel/traps.c        | 2 +-
 arch/um/kernel/trap.c           | 2 +-
 arch/x86/kernel/vm86_32.c       | 2 +-
 fs/exec.c                       | 2 +-
 8 files changed, 9 insertions(+), 9 deletions(-)