| Message ID | 20211020174406.17889-15-ebiederm@xmission.com |
|---|---|
| State | New |
| Headers | show |
| Series | exit cleanups | expand |
On Wed, Oct 20, 2021 at 12:44:01PM -0500, Eric W. Biederman wrote: > 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 > 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?
Kees Cook <keescook@chromium.org> writes: > On Wed, Oct 20, 2021 at 12:44:01PM -0500, Eric W. Biederman wrote: >> 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 > >> 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? The ret_trap_continue is the break out of the loop. So unless the code is not properly setting the signal to be pending the code should be good. > Also I see the "tp->w_saved = 0" never happens due to the "return" in > try_to_clear_window_buffer. Is that okay? It should be. As you point out the w_saved value is only used in generating signal frames. The code in get_signal should never return and should call do_group_exit which calls do_exit, so building signal frames that happens after get_signal returns should never be reached. Further this is the same way the code makes it to do_exit today. Also looking at it I think the logic is that w_saved == 0 says that the register windows have been saved on the user mode stack, and that clearly has not happened so I think it would in general be a bug to clear w_saved on failure. > Only synchronize_user_stack() > uses it, and that could be called in do_sigreturn(). Should the "return" > be removed? Of course I could be wrong, if David or someone else who knows sparc32 better than me wants to set me straight I would really appreciate it. Eric
diff --git a/arch/sparc/kernel/windows.c b/arch/sparc/kernel/windows.c index 69a6ba6e9293..bbbd40cc6b28 100644 --- a/arch/sparc/kernel/windows.c +++ b/arch/sparc/kernel/windows.c @@ -121,8 +121,10 @@ void try_to_clear_window_buffer(struct pt_regs *regs, int who) if ((sp & 7) || copy_to_user((char __user *) sp, &tp->reg_window[window], - sizeof(struct reg_window32))) - do_exit(SIGILL); + sizeof(struct reg_window32))) { + force_fatal_sig(SIGILL); + return; + } } tp->w_saved = 0; }
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. The functional difference between force_fatal_sig and do_exit is that do_exit will only terminate a single thread, and will never trigger a core-dump. A multi-threaded program for which a single thread terminates unexpectedly is hard to reason about. Calling force_fatal_sig does not give userspace a chance to catch the signal, but otherwise is an ordinary fatal signal exit, and it will trigger a coredump of the offending process if core dumps are enabled. Cc: David Miller <davem@davemloft.net> Cc: sparclinux@vger.kernel.org Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- arch/sparc/kernel/windows.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)