On Thu, Nov 18, 2021 at 04:04:58PM -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. > > Update force_sig_to_task to support both the case when we can > allow the debugger to intercept and possibly ignore the > signal and the case when it is not safe to let userspace > known about the signal until the process has exited. > > Reported-by: Kyle Huey <me@xxxxxxxxxxxx> > Reported-by: kernel test robot <oliver.sang@xxxxxxxxx> > Cc: stable@xxxxxxxxxxxxxxx > [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") > Signed-off-by: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx> Thanks! This passes the seccomp self-tests. Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx> Tested-by: Kees Cook <keescook@xxxxxxxxxxxx> -Kees > --- > kernel/signal.c | 23 ++++++++++++++++------- > 1 file changed, 16 insertions(+), 7 deletions(-) > > diff --git a/kernel/signal.c b/kernel/signal.c > index 7c4b7ae714d4..02058c983bd6 100644 > --- a/kernel/signal.c > +++ b/kernel/signal.c > @@ -1298,6 +1298,12 @@ int do_send_sig_info(int sig, struct kernel_siginfo *info, struct task_struct *p > return ret; > } > > +enum sig_handler { > + HANDLER_CURRENT, /* If reachable use the current handler */ > + HANDLER_SIG_DFL, /* Always use SIG_DFL handler semantics */ > + HANDLER_EXIT, /* Only visible as the proces exit code */ > +}; > + > /* > * Force a signal that the process can't ignore: if necessary > * we unblock the signal and change any SIG_IGN to SIG_DFL. > @@ -1310,7 +1316,8 @@ int do_send_sig_info(int sig, struct kernel_siginfo *info, struct task_struct *p > * that is why we also clear SIGNAL_UNKILLABLE. > */ > static int > -force_sig_info_to_task(struct kernel_siginfo *info, struct task_struct *t, bool sigdfl) > +force_sig_info_to_task(struct kernel_siginfo *info, struct task_struct *t, > + enum sig_handler handler) > { > unsigned long int flags; > int ret, blocked, ignored; > @@ -1321,9 +1328,10 @@ force_sig_info_to_task(struct kernel_siginfo *info, struct task_struct *t, bool > action = &t->sighand->action[sig-1]; > ignored = action->sa.sa_handler == SIG_IGN; > blocked = sigismember(&t->blocked, sig); > - if (blocked || ignored || sigdfl) { > + if (blocked || ignored || (handler != HANDLER_CURRENT)) { > action->sa.sa_handler = SIG_DFL; > - action->sa.sa_flags |= SA_IMMUTABLE; > + if (handler == HANDLER_EXIT) > + action->sa.sa_flags |= SA_IMMUTABLE; > if (blocked) { > sigdelset(&t->blocked, sig); > recalc_sigpending_and_wake(t); > @@ -1343,7 +1351,7 @@ force_sig_info_to_task(struct kernel_siginfo *info, struct task_struct *t, bool > > int force_sig_info(struct kernel_siginfo *info) > { > - return force_sig_info_to_task(info, current, false); > + return force_sig_info_to_task(info, current, HANDLER_CURRENT); > } > > /* > @@ -1660,7 +1668,7 @@ void force_fatal_sig(int sig) > info.si_code = SI_KERNEL; > info.si_pid = 0; > info.si_uid = 0; > - force_sig_info_to_task(&info, current, true); > + force_sig_info_to_task(&info, current, HANDLER_SIG_DFL); > } > > /* > @@ -1693,7 +1701,7 @@ int force_sig_fault_to_task(int sig, int code, void __user *addr > info.si_flags = flags; > info.si_isr = isr; > #endif > - return force_sig_info_to_task(&info, t, false); > + return force_sig_info_to_task(&info, t, HANDLER_CURRENT); > } > > int force_sig_fault(int sig, int code, void __user *addr > @@ -1813,7 +1821,8 @@ int force_sig_seccomp(int syscall, int reason, bool force_coredump) > info.si_errno = reason; > info.si_arch = syscall_get_arch(current); > info.si_syscall = syscall; > - return force_sig_info_to_task(&info, current, force_coredump); > + return force_sig_info_to_task(&info, current, > + force_coredump ? HANDLER_EXIT : HANDLER_CURRENT); > } > > /* For the crazy architectures that include trap information in > -- > 2.20.1 -- Kees Cook
