Don't allow blocking of signals using sigreturn.
Subsystems (6)
PATCH 1/1 Don't allow blocking of signals using sigreturn.
This prevents processes running under seccomp, including SECCOMP_MODE_STRICT, from abusing sigreturn() to block signals other than SIGKILL intended to kill them. (For example, someone might decide to use alarm() to restrict the wall-clock runtime of a seccomp-restricted process.)
A side effect is that after this change, unblocking signals in a signal handler is a persistent change, not a change that can be reversed by returning from the signal handler.
Is that side effect tolerable, or does the patch have to be rewritten so that removing signals from the signal mask in a signal handler stays temporary? (That would probably make it more complicated.)
Or should I throw this patch away and write a patch for the prctl() manpage instead that documents that being able to call sigreturn() implies being able to effectively call sigprocmask(), at least on some architectures like X86?
(I hope I got the list of recipients right?)
--- arch/arm/kernel/signal.c | 2 +- arch/arm64/kernel/signal.c | 2 +- arch/arm64/kernel/signal32.c | 2 +- arch/x86/ia32/ia32_signal.c | 4 ++-- arch/x86/kernel/signal.c | 6 +++--- arch/x86/um/signal.c | 4 ++-- include/linux/signal.h | 1 + kernel/signal.c | 16 ++++++++++++++++ 8 files changed, 27 insertions(+), 10 deletions(-)
arch/arm/kernel/signal.c +1 -1
@@ -147,7 +147,7 @@ static int restore_sigframe(struct pt_regs *regs, struct sigframe __user *sf) err =__copy_from_user (&set, &sf->uc.uc_sigmask,sizeof (set));if (err ==0 ) -set_current_blocked (&set); +unblock_current (&set);__get_user_error (regs->ARM_r0, &sf->uc.uc_mcontext.arm_r0, err);__get_user_error (regs->ARM_r1, &sf->uc.uc_mcontext.arm_r1, err);
arch/arm64/kernel/signal.c +1 -1
@@ -101,7 +101,7 @@ static int restore_sigframe(struct pt_regs *regs, err =__copy_from_user (&set, &sf->uc.uc_sigmask,sizeof (set));if (err ==0 ) -set_current_blocked (&set); +unblock_current (&set);for (i =0 ; i <31 ; i++) __get_user_error(regs->regs[i], &sf->uc.uc_mcontext.regs[i],
arch/arm64/kernel/signal32.c +1 -1
@@ -306,7 +306,7 @@ static int compat_restore_sigframe(struct pt_regs *regs, err =get_sigset_t (&set, &sf->uc.uc_sigmask);if (err ==0 ) {sigdelsetmask (&set, ~_BLOCKABLE); -set_current_blocked (&set); +unblock_current (&set); }__get_user_error (regs->regs[0 ], &sf->uc.uc_mcontext.arm_r0, err);
arch/x86/ia32/ia32_signal.c +2 -2
@@ -222,7 +222,7 @@ asmlinkage long sys32_sigreturn(void) sizeof(frame->extramask)))) goto badframe; -set_current_blocked (&set); +unblock_current (&set);if (ia32_restore_sigcontext (regs, &frame->sc, &ax)) goto badframe;
@@ -247,7 +247,7 @@ asmlinkage long sys32_rt_sigreturn(void)if (__copy_from_user (&set, &frame->uc.uc_sigmask,sizeof (set))) goto badframe; -set_current_blocked (&set); +unblock_current (&set);if (ia32_restore_sigcontext (regs, &frame->uc.uc_mcontext, &ax)) goto badframe;
arch/x86/kernel/signal.c +3 -3
@@ -551,7 +551,7 @@ asmlinkage unsigned long sys_sigreturn(void) sizeof(frame->extramask)))) goto badframe; -set_current_blocked (&set); +unblock_current (&set);if (restore_sigcontext (regs, &frame->sc, &ax)) goto badframe;
@@ -577,7 +577,7 @@ asmlinkage long sys_rt_sigreturn(void)if (__copy_from_user (&set, &frame->uc.uc_sigmask,sizeof (set))) goto badframe; -set_current_blocked (&set); +unblock_current (&set);if (restore_sigcontext (regs, &frame->uc.uc_mcontext, &ax)) goto badframe;
@@ -789,7 +789,7 @@ asmlinkage long sys32_x32_rt_sigreturn(void)if (__copy_from_user (&set, &frame->uc.uc_sigmask,sizeof (set))) goto badframe; -set_current_blocked (&set); +unblock_current (&set);if (restore_sigcontext (regs, &frame->uc.uc_mcontext, &ax)) goto badframe;
arch/x86/um/signal.c +2 -2
@@ -476,7 +476,7 @@ long sys_sigreturn(void) copy_from_user(&set.sig[1], extramask, sig_size)) goto segfault; -set_current_blocked (&set); +unblock_current (&set);if (copy_sc_from_user (¤t->thread.regs, sc)) goto segfault;
@@ -584,7 +584,7 @@ long sys_rt_sigreturn(void)if (copy_from_user (&set, &uc->uc_sigmask,sizeof (set))) goto segfault; -set_current_blocked (&set); +unblock_current (&set);if (copy_sc_from_user (¤t->thread.regs, &uc->uc_mcontext)) goto segfault;
include/linux/signal.h +1 -0
@@ -238,6 +238,7 @@ extern int do_sigtimedwait(const sigset_t *, siginfo_t *,extern int sigprocmask (int ,sigset_t *,sigset_t *);extern void set_current_blocked (sigset_t *);extern void __set_current_blocked (const sigset_t *); +extern void unblock_current (const sigset_t *);extern int show_unhandled_signals;extern int sigsuspend (sigset_t *);
kernel/signal.c +16 -0
@@ -2544,6 +2544,22 @@ void __set_current_blocked(const sigset_t *newset)spin_unlock_irq (&tsk->sighand->siglock); } +/** +* unblock_current - change current->blocked mask, but only allow +* unblocking of signals. +* @newset: new mask +* +* This method should be used in sigreturn implementations to prevent +* seccomp-isolated processes from blocking signals using sigreturn. +*/ +void unblock_current (const sigset_t *newset) +{ +spin_lock_irq (¤t->sighand->siglock); +sigandsets (¤t->blocked, ¤t->blocked, newset); +recalc_sigpending (); +spin_unlock_irq (¤t->sighand->siglock); +} +/* * This is also useful for kernel threads that want to temporarily * (or permanently) block certain signals.
Signature · 1 line
2.1.4
Quoted 5 lines · Jann Horn
Or should I throw this patch away and write a patch for the
prctl()manpage instead that documents that being able to callsigreturn()implies being able to effectively callsigprocmask(), at least on some architectures like X86?
Well, that is the semantics of sigreturn(). It is essentially setcontext() [which includes the actions of sigprocmask()], but with restrictions on parameter placement (at least on x86).
You could introduce some setting to restrict that aspect for seccomp processes, but you can't change this for normal processes without breaking things.
Quoted 7 lines · Mikael Pettersson
Quoted 5 lines · Jann Horn
Or should I throw this patch away and write a patch for the
prctl()manpage instead that documents that being able to callsigreturn()implies being able to effectively callsigprocmask(), at least on some architectures like X86?
Quoted 7 lines
Well, that is the semantics of
sigreturn(). It is essentiallysetcontext()[which includes the actions ofsigprocmask()], but with restrictions on parameter placement (at least on x86).You could introduce some setting to restrict that aspect for seccomp processes, but you can't change this for normal processes without breaking things.
Which leads to the interesting question: does anyone ever call sigreturn with a different signal mask than the kernel put there during signal delivery or, even more strangely, with a totally made up context? I suspect that the former does happen, even if the latter may be rare or completely implausible.
I certainly have code that modifies GPRs in the context prior to sigreturn.
--Andy
Signature · 2 lines
Andy Lutomirski AMA Capital Management, LLC
Quoted 12 lines · Andy Lutomirski
Quoted 11 lines · Mikael Pettersson
Quoted 5 lines · Jann Horn
Or should I throw this patch away and write a patch for the
prctl()manpage instead that documents that being able to callsigreturn()implies being able to effectively callsigprocmask(), at least on some architectures like X86?Well, that is the semantics of
sigreturn(). It is essentiallysetcontext()[which includes the actions ofsigprocmask()], but with restrictions on parameter placement (at least on x86).
Quoted 7 lines
Quoted 3 lines
You could introduce some setting to restrict that aspect for seccomp processes, but you can't change this for normal processes without breaking things.
Which leads to the interesting question: does anyone ever call sigreturn with a different signal mask than the kernel put there during signal delivery
Yes. Either a sigfillset();sigdelset(SIGSEGV), or a copy of the thread's sigmask from a previous sigframe.
Quoted 2 lines
or, even more strangely, with a totally made up
context?
Not "totally made up", but certainly with adjustments(*) made to both GPRs and PC. In a different piece of SW: FPU controls.
(*) Rolling back or force-committing a micro-transaction until PC+GPRs represent the state at an original instruction boundary. This was in a product using dynamic binary instrumentation.
PATCH 1/1 seccomp.2: Add note about alarm(2) not being sufficient to limit runtime
In reply to Mikael PetterssonQuoted 7 lines · Mikael Pettersson
Quoted 5 lines · Jann Horn
Or should I throw this patch away and write a patch for the
prctl()manpage instead that documents that being able to callsigreturn()implies being able to effectively callsigprocmask(), at least on some architectures like X86?
Quoted 7 lines
Well, that is the semantics of
sigreturn(). It is essentiallysetcontext()[which includes the actions ofsigprocmask()], but with restrictions on parameter placement (at least on x86).You could introduce some setting to restrict that aspect for seccomp processes, but you can't change this for normal processes without breaking things.
Then I think it's probably better and easier to just document the existing behavior? If a new setting would have to be introduced and developers would need to be aware of that, it's probably easier to just tell everyone to use SIGKILL.
Does this manpage patch look good?
--- man2/seccomp.2 | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)
man2/seccomp.2 +25 -0
@@ -64,6 +64,31 @@ Strict secure computing mode is useful for number-crunching applications that may need to execute untrusted byte code, perhaps obtained by reading from a pipe or socket. +Note that although the calling thread can no longer call +.BR sigprocmask (2), +it can use +.BR sigreturn (2) +to block all signals apart from +.BR SIGKILL +and +.BR SIGSTOP . +Therefore, to reliably terminate it, +.BR SIGKILL +has to be used, meaning that e.g. +.BR alarm (2) +is not sufficient for restricting its runtime. Instead, use +.BR timer_create (2) +with +.BR SIGEV_SIGNAL +and +.BR sigev_signo +set to +.BR SIGKILL +or use +.BR setrlimit (2) +to set the hard limit for +.BR RLIMIT_CPU . + This operation is available only if the kernel is configured with .BR CONFIG_SECCOMP enabled.
Signature · 1 line
2.1.4
Re: [PATCH] seccomp.2: Add note about alarm(2) not being sufficient to limit runtime
Quoted 16 lines · Jann Horn
Quoted 7 lines · Mikael Pettersson
Quoted 5 lines · Jann Horn
Or should I throw this patch away and write a patch for the
prctl()manpage instead that documents that being able to callsigreturn()implies being able to effectively callsigprocmask(), at least on some architectures like X86?Quoted 7 lines
Well, that is the semantics of
sigreturn(). It is essentiallysetcontext()[which includes the actions ofsigprocmask()], but with restrictions on parameter placement (at least on x86).You could introduce some setting to restrict that aspect for seccomp processes, but you can't change this for normal processes without breaking things.
Quoted 6 lines
Then I think it's probably better and easier to just document the existing behavior? If a new setting would have to be introduced and developers would need to be aware of that, it's probably easier to just tell everyone to use SIGKILL.
Does this manpage patch look good?
Looks good to me.
FWIW, if we wanted to fix this in the kernel, I think it could be easier to add SIG_KILL which would be just like SIG_DFL except always fatal even if masked rather than coming up with complicated changes to sigreturn.
--Andy
Quoted 44 lines
--- man2/seccomp.2 | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)man2/seccomp.2 +25
-1@@ -64,6 +64,31 @@ Strict secure computing mode is useful for number-crunching applications that may need to execute untrusted byte code, perhaps obtained by reading from a pipe or socket. +Note that although the calling thread can no longer call +.BR sigprocmask (2), +it can use +.BR sigreturn (2) +to block all signals apart from +.BR SIGKILL +and +.BR SIGSTOP . +Therefore, to reliably terminate it, +.BR SIGKILL +has to be used, meaning that e.g. +.BR alarm (2) +is not sufficient for restricting its runtime. Instead, use +.BR timer_create (2) +with +.BR SIGEV_SIGNAL +and +.BR sigev_signo +set to +.BR SIGKILL +or use +.BR setrlimit (2) +to set the hard limit for +.BR RLIMIT_CPU . + This operation is available only if the kernel is configured with .BR CONFIG_SECCOMP enabled. --2.1.4
Signature · 6 lines
Andy Lutomirski AMA Capital Management, LLC -- To unsubscribe from this list: send the line "unsubscribe linux-man" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] seccomp.2: Add note about alarm(2) not being sufficient to limit runtime
In reply to Jann HornQuoted 64 lines · Jann Horn
Quoted 7 lines · Mikael Pettersson
Quoted 5 lines · Jann Horn
Or should I throw this patch away and write a patch for the
prctl()manpage instead that documents that being able to callsigreturn()implies being able to effectively callsigprocmask(), at least on some architectures like X86?Quoted 7 lines
Well, that is the semantics of
sigreturn(). It is essentiallysetcontext()[which includes the actions ofsigprocmask()], but with restrictions on parameter placement (at least on x86).You could introduce some setting to restrict that aspect for seccomp processes, but you can't change this for normal processes without breaking things.
Then I think it's probably better and easier to just document the existing behavior? If a new setting would have to be introduced and developers would need to be aware of that, it's probably easier to just tell everyone to use SIGKILL.
Does this manpage patch look good?
--- man2/seccomp.2 | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)man2/seccomp.2 +25
-1@@ -64,6 +64,31 @@ Strict secure computing mode is useful for number-crunching applications that may need to execute untrusted byte code, perhaps obtained by reading from a pipe or socket. +Note that although the calling thread can no longer call +.BR sigprocmask (2), +it can use +.BR sigreturn (2) +to block all signals apart from +.BR SIGKILL +and +.BR SIGSTOP . +Therefore, to reliably terminate it, +.BR SIGKILL +has to be used, meaning that e.g. +.BR alarm (2) +is not sufficient for restricting its runtime. Instead, use +.BR timer_create (2) +with +.BR SIGEV_SIGNAL +and +.BR sigev_signo +set to +.BR SIGKILL +or use +.BR setrlimit (2) +to set the hard limit for +.BR RLIMIT_CPU . + This operation is available only if the kernel is configured with .BR CONFIG_SECCOMP enabled. --
Quoted 2 lines
2.1.4
Thanks! This looks good.
-Kees
Signature · 6 lines
Kees Cook Chrome OS Security -- To unsubscribe from this list: send the line "unsubscribe linux-man" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] seccomp.2: Add note about alarm(2) not being sufficient to limit runtime
In reply to Jann HornQuoted 16 lines · Jann Horn
Quoted 7 lines · Mikael Pettersson
Quoted 5 lines · Jann Horn
Or should I throw this patch away and write a patch for the
prctl()manpage instead that documents that being able to callsigreturn()implies being able to effectively callsigprocmask(), at least on some architectures like X86?Quoted 7 lines
Well, that is the semantics of
sigreturn(). It is essentiallysetcontext()[which includes the actions ofsigprocmask()], but with restrictions on parameter placement (at least on x86).You could introduce some setting to restrict that aspect for seccomp processes, but you can't change this for normal processes without breaking things.
Quoted 6 lines
Then I think it's probably better and easier to just document the existing behavior? If a new setting would have to be introduced and developers would need to be aware of that, it's probably easier to just tell everyone to use SIGKILL.
Does this manpage patch look good?
LGTM
Quoted 43 lines
--- man2/seccomp.2 | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)man2/seccomp.2 +25
-0@@ -64,6 +64,31 @@ Strict secure computing mode is useful for number-crunching applications that may need to execute untrusted byte code, perhaps obtained by reading from a pipe or socket. +Note that although the calling thread can no longer call +.BR sigprocmask (2), +it can use +.BR sigreturn (2) +to block all signals apart from +.BR SIGKILL +and +.BR SIGSTOP . +Therefore, to reliably terminate it, +.BR SIGKILL +has to be used, meaning that e.g. +.BR alarm (2) +is not sufficient for restricting its runtime. Instead, use +.BR timer_create (2) +with +.BR SIGEV_SIGNAL +and +.BR sigev_signo +set to +.BR SIGKILL +or use +.BR setrlimit (2) +to set the hard limit for +.BR RLIMIT_CPU . + This operation is available only if the kernel is configured with .BR CONFIG_SECCOMP enabled.Signature · 1 line
2.1.4
Re: [PATCH] seccomp.2: Add note about alarm(2) not being sufficient to limit runtime
In reply to Jann HornHello Jann,
Quoted 16 lines · Jann Horn
Quoted 7 lines · Mikael Pettersson
Quoted 5 lines · Jann Horn
Or should I throw this patch away and write a patch for the
prctl()manpage instead that documents that being able to callsigreturn()implies being able to effectively callsigprocmask(), at least on some architectures like X86?Quoted 7 lines
Well, that is the semantics of
sigreturn(). It is essentiallysetcontext()[which includes the actions ofsigprocmask()], but with restrictions on parameter placement (at least on x86).You could introduce some setting to restrict that aspect for seccomp processes, but you can't change this for normal processes without breaking things.
Quoted 6 lines
Then I think it's probably better and easier to just document the existing behavior? If a new setting would have to be introduced and developers would need to be aware of that, it's probably easier to just tell everyone to use SIGKILL.
Does this manpage patch look good?
Patch applied, with Acks from Andy, Mikael, and Kees (I don't usually get patches whose pedigree is that good. Thanks!)
I tweaked a few wordings. You can find the changes in Git [1]
Cheers,
Michael
Quoted 41 lines
--- man2/seccomp.2 | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)man2/seccomp.2 +25
-0@@ -64,6 +64,31 @@ Strict secure computing mode is useful for number-crunching applications that may need to execute untrusted byte code, perhaps obtained by reading from a pipe or socket. +Note that although the calling thread can no longer call +.BR sigprocmask (2), +it can use +.BR sigreturn (2) +to block all signals apart from +.BR SIGKILL +and +.BR SIGSTOP . +Therefore, to reliably terminate it, +.BR SIGKILL +has to be used, meaning that e.g. +.BR alarm (2) +is not sufficient for restricting its runtime. Instead, use +.BR timer_create (2) +with +.BR SIGEV_SIGNAL +and +.BR sigev_signo +set to +.BR SIGKILL +or use +.BR setrlimit (2) +to set the hard limit for +.BR RLIMIT_CPU . + This operation is available only if the kernel is configured with .BR CONFIG_SECCOMP enabled.
Signature · 7 lines
Michael Kerrisk Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/ Linux/UNIX System Programming Training: http://man7.org/training/ -- To unsubscribe from this list: send the line "unsubscribe linux-man" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html