From: Kees Cook <keescook@chromium.org>
To: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: linux-kernel@vger.kernel.org, Kyle Huey <me@kylehuey.com>,
Linus Torvalds <torvalds@linux-foundation.org>,
Andrea Righi <andrea.righi@canonical.com>,
Shuah Khan <shuah@kernel.org>,
Alexei Starovoitov <ast@kernel.org>,
Andy Lutomirski <luto@amacapital.net>,
Will Drewry <wad@chromium.org>,
"open list:KERNEL SELFTEST FRAMEWORK"
<linux-kselftest@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
linux-hardening@vger.kernel.org,
Robert O'Callahan <rocallahan@gmail.com>,
Oliver Sang <oliver.sang@intel.com>,
lkp@lists.01.org, lkp@intel.com
Subject: Re: [PATCH 2/2] signal: Replace force_fatal_sig with force_exit_sig when in doubt
Date: Thu, 18 Nov 2021 15:53:20 -0800 [thread overview]
Message-ID: <202111181552.A6F6E9B3EC@keescook> (raw)
In-Reply-To: <871r3dqfv8.fsf_-_@email.froward.int.ebiederm.org>
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@kylehuey.com>
> Reported-by: kernel test robot <oliver.sang@intel.com>
> [1] https://lkml.kernel.org/r/CAP045AoMY4xf8aC_4QU_-j7obuEPYgTcnQQP3Yxk=2X90jtpjw@mail.gmail.com
> [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@xmission.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Tested-by: Kees Cook <keescook@chromium.org>
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
next prev parent reply other threads:[~2021-11-18 23:53 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-17 18:47 [REGRESSION] 5.16rc1: SA_IMMUTABLE breaks debuggers Kyle Huey
2021-11-17 18:51 ` Kees Cook
2021-11-17 19:05 ` Kyle Huey
2021-11-17 19:09 ` Kyle Huey
2021-11-17 21:04 ` Eric W. Biederman
2021-11-17 21:54 ` Kees Cook
2021-11-17 23:24 ` Linus Torvalds
2021-11-18 0:05 ` Kees Cook
2021-11-18 0:15 ` Linus Torvalds
2021-11-18 0:37 ` Kyle Huey
2021-11-18 1:11 ` Linus Torvalds
2021-11-18 1:20 ` Kyle Huey
2021-11-18 1:32 ` Kees Cook
2021-11-18 16:10 ` Eric W. Biederman
2021-11-19 16:07 ` Kyle Huey
2021-11-19 16:35 ` Kees Cook
2021-11-19 16:58 ` Kyle Huey
2021-11-18 21:58 ` [PATCH 0/2] SA_IMMUTABLE fixes Eric W. Biederman
2021-11-18 22:04 ` [PATCH 1/2] signal: Don't always set SA_IMMUTABLE for forced signals Eric W. Biederman
2021-11-18 23:52 ` Kees Cook
2021-11-18 23:54 ` Kees Cook
2021-11-19 15:08 ` Eric W. Biederman
2021-11-19 1:13 ` Kyle Huey
2021-11-19 15:03 ` Eric W. Biederman
2021-11-18 22:05 ` [PATCH 2/2] signal: Replace force_fatal_sig with force_exit_sig when in doubt Eric W. Biederman
2021-11-18 23:53 ` Kees Cook [this message]
2021-11-19 1:12 ` [PATCH 0/2] SA_IMMUTABLE fixes Kyle Huey
2021-11-19 15:41 ` [GIT PULL] SA_IMMUTABLE fixes for v5.16-rc2 Eric W. Biederman
2021-11-19 19:46 ` pr-tracker-bot
2021-11-17 22:29 ` [REGRESSION] 5.16rc1: SA_IMMUTABLE breaks debuggers Kyle Huey
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=202111181552.A6F6E9B3EC@keescook \
--to=keescook@chromium.org \
--cc=andrea.righi@canonical.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=ebiederm@xmission.com \
--cc=linux-hardening@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=lkp@intel.com \
--cc=lkp@lists.01.org \
--cc=luto@amacapital.net \
--cc=me@kylehuey.com \
--cc=oliver.sang@intel.com \
--cc=rocallahan@gmail.com \
--cc=shuah@kernel.org \
--cc=torvalds@linux-foundation.org \
--cc=wad@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).