On Thu, Nov 18, 2021 at 04:05:31PM -0600, Eric W. Biederman wrote: > > Recently to prevent issues with SECCOMP_RET_KILL and similar signals > being changed before they are delivered SA_IMMUTABLE was added. > > Unfortunately this broke debuggers[1][2] which reasonably expect > to be able to trap synchronous SIGTRAP and SIGSEGV even when > the target process is not configured to handle those signals. > > Add force_exit_sig and use it instead of force_fatal_sig where > historically the code has directly called do_exit. This has the > implementation benefits of going through the signal exit path > (including generating core dumps) without the danger of allowing > userspace to ignore or change these signals. > > This avoids userspace regressions as older kernels exited with do_exit > which debuggers also can not intercept. > > In the future is should be possible to improve the quality of > implementation of the kernel by changing some of these force_exit_sig > calls to force_fatal_sig. That can be done where it matters on > a case-by-case basis with careful analysis. > > Reported-by: Kyle Huey <me@xxxxxxxxxxxx> > Reported-by: kernel test robot <oliver.sang@xxxxxxxxx> > [1] https://lkml.kernel.org/r/CAP045AoMY4xf8aC_4QU_-j7obuEPYgTcnQQP3Yxk=2X90jtpjw@xxxxxxxxxxxxxx > [2] https://lkml.kernel.org/r/20211117150258.GB5403@xsang-OptiPlex-902 > Fixes: 00b06da29cf9 ("signal: Add SA_IMMUTABLE to ensure forced siganls do not get changed") > Fixes: a3616a3c0272 ("signal/m68k: Use force_sigsegv(SIGSEGV) in fpsp040_die") > Fixes: 83a1f27ad773 ("signal/powerpc: On swapcontext failure force SIGSEGV") > Fixes: 9bc508cf0791 ("signal/s390: Use force_sigsegv in default_trap_handler") > Fixes: 086ec444f866 ("signal/sparc32: In setup_rt_frame and setup_fram use force_fatal_sig") > Fixes: c317d306d550 ("signal/sparc32: Exit with a fatal signal when try_to_clear_window_buffer fails") > Fixes: 695dd0d634df ("signal/x86: In emulate_vsyscall force a signal instead of calling do_exit") > Fixes: 1fbd60df8a85 ("signal/vm86_32: Properly send SIGSEGV when the vm86 state cannot be saved.") > Fixes: 941edc5bf174 ("exit/syscall_user_dispatch: Send ordinary signals on failure") > Signed-off-by: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx> Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx> Tested-by: Kees Cook <keescook@xxxxxxxxxxxx> Thanks! -Kees > --- > 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/sparc/kernel/signal_32.c | 4 ++-- > arch/sparc/kernel/windows.c | 2 +- > arch/x86/entry/vsyscall/vsyscall_64.c | 2 +- > arch/x86/kernel/vm86_32.c | 2 +- > include/linux/sched/signal.h | 1 + > kernel/entry/syscall_user_dispatch.c | 4 ++-- > kernel/signal.c | 13 +++++++++++++ > 11 files changed, 26 insertions(+), 12 deletions(-) > > diff --git a/arch/m68k/kernel/traps.c b/arch/m68k/kernel/traps.c > index 99058a6da956..34d6458340b0 100644 > --- a/arch/m68k/kernel/traps.c > +++ b/arch/m68k/kernel/traps.c > @@ -1145,7 +1145,7 @@ asmlinkage void set_esp0(unsigned long ssp) > */ > asmlinkage void fpsp040_die(void) > { > - force_fatal_sig(SIGSEGV); > + force_exit_sig(SIGSEGV); > } > > #ifdef CONFIG_M68KFPU_EMU > diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c > index 00a9c9cd6d42..3e053e2fd6b6 100644 > --- a/arch/powerpc/kernel/signal_32.c > +++ b/arch/powerpc/kernel/signal_32.c > @@ -1063,7 +1063,7 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx, > * We kill the task with a SIGSEGV in this situation. > */ > if (do_setcontext(new_ctx, regs, 0)) { > - force_fatal_sig(SIGSEGV); > + force_exit_sig(SIGSEGV); > return -EFAULT; > } > > diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c > index ef518535d436..d1e1fc0acbea 100644 > --- a/arch/powerpc/kernel/signal_64.c > +++ b/arch/powerpc/kernel/signal_64.c > @@ -704,7 +704,7 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx, > */ > > if (__get_user_sigset(&set, &new_ctx->uc_sigmask)) { > - force_fatal_sig(SIGSEGV); > + force_exit_sig(SIGSEGV); > return -EFAULT; > } > set_current_blocked(&set); > @@ -713,7 +713,7 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx, > return -EFAULT; > if (__unsafe_restore_sigcontext(current, NULL, 0, &new_ctx->uc_mcontext)) { > user_read_access_end(); > - force_fatal_sig(SIGSEGV); > + force_exit_sig(SIGSEGV); > return -EFAULT; > } > user_read_access_end(); > diff --git a/arch/s390/kernel/traps.c b/arch/s390/kernel/traps.c > index 035705c9f23e..2b780786fc68 100644 > --- a/arch/s390/kernel/traps.c > +++ b/arch/s390/kernel/traps.c > @@ -84,7 +84,7 @@ static void default_trap_handler(struct pt_regs *regs) > { > if (user_mode(regs)) { > report_user_fault(regs, SIGSEGV, 0); > - force_fatal_sig(SIGSEGV); > + force_exit_sig(SIGSEGV); > } else > die(regs, "Unknown program exception"); > } > diff --git a/arch/sparc/kernel/signal_32.c b/arch/sparc/kernel/signal_32.c > index cd677bc564a7..ffab16369bea 100644 > --- a/arch/sparc/kernel/signal_32.c > +++ b/arch/sparc/kernel/signal_32.c > @@ -244,7 +244,7 @@ static int setup_frame(struct ksignal *ksig, struct pt_regs *regs, > get_sigframe(ksig, regs, sigframe_size); > > if (invalid_frame_pointer(sf, sigframe_size)) { > - force_fatal_sig(SIGILL); > + force_exit_sig(SIGILL); > return -EINVAL; > } > > @@ -336,7 +336,7 @@ static int setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs, > sf = (struct rt_signal_frame __user *) > get_sigframe(ksig, regs, sigframe_size); > if (invalid_frame_pointer(sf, sigframe_size)) { > - force_fatal_sig(SIGILL); > + force_exit_sig(SIGILL); > return -EINVAL; > } > > diff --git a/arch/sparc/kernel/windows.c b/arch/sparc/kernel/windows.c > index bbbd40cc6b28..8f20862ccc83 100644 > --- a/arch/sparc/kernel/windows.c > +++ b/arch/sparc/kernel/windows.c > @@ -122,7 +122,7 @@ 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))) { > - force_fatal_sig(SIGILL); > + force_exit_sig(SIGILL); > return; > } > } > diff --git a/arch/x86/entry/vsyscall/vsyscall_64.c b/arch/x86/entry/vsyscall/vsyscall_64.c > index 0b6b277ee050..fd2ee9408e91 100644 > --- a/arch/x86/entry/vsyscall/vsyscall_64.c > +++ b/arch/x86/entry/vsyscall/vsyscall_64.c > @@ -226,7 +226,7 @@ bool emulate_vsyscall(unsigned long error_code, > if ((!tmp && regs->orig_ax != syscall_nr) || regs->ip != address) { > warn_bad_vsyscall(KERN_DEBUG, regs, > "seccomp tried to change syscall nr or ip"); > - force_fatal_sig(SIGSYS); > + force_exit_sig(SIGSYS); > return true; > } > regs->orig_ax = -1; > diff --git a/arch/x86/kernel/vm86_32.c b/arch/x86/kernel/vm86_32.c > index cce1c89cb7df..c21bcd668284 100644 > --- a/arch/x86/kernel/vm86_32.c > +++ b/arch/x86/kernel/vm86_32.c > @@ -160,7 +160,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"); > - force_fatal_sig(SIGSEGV); > + force_exit_sig(SIGSEGV); > goto exit_vm86; > } > > diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h > index 23505394ef70..33a50642cf41 100644 > --- a/include/linux/sched/signal.h > +++ b/include/linux/sched/signal.h > @@ -352,6 +352,7 @@ extern __must_check bool do_notify_parent(struct task_struct *, int); > extern void __wake_up_parent(struct task_struct *p, struct task_struct *parent); > extern void force_sig(int); > extern void force_fatal_sig(int); > +extern void force_exit_sig(int); > extern int send_sig(int, struct task_struct *, int); > extern int zap_other_threads(struct task_struct *p); > extern struct sigqueue *sigqueue_alloc(void); > diff --git a/kernel/entry/syscall_user_dispatch.c b/kernel/entry/syscall_user_dispatch.c > index 4508201847d2..0b6379adff6b 100644 > --- a/kernel/entry/syscall_user_dispatch.c > +++ b/kernel/entry/syscall_user_dispatch.c > @@ -48,7 +48,7 @@ bool syscall_user_dispatch(struct pt_regs *regs) > * the selector is loaded by userspace. > */ > if (unlikely(__get_user(state, sd->selector))) { > - force_fatal_sig(SIGSEGV); > + force_exit_sig(SIGSEGV); > return true; > } > > @@ -56,7 +56,7 @@ bool syscall_user_dispatch(struct pt_regs *regs) > return false; > > if (state != SYSCALL_DISPATCH_FILTER_BLOCK) { > - force_fatal_sig(SIGSYS); > + force_exit_sig(SIGSYS); > return true; > } > } > diff --git a/kernel/signal.c b/kernel/signal.c > index 02058c983bd6..fe7ba05145d4 100644 > --- a/kernel/signal.c > +++ b/kernel/signal.c > @@ -1671,6 +1671,19 @@ void force_fatal_sig(int sig) > force_sig_info_to_task(&info, current, HANDLER_SIG_DFL); > } > > +void force_exit_sig(int sig) > +{ > + struct kernel_siginfo info; > + > + clear_siginfo(&info); > + info.si_signo = sig; > + info.si_errno = 0; > + info.si_code = SI_KERNEL; > + info.si_pid = 0; > + info.si_uid = 0; > + force_sig_info_to_task(&info, current, HANDLER_EXIT); > +} > + > /* > * When things go south during signal handling, we > * will force a SIGSEGV. And if the signal that caused > -- > 2.20.1 > -- Kees Cook
