Signal handler for SIGBUS, SIGSEGV #1070

alxiord · 2019-04-23T18:08:10Z

Added a custom signal handler that terminates the Firecracker process (with libc::_exit) when a SIGSEGV or SIGBUS is intercepted. See #1064 for details on the current handling of these signals.

Issue #, if available: #1064

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

serban300 · 2019-04-25T11:14:18Z

Could we get away with just printing a hex dump of the siginfo struct ? Something like this maybe.

serban300 · 2019-04-25T11:36:31Z

Hmmm actually, if we write the handler in C and just call it from Rust shouldn't it work on different versions of linux, provided that the names of the members of the struct remain the same. Because the definition of the struct will be imported directly from siginfo.h .

acatangiu · 2019-04-30T10:18:54Z

vmm/src/signal.rs

@@ -0,0 +1,230 @@
+// Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved.


I'm not really keen on having linux signals related code split around different crates like so. For example there is also sys_util/src/signal.rs.

The logic looks solid 👍

nbsdx · 2019-05-01T18:51:39Z

Just dropping by with my 2 cents (I spent a fair amount of time messing with siginfo_t in rust to get some waitid stuff working properly) and trying to share issues I ran into. My full comments on the issue can be found in rust-lang/libc#716.

@serban300 If the user built the binaries themselves, yes. But distributed versions would need to have runtime kernel version detection to pick the correct structure layout to use, or be built to target specific kernel versions.

@aghecenco Your _siginfo_t struct is going to be larger than the expected 128 bytes. The 29 c_int padding assumes a 4 byte alignment of the union, which isn't the case. The SIGSYS and SIGFAULT union members require 8 byte alignment on 64-bit systems due to the void * being the first member of those structures. Since the sifields member of _siginfo_t is at an offset of 12 bytes, it will add 4 bytes of padding to fix alignment issues, resulting in a 16 byte header, and then your 29*4 bytes of padding, resulting in a total size of 132 bytes. So the padding should probably be conditionally sized based on the alignment of the system.

alxiord · 2019-05-06T10:40:29Z

@nbsdx Thanks for that, good call! I followed your issue - I first stumbled across it when I added the signal handler for seccomp, and again now for the more general purpose one. I got a bit lost in the details and would need to backtrack and fix the alignment issues but first I need to know if there's value for Firecracker in doing this in Rust. The problem is with kernel compatibility, as Firecracker isn't strongly tied to a specific kernel version, and I find it would be overkill to add kernel-specific functionalities to work around known bugs in older kernels that we support (plus runtime version checks).

I don't understand why the solution @serban300 proposes wouldn't work. The C API also exposes #define'd getters for certain values, and their implementation differs from kernel to kernel - see for instance si_pkey:

4.14:

#define si_pkey		_sifields._sigfault._pkey

4.16:

#define si_pkey		_sifields._sigfault._addr_pkey._pkey

Using the getters instead of accessing the struct fields themselves should protect against compatibility issues.

That said, I'm wary about adding raw C in Firecracker. @firecracker-microvm/compute-capsule thoughts?

nbsdx · 2019-05-06T17:31:48Z

Sorry, I wasn't particularly clear on that. The issue would be with binary releases. User-built binaries would generally be fine as they would be built against the correct headers. But if the firecracker build server builds against 4.14, and a user tries to run the prebuilt binaries against 4.16, the field access will be incorrect, since the kernel/userspace interface (ie, structure layout) changed.

For what it's worth, Linux 4.14 LTS support ends in Jan 2020. But since Firecracker claims to support 4.14 onwards, there should be proper handling of kernel interface changes (probably at minimum across Linux LTS versions).

Again - just my 2 cents - I obviously don't have any control over decisions :)

alxiord · 2019-05-07T11:35:30Z

@nbsdx You're right again, I'd missed that, it won't work. For now we resolved to restring handling to _exit and revisit the idea if the need for more siginfo stuff arises later. I will rework this PR in this direction.

@acatangiu yup there's already signal handling functionality split between the sys_util and vnm crates. I propose this:

keep generic stuff in sys_util (unless you know a better place). This would be code such as install a generic signal handler; for instance, the setup_sigsys_handler in vmm would move here and change to a generic form;
place Firecracker/VMM specifics in vmm. This would consist in the actual signal handlers, where we want exact actions to be taken for specific signals (well, not much other than logging and exiting, but still).

What do you think?

alxiord · 2019-05-09T15:53:22Z

vmm/src/signal_handler.rs

