linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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

  reply	other threads:[~2021-11-18 23:53 UTC|newest]

Thread overview: 32+ 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
2021-11-18  5:43 ` Thorsten Leemhuis
2021-11-20  6:13   ` Thorsten Leemhuis

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 \
    --subject='Re: [PATCH 2/2] signal: Replace force_fatal_sig with force_exit_sig when in doubt' \
    /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

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).