Page MenuHomeFreeBSD

Optionally deliver SIGCAP on capsicum violations.
AcceptedPublic

Authored by theraven on Dec 3 2021, 3:13 PM.

Details

Summary

SIGTRAP is useful for debugging but it has particular interactions with
the debugger that make it difficult for a process to use the existing
mechanism to provide a fall-back codepath for Capsicum failures.

On Linux, seccomp-bpf can raise SIGSYS to allow this on blocked calls.
After discussion it was decided that we should add a new signal, rather
than overload SIGSYS, for this. This makes it easy to not receive a
signal for system calls that don't exist at all, only ones that exist
but are not permitted in the current context.

Test Plan

Tested with:

https://github.com/microsoft/verona/tree/master/experiments/process_sandbox

Both with the SIGTRAP and SIGCAP mode. With SIGCAP, a debugger can properly
trace the child process and get useful information.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 43212
Build 40100: arc lint + arc unit

Event Timeline

sys/sys/signal.h
133

Intentionally equal to SIGLIBRT?

  • Fix copy-and-paste fail.
sys/sys/signal.h
133

Intentionally equal to SIGLIBRT?

No, that's a copy and paste fail. Fixed.

Not competent to review the source code, but other than that disambiguation request, the manual page change LGTM.

lib/libc/sys/procctl.2
433–441

"either a synchronous", to me, hints that SIGTRAP is delivered synchronously but SIGCAP isn't or may not be, but doesn't state that conclusively. If this is what you mean If this isn't what you mean and both are synchronous, "synchronous delivery of either" would be better. If SIGCAP is or can be delivered asynchronously, "or an asynchronous" or something similar would disambiguate.

FWIW, I tried the tests for your process_sandbox (with this patch applied of course) and one test consistently fails:

markj@biggie> ./test-onebitsem-child
{unknown file}:0 in {unknown function}: Test timed out

Abort trap (core dumped)
...
(gdb) bt                                                                               
#0  thr_kill () at thr_kill.S:4                                                        
#1  0x00000008004a8cf4 in __raise (s=s@entry=6) at /root/freebsd/lib/libc/gen/raise.c:52
#2  0x000000080055a3c9 in abort () at /root/freebsd/lib/libc/stdlib/abort.c:67                                                                                                
#3  0x000000000020a1a9 in snmalloc::PALPOSIX<snmalloc::PALFreeBSD>::error(char const*) ()
#4  0x00000000002112a4 in void sandbox::invariant_fail<test_sem<sandbox::platform::UMtxOneBitSem>()::{lambda()#2}::operator()() const::FMT_COMPILE_STRING>(test_sem<sandbox::p
latform::UMtxOneBitSem>()::{lambda()#2}::operator()() const::FMT_COMPILE_STRING, fmt::v8::basic_format_args<fmt::v8::basic_format_context<fmt::v8::appender, char> >, sandbox:
:source_location) ()                                                                                                                                                          
#5  0x0000000000209248 in void test_sem<sandbox::platform::UMtxOneBitSem>() ()         
#6  0x0000000000208ff9 in main ()
sys/kern/kern_procctl.c
444

These lines should be indented by four spaces.

sys/sys/signal.h
133

This should be documented in signal.3.

theraven marked an inline comment as done.
  • Address code review comments.

FWIW, I tried the tests for your process_sandbox (with this patch applied of course) and one test consistently fails:

markj@biggie> ./test-onebitsem-child

This test doesn't touch any of the new code (test-sandbox-fake-open does). Could you take a look at: https://github.com/microsoft/verona/blob/master/experiments/process_sandbox/include/process_sandbox/platform/onebitsem_umtx.h

The test is a unit test that's failing is a unit test for this code and fails intermittently for me (more often on 14 than 12, where it fails very rarely). The equivalent Linux code is here: https://github.com/microsoft/verona/blob/master/experiments/process_sandbox/include/process_sandbox/platform/onebitsem_futex.h