@@ -144,8 +170,8 @@ mod tests {
        assert!(filter.apply().is_ok());
        assert_eq!(METRICS.seccomp.num_faults.count(), 0);

-        // Calls the blacklisted SYS_getpid.
-        let _pid = process::id();
+        // Call the blacklisted `SYS_mkdir`.


TLDR I allowed getpid and kill and changed the blackisted syscall from getpid to mkdir to make the GNU test pass consistently.

Long version:

The glibc SYS_getpid caches its result, so it doesn't always do the syscall. Therefore, process::id didn't always break the seccomp filter on GNU - depending on whether SYS_getpid had been called before.

I switched to:
syscall(libc::SYS_getpid); // -> seccomp violation, handled ... raise(SIGBUS); // -> signal handler called
That didn't work because glibc's raise calls both getpid and gettid (in some implementations - for instance, on the test machine it does, on mine it doesn't) so getpid had to be whitelisted. That's when I switched to whitelisting getpid and gettid for GNU - but:

the code became overly complicated with #[cfg]s for this simple test;

the test still failed on the test machine - despite gettid being whitelisted, it still caused a SIGSYS which, even more weirdly, did not get handled by the signal handler and terminated the test.

At this point I decided to eliminate #[cfg]s and workarounds and:

cause a SIGSYS with mkdir (instead of getpid);

emit a SIGBUS with kill (instead of raise);

whitelist kill and getpid (for the kill to go through).
This solution works on both GNU and musl.

Leaving this clarification here:

whitelist kill and getpid (for the kill to go through).

is done just in a particular unit-test, not whitelisted in general.

acatangiu

Love the overall look and documentation 👍

sys_util/src/signal.rs

tests/integration_tests/functional/test_signals.py

acatangiu · 2019-05-10T11:21:35Z

tests/integration_tests/functional/test_signals.py

+
+    msg = 'Shutting down VM after intercepting signal {}'.format(signum)
+    lines = log_fifo.sequential_reader(3)
+    assert msg in lines[1]


Are you sure it will always be on line 1?

Umm... no 😄 I modified the test to leave the logger some room to wriggle.

acatangiu · 2019-05-10T11:22:19Z

sys_util/src/signal.rs

+/// }
+/// ```
+///
+pub unsafe fn register_vcpu_signal_handler(


acatangiu · 2019-05-10T11:34:27Z

vmm/src/signal_handler.rs

-        return Err(io::Error::last_os_error());
+    // Other signals which might do async unsafe things incompatible with the rest of this
+    // function are blocked due to the sa_mask used when registering the signal handler.
+    let syscall = unsafe { *(info as *const i32).offset(SI_OFF_SYSCALL) as usize };


Hmm... magic? Is this the standard way of getting the signum?

The signum is fortunately always at the beginning of the sigaction struct - the syscall number is trickier to get to because it's embedded in one of those unions in the struct. I found it easier to just navigate to its offset than to unpack the whole thing, like I (incompletely) tried previously.

acatangiu · 2019-05-10T11:35:34Z

vmm/src/signal_handler.rs

+    // function are blocked due to the sa_mask used when registering the signal handler.
+    let syscall = unsafe { *(info as *const i32).offset(SI_OFF_SYSCALL) as usize };
+    METRICS.seccomp.num_faults.inc();
+    error!(


Can we log stuff in signal handler?

I think so. write is safe, and the formatting doesn't use any of the unsafe sprintf family.

acatangiu · 2019-05-10T11:46:49Z

vmm/src/signal_handler.rs

@@ -144,8 +170,8 @@ mod tests {
        assert!(filter.apply().is_ok());
        assert_eq!(METRICS.seccomp.num_faults.count(), 0);

-        // Calls the blacklisted SYS_getpid.
-        let _pid = process::id();
+        // Call the blacklisted `SYS_mkdir`.


Leaving this clarification here:

whitelist kill and getpid (for the kill to go through).

is done just in a particular unit-test, not whitelisted in general.

This test fails intermittently when linked against glibc. Glibc's getpid() caches the result, so it doesn't always do the syscall. To avoid GNU-related complications, the test now forces a different syscall (mkdir) - as getpid will be needed in a later commit. Signed-off-by: Alexandra Iordache <aghecen@amazon.com>

This step is in preparation to a refactoring that moves generic signal handling utilities to the sys_util crate, leaving only Firecracker specifics (read: custom signal handlers themselves) in vmm. Signed-off-by: Alexandra Iordache <aghecen@amazon.com>

* added a single function in vmm that installs all relevant signal handlers; * left signal handler functions in vmm (currently only for SIGSYS); * moved signal handling logic to sys_util; * split sys_util's register_signal_handler into 2 separate functions, one to be called exclusively for vCPUs and a second general purpose one. The vCPU-specific one doen't change the signal mask and alters the signal number, forcing it between SIGRTMIN and SIGRTMAX. The general purpose one is setup_sigsys_handler renamed, and will morph into a generic one in a following commit. Signed-off-by: Alexandra Iordache <aghecen@amazon.com>

Renamed register_sigsys_handler to register_signal_handler, enabling the installation of custom signal handlers for any signal. Signed-off-by: Alexandra Iordache <aghecen@amazon.com>

Log a message and exit with a specific exit code upon intercepting SIGBUS/SIGSEGV. Signed-off-by: Alexandra Iordache <aghecen@amazon.com>

Signed-off-by: Alexandra Iordache <aghecen@amazon.com>

The license checker now accepts files licensed in 2018 and 2019. Signed-off-by: Alexandra Iordache <aghecen@amazon.com>

The test checks that Firecracker logs the appropriate mesage when intercepting the signal. Exit code is not checked because, as the jailer clones into a new pid namespace, it's no longer in the test process' tree and its pid cannot be waited on. Signed-off-by: Alexandra Iordache <aghecen@amazon.com>

Signed-off-by: Alexandra Iordache <aghecen@amazon.com>

alxiord self-assigned this Apr 23, 2019

serban300 self-requested a review April 25, 2019 08:45

acatangiu reviewed Apr 30, 2019

View reviewed changes

alxiord force-pushed the segv_bus branch from e7af44b to 76d668e Compare May 7, 2019 14:59

alxiord changed the title ~~[RFC] Signal handler for SIGBUS, SIGSEGV~~ Signal handler for SIGBUS, SIGSEGV May 7, 2019

alxiord force-pushed the segv_bus branch 6 times, most recently from 747fd20 to eaf30fd Compare May 9, 2019 15:36

alxiord commented May 9, 2019

View reviewed changes

alxiord force-pushed the segv_bus branch 3 times, most recently from 634a0c5 to 140fc56 Compare May 10, 2019 06:37

acatangiu reviewed May 10, 2019

View reviewed changes

Alexandra Iordache added 8 commits May 10, 2019 16:16

sys_util: generic register_signal_handler

65dc516

Renamed register_sigsys_handler to register_signal_handler, enabling the installation of custom signal handlers for any signal. Signed-off-by: Alexandra Iordache <aghecen@amazon.com>

vmm: handle SIGBUS & SIGSEGV

f533bf1

Log a message and exit with a specific exit code upon intercepting SIGBUS/SIGSEGV. Signed-off-by: Alexandra Iordache <aghecen@amazon.com>

documentation & doctests for signal handling utils

95f5d7e

Signed-off-by: Alexandra Iordache <aghecen@amazon.com>

tests: update licenses test

1f722e2

The license checker now accepts files licensed in 2018 and 2019. Signed-off-by: Alexandra Iordache <aghecen@amazon.com>

changelog: add note on new signal handler

333869b

Signed-off-by: Alexandra Iordache <aghecen@amazon.com>

alxiord force-pushed the segv_bus branch from 140fc56 to 333869b Compare May 10, 2019 13:16

acatangiu approved these changes May 10, 2019

View reviewed changes

serban300 approved these changes May 13, 2019

View reviewed changes

alxiord merged commit 76594aa into firecracker-microvm:master May 13, 2019

alxiord mentioned this pull request May 13, 2019

Seccomp violation when Firecracker gets SIGSEGV/SIGBUS #1064

Closed

alxiord deleted the segv_bus branch May 13, 2019 12:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Signal handler for SIGBUS, SIGSEGV #1070

Signal handler for SIGBUS, SIGSEGV #1070

alxiord commented Apr 23, 2019 •

edited

Loading

serban300 commented Apr 25, 2019

serban300 commented Apr 25, 2019

acatangiu Apr 30, 2019

nbsdx commented May 1, 2019

alxiord commented May 6, 2019

nbsdx commented May 6, 2019

alxiord commented May 7, 2019 •

edited

Loading

alxiord May 9, 2019

acatangiu May 10, 2019

acatangiu left a comment

acatangiu May 10, 2019

alxiord May 10, 2019

acatangiu May 10, 2019

acatangiu May 10, 2019 •

edited

Loading

alxiord May 10, 2019

acatangiu May 10, 2019

alxiord May 10, 2019

acatangiu May 10, 2019

		@@ -0,0 +1,230 @@
		// Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved.

Signal handler for SIGBUS, SIGSEGV #1070

Signal handler for SIGBUS, SIGSEGV #1070

Conversation

alxiord commented Apr 23, 2019 • edited Loading

serban300 commented Apr 25, 2019

serban300 commented Apr 25, 2019

Choose a reason for hiding this comment

nbsdx commented May 1, 2019

alxiord commented May 6, 2019

nbsdx commented May 6, 2019

alxiord commented May 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acatangiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acatangiu May 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alxiord commented Apr 23, 2019 •

edited

Loading

alxiord commented May 7, 2019 •

edited

Loading

acatangiu May 10, 2019 •

edited

Loading