rust-vmm / seccompiler

Provides easy-to-use Linux seccomp-bpf jailing.
https://crates.io/crates/seccompiler
Apache License 2.0
77 stars 11 forks source link

Add support for seccomp thread sync feature #58

Closed boustrophedon closed 1 year ago

boustrophedon commented 1 year ago

Summary of the PR

Resolves #57

Requirements

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

boustrophedon commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

https://github.com/rust-lang/libc/issues/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 commented 1 year ago

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?

boustrophedon commented 1 year ago

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!

boustrophedon commented 1 year ago

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?

boustrophedon commented 1 year ago

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 commented 1 year ago

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

boustrophedon commented 1 year ago

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