Optionally deliver SIGCAP on capsicum violations.

[theraven]

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.

[emaste]

sys/sys/signal.h

134

Intentionally equal to SIGLIBRT?

[theraven]

• Fix copy-and-paste fail.

[theraven]

sys/sys/signal.h

134

Intentionally equal to SIGLIBRT?

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

[pauamma_gundo.com]

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

lib/libc/sys/procctl.2

433–440

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

[markj]

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

445

These lines should be indented by four spaces.

[markj]

sys/sys/signal.h

134

This should be documented in signal.3.

[theraven]

• Address code review comments.

[theraven]

In D33248#752692, @markj wrote:

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

134

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

[pauamma_gundo.com]

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

lib/libc/sys/procctl.2

439

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

[markj]

In D33248#752778, @theraven wrote:

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 .

[theraven]

• Fix comma splice.

[pauamma_gundo.com]

Manual page part LGTM now, grammar-wise.

[theraven]

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

[markj]

In D33248#763988, @theraven wrote:

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 ↗

(On Diff #99645)

An .Xr capsicum 4 would be worth adding IMO.

[kib]

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

445

This is still not done.

[jilles]

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

134

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

[theraven]

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

445

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.

[emaste]

sys/kern/kern_procctl.c

442–446

[theraven]

sys/kern/kern_procctl.c

442–446

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

[pauamma_gundo.com]

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

sys/kern/kern_procctl.c

442–446

Does https://wiki.freebsd.org/Phabricator#Optional:_Apply_the_Revision_Locally help?

[theraven]

• Address some reviewer comments.

[theraven]

I've rebased this, addressed reviewer comments (except the one that wants me to change some whitespace in a way that I don't fully understand), and tested it again with the Verona process-sandbox code.

sys/kern/kern_procctl.c

442–446

No, that lets me check out a patch. This is a suggestion block that sane code review systems let you commit to the diff via the UI. I cannot see any mechanism to do it with phabricator or arcanist.

I suggest that people who care about whitespace add a clang-format file that does the right thing.

[pauamma_gundo.com]

Other than the date bumps, manual pages LGTM.

lib/libc/gen/signal.3

31 ↗

(On Diff #116781)

Bump on commit.

lib/libc/sys/procctl.2

32

Bump on commit

[kib]

sys/kern/kern_procctl.c

411–417

Why it is 3 and not a symbolic expression?

This check seems to disable any trapcap ctl commands except
PROC_TRAPCAP_CTL_ENABLE_SIGCAP .

445

This means that the indent of lines 654 should be equal to the indent of line 653 plus four spaces. Lines 655 and 656 should have the same indent as line 654.