Add support for seccomp thread sync feature by boustrophedon · Pull Request #58 · rust-vmm/seccompiler

boustrophedon · 2023-09-06T02:20:15Z

Summary of the PR

Adds public function seccompiler::apply_filter_all_threads
Moves the body of apply_filter into apply_filter_with_flags
Use seccomp syscall instead of prctl to apply seccomp filters

Resolves #57

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

All commits in this PR are signed (with git commit -s), and the commit
message has max 60 characters for the summary and max 75 characters for each
description line.
All added/changed functionality has a corresponding unit/integration
test.
All added/changed public-facing functionality has entries in the "Upcoming
Release" section of CHANGELOG.md (if no such section exists, please create one).
Any newly added unsafe code is properly documented.

src/backend/flags.rs

boustrophedon · 2023-09-06T02:23:16Z

tests/multi_thread.rs

@@ -0,0 +1,80 @@
+#![allow(clippy::undocumented_unsafe_blocks)]


This test was adapted from my tsync test in extrasafe. https://github.com/boustrophedon/extrasafe/blob/master/tests/thread_multi.rs

boustrophedon · 2023-09-06T04:01:52Z

Looks like musl doesn't have the linux/seccomp.h SECCOMP_SET_MODE_FILTER constant defined in libc? Strange though because I've build libseccomp itself with musl before.

There's also lint and formatting errors that I need to fix.

boustrophedon · 2023-09-06T04:49:59Z

It looks like everything except coverage and musl builds are passing now. I'm not sure what the best resolution is for the missing constant in musl libc.

rbradford · 2023-09-06T17:27:23Z

It looks like everything except coverage and musl builds are passing now. I'm not sure what the best resolution is for the missing constant in musl libc.

Open a PR against rust-lang/libc since this looks like it wasn't included when then gnu/musl split happened.

boustrophedon · 2023-09-06T21:37:19Z

rust-lang/libc#3342

Did some digging, seems like it was just missed at some point.

In the meantime, I'll update this diff to use libc::SECCOMP_FILTER_FLAG_TSYNC inside flags.rs which I had missed before but does anyone have an issue with landing this just using a locally-defined const SECCOMP_SET_MODE_FILTER: libc::c_int = 1 inside seccompiler until the libc PR gets merged? It seems like that's happening a lot in bpf.rs already so maybe I can do a follow up PR after the libc PR gets merged to clean up all the constants.

alindima · 2023-09-07T06:12:05Z

rust-lang/libc#3342

Did some digging, seems like it was just missed at some point.

In the meantime, I'll update this diff to use libc::SECCOMP_FILTER_FLAG_TSYNC inside flags.rs which I had missed before but does anyone have an issue with landing this just using a locally-defined const SECCOMP_SET_MODE_FILTER: libc::c_int = 1 inside seccompiler until the libc PR gets merged? It seems like that's happening a lot in bpf.rs already so maybe I can do a follow up PR after the libc PR gets merged to clean up all the constants.

Yes, this is fine until the PR is merged in rust/libc 👍🏻 Have you checked that it's the same value on x86 and arm as well?

src/backend/flags.rs

src/lib.rs

CHANGELOG.md

src/lib.rs

boustrophedon · 2023-09-07T06:35:53Z

Thanks for the review! I just pushed my PR to libc and am about to go to bed but I'll start working on the fixes tomorrow!

src/lib.rs

tests/multi_thread.rs

alindima

Thanks for the contribution! Glad to hear you're using seccompiler 😄

tests/multi_thread.rs

boustrophedon · 2023-09-07T22:42:24Z

I've made all the requested changes I think.

Regarding the value of SECCOMP_SET_MODE_FILTER I'm pretty sure it's the same on all platforms. It's only defined once in include/uapi/linux/seccomp.h in the linux repo as far as I can see.

One thing I just noticed is that the manpage for seccomp says that the flags parameter is an unsigned int, but the definition for the flags in <linux/seccomp.h> is (1UL << 0) i.e. an unsigned long. I've changed the parameter to libc::c_ulong since the TSYNC flag constant is a ulong and my PR also uses ulong.

The only thing I'm not sure about is the source impl for ThreadSync - currently I left it as None since i64 doesn't impl Error but maybe there's something better I could do?

src/lib.rs

CHANGELOG.md

src/lib.rs

petreeftime · 2023-09-08T10:06:00Z

src/lib.rs

+/// * `flags` - A u64 representing a bitset of seccomp's flags parameter.
+///
+/// [`BpfProgram`]: type.BpfProgram.html
+fn apply_filter_with_flags(bpf_filter: BpfProgramRef, flags: libc::c_ulong) -> Result<()> {


Ideally flags should be similar to something like OFlags in nix: https://docs.rs/nix/latest/nix/fcntl/struct.OFlag.html, it's slightly more robust, but if apply_filter_with_flags remain just for internal purposes (ie. flags is not part of the public API), then leaving it like this for now is fine.

This was the initial approach in the PR but since different flags for the seccomp syscall results in different return types, I suggested to keep this function for internal use only, to keep things simple and hard to misuse

boustrophedon · 2023-09-08T20:07:48Z

Thanks for catching everything that I missed!

Regarding the clippy failure, I can make the change if you want but I personally think clippy's recommendation is not nearly as readable as just writing "rc > 0". If I'm looking at a single arm, "Ordering::Greater" doesn't tell me which is greater - I have to refer back to the top of the match statement. x < y is something everyone learned how to read in elementary school.

alindima

Thanks

tests/multi_thread.rs

alindima · 2023-09-11T06:45:25Z

I agree that clippy's suggestion is less readable.
You can use this line to disable it on the function definition: #[allow(clippy::comparison_chain)]

- Adds public functions `seccompiler::apply_filter_all_threads` and private `apply_filter_with_flags` - Moves the body of apply_filter into apply_filter_with_flags - Uses seccomp call directly in apply_filter, so new Error variant is added. - Error variant also added for TSYNC failures Resolves rust-vmm#57 Signed-off-by: Harry Stern <harry@harrystern.net>

boustrophedon · 2023-09-12T02:48:18Z

Looks like everything is passing now, thanks everyone for the reviews!

alindima

LGTM

boustrophedon requested review from alindima and petreeftime as code owners September 6, 2023 02:20

boustrophedon commented Sep 6, 2023

View reviewed changes

src/backend/flags.rs Outdated Show resolved Hide resolved

boustrophedon commented Sep 6, 2023

View reviewed changes

boustrophedon force-pushed the tsync branch 2 times, most recently from f2ccfc3 to c63ac88 Compare September 6, 2023 04:32

boustrophedon force-pushed the tsync branch from c63ac88 to 762bb6a Compare September 6, 2023 22:38