The Linux code never fails for me on Ubuntu. From this I deduce one of the following is true:

  • My use of _umtx_op is incorrect.
  • There is a bug in the implementation of _umtx_op.
  • There is a bug in the test.

The first seems very likely, the second seems moderately likely. The third seems less likely since it passes on Linux (though there is some difference between how the child processes are created - on Linux it will be called in a vfork context, on FreeBSD it's in a pdfork child by default, though if you toggle ENABLE_PROCDESC in ccmake then it will also use vfork on FreeBSD).

sys/sys/signal.h
133

This makes me realise that I should be setting the default behaviour but the array that describes the default behaviour in the kernel doesn't cover SIGTHR or SIGLIBRT (or anything more than NSIG, which is documented as only containing the number of old signals).

Other than that comma splice, both manual page changes (signal.3 and procctl.2) LGTM now.

lib/libc/sys/procctl.2
438

Comma splice after "mode". I'd either s/,/;/ or s/,/ and/

The test is a unit test that's failing is a unit test for this code and fails intermittently for me (more often on 14 than 12, where it fails very rarely). The equivalent Linux code is here: https://github.com/microsoft/verona/blob/master/experiments/process_sandbox/include/process_sandbox/platform/onebitsem_futex.h

The Linux code never fails for me on Ubuntu. From this I deduce one of the following is true:

  • My use of _umtx_op is incorrect.
  • There is a bug in the implementation of _umtx_op.
  • There is a bug in the test.

None of the above. :)

It's a bug in the libc build which results in pdfork() not being interposed by libthr, so various internal locks do not get reinitialized resulting in various deadlocks when the child tries to exit. Fixed by commit cbdec8db18b5 .

Manual page part LGTM now, grammar-wise.

Are there any other blockers to committing this? @markj, please can you approve if not?

Are there any other blockers to committing this? @markj, please can you approve if not?

No, just a couple of minor comments. Consider the diff approved.

I missed that @kib was not on the review, I think he should get some time to take a look. There is PR 259778 for context.

lib/libc/gen/signal.3
264

An .Xr capsicum 4 would be worth adding IMO.

Don't you want to have core dumped for unhandled SIGCAP?

sys/kern/kern_procctl.c
417

You validate only the 2 low bits of the data, basically making it impossible to safely extend the interface in a compatible way.

428

Why not to write p->p_flag2 &= ~(P2_TRAPCAP | P2_SIGCAP);

444

This is still not done.

jilles added inline comments.
sys/kern/subr_syscall.c
231

So SIGCAP continues to use the si_code TRAP_CAP. Perhaps this should use a separate define CAP_CAP?

sys/sys/signal.h
133

Perhaps the array should be extended a bit (but possibly not in this review).

In D33248#764167, @kib wrote:

Don't you want to have core dumped for unhandled SIGCAP?

I don't have strong opinions here. I can imagine setting this in a process and inheriting it from a parent process in something that doesn't set up a signal handler and expects to properly handle ENOTCAPABLE returns. Clearing it there is probably a bug, so dumping core is probably the right thing to do, though rtld will crash currently if it runs with this flag inherited, so I can see arguments either way.

sys/kern/kern_procctl.c
444

I can't figure out what this means. If I run clang-format on this file, I get a completely different wrapping here.

sys/kern/subr_syscall.c
231

I could. The only impact it would have for me is requiring more #ifdefs in the code that uses this, so I don't see a compelling reason to do so.

sys/kern/kern_procctl.c
442–445
sys/kern/kern_procctl.c
442–445

How do I apply a diff from a phab comment (there's a button on GitHub PRs, I can't find the equivalent here).

Formally approving the manual pages once .Xr capsicum 4 is added to signal(3).

sys/kern/kern_procctl.c
442–445
This revision is now accepted and ready to land.Apr 1 2022, 10:25 PM