On Wed, Nov 17, 2021 at 11:05 AM Kyle Huey me@kylehuey.com wrote:

On Wed, Nov 17, 2021 at 10:51 AM Kees Cook keescook@chromium.org wrote:

...

On Wed, Nov 17, 2021 at 10:47:13AM -0800, Kyle Huey wrote:

...

rr, a userspace record and replay debugger[0], is completely broken on 5.16rc1. I bisected this to 00b06da29cf9dc633cdba87acd3f57f4df3fd5c7.

That patch makes two changes, it blocks sigaction from changing signal handlers once the kernel has decided to force the program to take a signal and it also stops notifying ptracers of the signal in the same circumstances. The latter behavior is just wrong. There's no reason that ptrace should not be able to observe and even change (non-SIGKILL) forced signals. It should be reverted.

This behavior change is also observable in gdb. If you take a program that sets SIGSYS to SIG_IGN and then raises a SIGSYS via SECCOMP_RET_TRAP and run it under gdb on a good kernel gdb will stop when the SIGSYS is raised, let you inspect program state, etc. After the SA_IMMUTABLE change gdb won't stop until the program has already died of SIGSYS.

Ah, hm, this was trying to fix the case where a program trips SECCOMP_RET_KILL (which is a "fatal SIGSYS"), and had been unobservable before. I guess the fix was too broad...

Perhaps I don't understand precisely what you mean by this, but gdb's behavior for a program that is SECCOMP_RET_KILLed was not changed by this patch (the SIGSYS is not observed until after program exit before or after this change).

Ah, maybe that behavior changed in 5.15 (my "before" here is a 5.14 kernel). I would argue that the debugger seeing the SIGSYS for SECCOMP_RET_KILL is desirable though ...

- Kyle

On Wed, Nov 17, 2021 at 03:04:31PM -0600, Eric W. Biederman wrote:

Kyle Huey me@kylehuey.com writes:

...

On Wed, Nov 17, 2021 at 11:05 AM Kyle Huey me@kylehuey.com wrote:

...

On Wed, Nov 17, 2021 at 10:51 AM Kees Cook keescook@chromium.org wrote:

...

On Wed, Nov 17, 2021 at 10:47:13AM -0800, Kyle Huey wrote:

...

rr, a userspace record and replay debugger[0], is completely broken on 5.16rc1. I bisected this to 00b06da29cf9dc633cdba87acd3f57f4df3fd5c7.

That patch makes two changes, it blocks sigaction from changing signal handlers once the kernel has decided to force the program to take a signal and it also stops notifying ptracers of the signal in the same circumstances. The latter behavior is just wrong. There's no reason that ptrace should not be able to observe and even change (non-SIGKILL) forced signals. It should be reverted.

This behavior change is also observable in gdb. If you take a program that sets SIGSYS to SIG_IGN and then raises a SIGSYS via SECCOMP_RET_TRAP and run it under gdb on a good kernel gdb will stop when the SIGSYS is raised, let you inspect program state, etc. After the SA_IMMUTABLE change gdb won't stop until the program has already died of SIGSYS.

Ah, hm, this was trying to fix the case where a program trips SECCOMP_RET_KILL (which is a "fatal SIGSYS"), and had been unobservable before. I guess the fix was too broad...

Perhaps I don't understand precisely what you mean by this, but gdb's behavior for a program that is SECCOMP_RET_KILLed was not changed by this patch (the SIGSYS is not observed until after program exit before or after this change).

The SA_IMMUTABLE change was to deal with failures seen in the seccomp test suite after the recent fatal signal refactoring. Mainly that a process that should have effectively performed do_exit() was suddenly visible to the tracer.

...

Ah, maybe that behavior changed in 5.15 (my "before" here is a 5.14 kernel). I would argue that the debugger seeing the SIGSYS for SECCOMP_RET_KILL is desirable though ...

This is definitely worth discussing, and probably in need of fixing (aka something in rr seems to have broken).

We definitely need protection against the race with sigaction.

The fundamental question becomes does it make sense and is it safe to allow a debugger to stop at, and possibly change these signals.

I have no problem with a debugger getting notified about a fatal (SECCOMP_RET_KILL*-originated) SIGSYS. But whatever happens, the kernel needs to make sure the process does not continue. (i.e. signal can't be changed/removed/etc.)

Stopping at something SA_IMMUTABLE as long as the signal is allowed to continue and kill the process when PTRACE_CONT happens seems harmless.

Allowing the debugger to change the signal, or change it's handling I don't know.

Right -- I'm fine with a visibility change (the seccomp test suite is just checking for various expected state machine changes across the various signal/death cases: as long as it _dies_, that's what we want. If a extra notification appears before it dies, that's okay, it just needs the test suite to change).

[...] Kees I am back to asking the question I had before I figured out SA_IMMUTABLE. Are there security concerns with debuggers intercepting SECCOMP_RET_KILL.

I see no problem with allowing a tracer to observe the signal, but the signalled process must have no way to continue running. If we end up in such a state, then a seccomp process with access to clone() and ptrace() can escape the seccomp sandbox. This is why seccomp had been using the big do_exit() hammer -- I really want to absolutely never have a bug manifest with a bypassed SECCOMP_RET_KILL: having a completely unavoidable "dying" state is needed.

On Wed, Nov 17, 2021 at 5:11 PM Linus Torvalds torvalds@linux-foundation.org wrote:

On Wed, Nov 17, 2021 at 4:37 PM Kyle Huey me@kylehuey.com wrote:

...

This fixes most of the issues with rr, but it still changes the ptrace behavior for the double-SIGSEGV case

Hmm. I think that's because of how "force_sigsgv()" works.

I absolutely detest that function.

So we have signal_setup_done() doing that

    if (failed)
            force_sigsegv(ksig->sig);

and then force_sigsegv() has that completely insane

    if (sig == SIGSEGV)
            force_fatal_sig(SIGSEGV);
    else
            force_sig(SIGSEGV);

behavior.

And I think I know the _reason_ for that complete insanity: when SIGSEGV takes a SIGSEGV, and there is a handler, we need to stop trying to send more SIGSEGV's.

Right, in our test we setup a SIGSEGV handler on an alt stack that doesn't actually exist, and then overflow the regular stack, and that would loop forever trying to setup SIGSEGV handlers if it weren't for force_sigsegv and the sigdfl=true stuff.

But it does mean that with my change, that second SIGSEGV now ends up being that SA_IMMUTABLE kind, so yeah, it broke the debugger test - where catching the second SIGSEGV is actually somewhat sensible (ok, not really, but at least understandable)

End result: I think we want not a boolean, but a three-way choice for that force_sig_info_to_task() thing:

with the following clarifications, yes

• unconditionally fatal (for things that just want to force an exit

and used to do do_exit())

no matter what the ptracer wants

• ignore valid and unblocked handler (for that SIGSEGV recursion

case, aka force "sigdfl")

but following the usual ptrace rules

• catching signal ok

So my one-liner isn't sufficient. It wants some kind of nasty enum.

At least the enum can be entirely internal to kernel/signal.c, I think. No need to expose this all to anything else.

        Linus

Yeah that's one way to solve the problem. I think you're right that fundamentally the problem here is that what SECCOMP_RET_KILL wants is not really a signal. To the extent that it wants a signal, what it really wants is SIGKILL, and the problem here is the code trying to act like SIGKILL but call it SIGSYS. I assume the ship for fixing that sailed years ago though.

- Kyle

Kees Cook keescook@chromium.org writes:

On Wed, Nov 17, 2021 at 05:20:33PM -0800, Kyle Huey wrote:

...

Yeah that's one way to solve the problem. I think you're right that fundamentally the problem here is that what SECCOMP_RET_KILL wants is not really a signal. To the extent that it wants a signal, what it really wants is SIGKILL, and the problem here is the code trying to act like SIGKILL but call it SIGSYS. I assume the ship for fixing that sailed years ago though.

Yeah, this was IIRC, a specific design choice (to distinguish a seccomp KILL from a SIGKILL), as desired by the sandboxing folks, and instead of using two different signals (one for KILL and one for TRAP), both used SIGSYS, with the KILL variant being uncatchable.

I see a general consensus on how to fix the regression. Linus patch plus some tweaks. I will get to work on that today.

For v5.15 I think all that needs to get fixed is what Linus fixed and the force_sigsegv case. That is my priority.

For v5.16-rc1+ the instances that became force_fatal_signal need a careful review to figure out which semantics we want.

Having a clear distinction between which forced signals we can let the debugger intercept and which ones we can not seems to be what needs to be added.

Kyle thank you for your explanation of what breaks. For future kernels I do need to do some work in this area and I will copy on the patches going forward. In particular I strongly suspect that changing the sigaction and blocked state of the signal for these synchronous signals is the wrong thing to do, especially if the process is not killed. I want to find another solution that does not break things but that also does not change the program state behind the programs back so things work differently under the debugger.

Eric

On Fri, Nov 19, 2021 at 08:07:36AM -0800, Kyle Huey wrote:

On Thu, Nov 18, 2021 at 8:12 AM Eric W. Biederman ebiederm@xmission.com wrote:

...

Kyle thank you for your explanation of what breaks. For future kernels I do need to do some work in this area and I will copy on the patches going forward. In particular I strongly suspect that changing the sigaction and blocked state of the signal for these synchronous signals is the wrong thing to do, especially if the process is not killed. I want to find another solution that does not break things but that also does not change the program state behind the programs back so things work differently under the debugger.

The heads up in the future is appreciated, thanks.

Yeah, I wonder if we could add you as a Reviewer in the MAINTAINERS file for ptrace/signal stuff? Then anyone using scripts/get_maintainers.pl would have a CC to you added.

Also, are there more instructions about running the rr tests? When the execve refactoring was happening, I tried it[1], but the results were unclear (there seemed to be a lot of warnings and it made me think I'd done something wrong on my end).

-Kees

[1] https://github.com/rr-debugger/rr/wiki/Building-And-Installing#tests

SA_IMMUTABLE fixed issues with force_sig_seccomp and the introduction for force_sig_fatal where the exit previously could not be interrupted but now it can. Unfortunately it added that behavior to all force_sig functions under the right conditions which debuggers usage of SIG_TRAP and debuggers handling of SIGSEGV.

Solve that by limiting SA_IMMUTABLE to just the cases that historically debuggers have not been able to intercept.

The first patch changes force_sig_info_to_task to take a flag that requests which behavior is desired.

The second patch adds force_exit_sig which replaces force_fatal_sig in the cases where historically userspace would only find out about the ``signal'' after the process has exited.

The first one with the hunk changing force_fatal_sig removed should be suitable for backporting to v5.15. v5.15 does not implement force_fatal_sig.

This should be enough to fix the regressions.

Kyle if you can double check me that I have properly fixed these issues that would be appreciated.

Any other review or suggestions to improve the names would be appreciated. I think I have named things reasonably well but I am very close to the code so it is easy for me to miss things.

Eric W. Biederman (2): signal: Don't always set SA_IMMUTABLE for forced signals signal: Replace force_fatal_sig with force_exit_sig when in doubt

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 | 36 ++++++++++++++++++++++++++++------- 11 files changed, 42 insertions(+), 19 deletions(-)

Eric

Kyle Huey me@kylehuey.com writes:

On Thu, Nov 18, 2021 at 2:05 PM Eric W. Biederman ebiederm@xmission.com 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.

s/known/know/

Fixed.

...

Reported-by: Kyle Huey me@kylehuey.com Reported-by: kernel test robot oliver.sang@intel.com Cc: stable@vger.kernel.org [1] https://lkml.kernel.org/r/CAP045AoMY4xf8aC_4QU_-j7obuEPYgTcnQQP3Yxk=2X90jtpj... [2] https://lkml.kernel.org/r/20211117150258.GB5403@xsang-OptiPlex-902

This link doesn't work.

Shame. I missed a trailing 0, but unfortunately that request did not go to list that is archived on lore. I will keep the link on the chance the message winds up in a lore archive in the future.

Eric

Linus,

Please pull the SA_IMMUTABLE-fixes-for-v5.16-rc2 branch from the git tree:

git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git SA_IMMUTABLE-fixes-for-v5.16-rc2

HEAD: fcb116bc43c8c37c052530ead79872f8b2615711 signal: Replace force_fatal_sig with force_exit_sig when in doubt

This is just a small set of changes where debuggers were no longer able to intercept synchronous SIGTRAP and SIGSEGV. This is essentially the change you suggested with all of i's dotted and the t's crossed so that ptrace can intercept all of the cases it has been able to intercept the past, and all of the cases that made it to exit without giving ptrace a chance still don't give ptrace a chance.

This change[1] has been tested by both Kyle and Kees.

Eric W. Biederman (2): signal: Don't always set SA_IMMUTABLE for forced signals signal: Replace force_fatal_sig with force_exit_sig when in doubt

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 | 36 ++++++++++++++++++++++++++++------- 11 files changed, 42 insertions(+), 19 deletions(-)

[1]: https://lkml.kernel.org/r/87h7c9qg7p.fsf_-_@email.froward.int.ebiederm.org

Eric

On Wed, Nov 17, 2021 at 1:05 PM Eric W. Biederman ebiederm@xmission.com wrote:

Kyle Huey me@kylehuey.com writes:

...

On Wed, Nov 17, 2021 at 11:05 AM Kyle Huey me@kylehuey.com wrote:

...

On Wed, Nov 17, 2021 at 10:51 AM Kees Cook keescook@chromium.org wrote:

...

On Wed, Nov 17, 2021 at 10:47:13AM -0800, Kyle Huey wrote:

...

rr, a userspace record and replay debugger[0], is completely broken on 5.16rc1. I bisected this to 00b06da29cf9dc633cdba87acd3f57f4df3fd5c7.

That patch makes two changes, it blocks sigaction from changing signal handlers once the kernel has decided to force the program to take a signal and it also stops notifying ptracers of the signal in the same circumstances. The latter behavior is just wrong. There's no reason that ptrace should not be able to observe and even change (non-SIGKILL) forced signals. It should be reverted.

This behavior change is also observable in gdb. If you take a program that sets SIGSYS to SIG_IGN and then raises a SIGSYS via SECCOMP_RET_TRAP and run it under gdb on a good kernel gdb will stop when the SIGSYS is raised, let you inspect program state, etc. After the SA_IMMUTABLE change gdb won't stop until the program has already died of SIGSYS.

Ah, hm, this was trying to fix the case where a program trips SECCOMP_RET_KILL (which is a "fatal SIGSYS"), and had been unobservable before. I guess the fix was too broad...

Perhaps I don't understand precisely what you mean by this, but gdb's behavior for a program that is SECCOMP_RET_KILLed was not changed by this patch (the SIGSYS is not observed until after program exit before or after this change).

Ah, maybe that behavior changed in 5.15 (my "before" here is a 5.14 kernel). I would argue that the debugger seeing the SIGSYS for SECCOMP_RET_KILL is desirable though ...

This is definitely worth discussing, and probably in need of fixing (aka something in rr seems to have broken).

I mean this in the nicest possible way: fixing this is not optional.

We definitely need protection against the race with sigaction.

Sure, no argument here, and that doesn't cause any problems for us.

The fundamental question becomes does it make sense and is it safe to allow a debugger to stop at, and possibly change these signals.

And the answer is yes, because at least some of these signals are generated by actions of the debugger (e.g. setting a breakpoint).

Stopping at something SA_IMMUTABLE as long as the signal is allowed to continue and kill the process when PTRACE_CONT happens seems harmless.

Allowing the debugger to change the signal, or change it's handling I don't know.

This is required to support breakpoints.

All of this is channeled through the following function.

...

static int force_sig_info_to_task(struct kernel_siginfo *info, struct task_struct *t, bool sigdfl) { unsigned long int flags; int ret, blocked, ignored; struct k_sigaction *action; int sig = info->si_signo;

  spin_lock_irqsave(&t->sighand->siglock, flags);
  action = &t->sighand->action[sig-1];
  ignored = action->sa.sa_handler == SIG_IGN;
  blocked = sigismember(&t->blocked, sig);
  if (blocked || ignored || sigdfl) {
          action->sa.sa_handler = SIG_DFL;
          action->sa.sa_flags |= SA_IMMUTABLE;
          if (blocked) {
                  sigdelset(&t->blocked, sig);
                  recalc_sigpending_and_wake(t);
          }
  }
  /*
   * Don't clear SIGNAL_UNKILLABLE for traced tasks, users won't expect
   * debugging to leave init killable.
   */
  if (action->sa.sa_handler == SIG_DFL && !t->ptrace)
          t->signal->flags &= ~SIGNAL_UNKILLABLE;
  ret = send_signal(sig, info, t, PIDTYPE_PID);
  spin_unlock_irqrestore(&t->sighand->siglock, flags);

  return ret;

}

Right now we have 3 conditions that trigger SA_IMMUTABLE.

• The sigdfl parameter is passed asking that userspace not be able to change the handling of the signal.

• A synchronous exception is taken and the signal is blocked.

• A synchronous exception is taken and the signal is ignored.

Delivering signals to a ptracee in the latter two cases is simply not optional. As it stands with your change, a program that blocks SIGTRAP or sets its SIGTRAP handler to SIG_IGN becomes undebuggable. If a debugger injects a breakpoint or uses PTRACE_SINGLESTEP on a tracee the delivery of that signal can't be controlled by the tracee's signal state.

Today because of how things are implemented the code most change the userspace state to allow the signal to kill the process. I really want to get rid of that, because that has other side effects. As part of getting rid of changing the state it is my plan to get rid of SA_IMMUTABLE as well. If I don't have to allow the debugger to stop and observe what is happening with the signal that change is much easier to implement.

The classic trigger of sigdfl is a recursive SIGSEGV.

However we have other cases like SECCOMP_RET_KILL where the kernel has never allowed userspace to intercept the killing of the process. Things that have messages like: "seccomp tried to change syscall nr or ip"

My brain is drawing a blank on how to analyze those.

Kees I am back to asking the question I had before I figured out SA_IMMUTABLE. Are there security concerns with debuggers intercepting SECCOMP_RET_KILL.

I think I can modify dequeue_synchronous_signal so that we can perform the necessary logic in get_signal rather than hack up the signal handling state in force_sig_info_to_task.

Except for the cases like SECCOMP_RET_KILL where the kernel has never allowed userspace to intercept the handling. I don't see any fundamental reason why ptrace could not intercept the signal. The handling is overriden to force the process to die, because the way userspace is currently configured to handle the signal does not work so it is necessary to kill the process.

I think there are cases where the userspace state is known to be sufficiently wrong that the kernel can not safely allow anything more than inspecting the state.

I can revisit the code to see if the kernel will get confused if something more is allowed. Still I really like the current semantics of SA_IMMUTABLE because these are cases where something wrong. If someone miscalculates how things are wrong it could result in the kernel getting confused and doing the wrong thing. Allowing the debugger to intercept the signal requires we risk miscalculating what is wrong.

Kyle how exactly is rr broken? Certainly a historical usage does not work. How does this affect actual real world debugging sessions?

rr is broken across the board because of specific things related to its handling of exit_group (namely we first block all signals in the tracee, so that we don't catch a signal during our handling of it, then we hijack the tracee to do some cleanup before exit_group is really allowed to execute, and we use e.g. PTRACE_SINGLESTEP that expects to punch through the signal blocking). But even if I fixed that, I expect there would be other issues. The expectation that these signals will be delivered is deeply embedded.

You noticed this and bisected the change quickly so I fully expect this does affect real world debugging sessions. I just want to know exactly how so that exactly what is wrong can be fixed.

I noticed this because we have a test suite we run against new kernel releases precisely to catch regressions like this.

You don't need rr to reproduce the underlying issue though. Compile the following

``` #include <signal.h> #include <stdio.h>

int main() { signal(SIGTRAP, SIG_IGN); printf("Hello World\n"); return 0; } ```

And try to break on the printf under gdb. After you fix that (and the equivalent where SIGTRAP is blocked) rr should be fine.

- Kyle

As far as I can tell SA_IMMUTABLE has only been backported to v5.15.x where in cleaning things up I made SECCOMP_RET_KILL susceptible to races with sigaction, and ptrace. Those races need to be closed or we need to decide that we don't actually care if the debugger does things.

Eric