Add full arm64/aarch64 support #7

AaronDewes · 2023-04-09T09:54:44Z

This adds full arm64 support.

I implemented this as a feature flag, so you can easily test if it compiles by just passing --all-features to cargo.

ARM64 is missing a few syscalls that have been deprecated (I could not find details about this, but got this information from https://chromium.googlesource.com/linux-syscall-support/+/2f724fced658cd6182096c615efdf3767b7c57fe%5E%21/), I removed them from the code.

I also ran cargo fmt and cargo clippy to fix issues. This changed formatting of unchanged code because the default rules changed since the last commit. Please let me know if I should revert that.

boustrophedon · 2023-04-09T14:38:27Z

Hi!

Thanks for the PR - I'll try to take a look at it in the next few days. I haven't really touched this project much since starting a new job last year so it might take me a bit to figure everything out.

boustrophedon

Sorry for taking so long to review!

In short:

I'd prefer the PR without any of the formatting if possible
I don't think the current approach is the right one - it would be too easy for the different arches to get out of sync with each other
If you want to try the approach I outlined in one of the comments let me know if you have any questions!

Thanks again for taking the time to make the PR.

boustrophedon · 2023-04-21T01:03:06Z

.vscode/settings.json

@@ -0,0 +1,3 @@
+{


I would prefer not to have this checked in - is it not possible to configure vscode in your home directory or something?

boustrophedon · 2023-04-21T01:08:10Z

examples/time.rs

-                .allow_gettime()
-        ).unwrap()
-        .apply_to_current_thread().unwrap();
+        .enable(Time::nothing().allow_gettime())


I think it was the stuff like this that made me not want to format. This could be resolved by putting the RuleSet instance in its own variable I guess.

Also putting unwrap() at the same indentation as the "real" functions being called is a bit annoying as well.

boustrophedon · 2023-04-21T01:19:41Z

src/aarch64.rs

@@ -0,0 +1,210 @@
+use libseccomp::*;


I don't think having two copies of the source code is the way to implement multi-arch support, and I don't think I can accept this PR as-is.

I think there's probably a way to make Rule generic over syscall types, but this is hampered by the fact that Sysno is an enum in an external crate.

Probably doing something like

pub struct X86_64Sysno { pub sysno: syscalls::x86_64::Sysno } pub struct AArchSysno { ... } trait ESSysno { /* maybe a get_sysno() here or something */} impl ESSysno for X86_64Sysno {} impl ESSysno for AArch64Sysno {} pub struct Rule<S: ESSysno>

would work. I'm not sure whether sealing the trait would be the right thing to do or not, would have to think about it.

AaronDewes · 2024-03-17T11:59:31Z

@boustrophedon Cann you have a look at this reimplementation?

The actual changes are just 15 files changed, 140 insertions(+), 33 deletions(-).

This now implements both RISC-V and arm64 with a compile-time check that disables/enables certain syscalls for certain architectures.
extrasafe::syscalls now re-exports syscalls for the current architecture.

Features are used so we can cargo check for all supported architectures.

In addition, I added the readlinkat syscall at a place where previously, only readlink was enabled.

This reverts commit 9de68ea.

boustrophedon · 2024-03-17T19:17:26Z

build.rs

@@ -0,0 +1,12 @@
+fn main() {
+    // prefer method A if both method A and B are selected
+    if cfg!(feature = "__aarch64") || cfg!(target_arch = "aarch64") {


I don't understand why we need separate features for arm and riscv in addition to the existing target_arch config.

I added these for easier compile-checking. This means I can just do cargo check --feature=__aarch64 to check if the code would compile on aarch64 without installing a target toolchain.

boustrophedon · 2024-03-17T19:18:18Z

Makefile

@@ -1,18 +1,26 @@
 build:
-	cargo build --all-features


It seems like these got changed because of the new "enabled_arch" below.

boustrophedon · 2024-03-17T19:20:24Z

Hi! Thanks for working on this. I think it looks better than the original PR but the underlying issue here is twofold:

One, seccompiler doesn't support riscv. Two, I see that you added support for cross-compiling, but I'm not comfortable with releasing the code without it actually running in CI.

See issue #35 if you want to discuss working on it there

AaronDewes · 2024-03-17T20:07:15Z

One, seccompiler doesn't support riscv

Right, I didn't check for that because it was not used when I made my initial implementation.

Two, I see that you added support for cross-compiling, but I'm not comfortable with releasing the code without it actually running in CI.

Would qemu be okay for that?

AaronDewes · 2024-03-17T20:10:03Z

Another thing I could do would be to remove RISC-V and set up a aarch64 VM you can use as actions runner (and then deal with RISC-V later).

If you want, I can also remove the extra stuff from build.rs and the feature flags to simplify this, but I think being able to locally test with just a simple cargo check without much else is easier.

boustrophedon · 2024-03-17T20:51:25Z

If you can get qemu running in github actions in a reasonable amount of time (I mean CI runtime) that would be awesome. If not we can try exploring something like buildkite.

boustrophedon · 2024-03-18T19:01:39Z

Let me know when you're ready for review but I also noticed that someone has a PR open in seccompiler for riscv.

AaronDewes · 2024-03-18T19:03:24Z

Let me know when you're ready for review but I also noticed that someone has a PR open in seccompiler for riscv.

The code itself is ready for review, but CI implementation is not finished yet.

I also saw that PR, but I think just merging arm64 is easier and then dealing with RISC-V later.

boustrophedon · 2024-03-18T19:20:17Z

The code itself is ready for review, but CI implementation is not finished yet.

Okay. I think if we are going to plan to support riscv though it might be easier to try out buildkite or similar. Looking at seccompiler's setup it seems a lot simpler than having to deal with running qemu via Github Actions' yaml.

We could have the regular x86_64 tests and lints etc run in github actions and then an extra step to just run the tests with arm and riscv via buildkite.

I'm currently working on a user namespace-based isolation api for extrasafe but I can work on the buildkite stuff probably later in the week if you want to continue on the qemu side.

I also saw that PR, but I think just merging arm64 is easier and then dealing with RISC-V later.

agreed.

boustrophedon · 2024-04-04T17:59:05Z

Is this ready for review? I just released 0.5.0 with the user namespaces stuff I mentioned so you'll have to rebase

AaronDewes · 2024-04-05T09:10:00Z

I merged the new branch, but there are still some fixes needed for the CI.

boustrophedon · 2024-04-05T23:02:39Z

There's no rush or anything, I just wasn't sure if you were blocked on me. Thanks again for working on this!

boustrophedon reviewed Apr 21, 2023

View reviewed changes

Kijewski mentioned this pull request Jun 15, 2023

Implement for aarch64 #13

Closed

AaronDewes added 4 commits March 17, 2024 11:26

cargo fmt, fix some warnings

9de68ea

Implement multiarch-support

38de2f9

Fix Makefile

6521a6c

Ensure builds work

379f4cf

AaronDewes force-pushed the master branch from 50d8286 to 379f4cf Compare March 17, 2024 11:41

Revert "cargo fmt, fix some warnings"

f1318b1

This reverts commit 9de68ea.

boustrophedon reviewed Mar 17, 2024

View reviewed changes

AaronDewes added 10 commits March 18, 2024 06:35

Simplify this, drop RISC-V, use Qemu in CI

b2363c8

Remove RustRover files

629a935

Revert tests change

67c0db7

Completely revert makefile

8e262e0

Try to improve OpenSSL handling

b9dec5e

sudo

ec4457d

TLS vendored

20da728

musl fix

716e833

Some slight improvements

a195ce0

Last build fix

68e58c5

Check if this works

261ccaa

AaronDewes added 4 commits March 20, 2024 20:17

Try lld

b780975

Update build-test.yaml

41b4304

Try cross-rs

be61f52

Space

d53fb3e

AaronDewes added 5 commits April 5, 2024 10:56

Merge branch 'master' of https://github.com/boustrophedon/extrasafe

2980675

Cross config

9941e00

Improve cross config

c0516a1

Fix isolate_sys on arm64

031ab56

Another musl fixup

aa9112f

Another update

50c84b3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add full arm64/aarch64 support #7

Add full arm64/aarch64 support #7

AaronDewes commented Apr 9, 2023

boustrophedon commented Apr 9, 2023

boustrophedon left a comment

boustrophedon Apr 21, 2023

boustrophedon Apr 21, 2023

boustrophedon Apr 21, 2023

boustrophedon Apr 21, 2023

AaronDewes commented Mar 17, 2024 •

edited

Loading

boustrophedon Mar 17, 2024

AaronDewes Mar 17, 2024

boustrophedon Mar 17, 2024

boustrophedon commented Mar 17, 2024

AaronDewes commented Mar 17, 2024

AaronDewes commented Mar 17, 2024

boustrophedon commented Mar 17, 2024

boustrophedon commented Mar 18, 2024

AaronDewes commented Mar 18, 2024

boustrophedon commented Mar 18, 2024 •

edited

Loading

boustrophedon commented Apr 4, 2024

AaronDewes commented Apr 5, 2024

boustrophedon commented Apr 5, 2024

Add full arm64/aarch64 support #7

Are you sure you want to change the base?

Add full arm64/aarch64 support #7

Conversation

AaronDewes commented Apr 9, 2023

boustrophedon commented Apr 9, 2023

boustrophedon left a comment

Choose a reason for hiding this comment

boustrophedon Apr 21, 2023

Choose a reason for hiding this comment

boustrophedon Apr 21, 2023

Choose a reason for hiding this comment

boustrophedon Apr 21, 2023

Choose a reason for hiding this comment

boustrophedon Apr 21, 2023

Choose a reason for hiding this comment

AaronDewes commented Mar 17, 2024 • edited Loading

boustrophedon Mar 17, 2024

Choose a reason for hiding this comment

AaronDewes Mar 17, 2024

Choose a reason for hiding this comment

boustrophedon Mar 17, 2024

Choose a reason for hiding this comment

boustrophedon commented Mar 17, 2024

AaronDewes commented Mar 17, 2024

AaronDewes commented Mar 17, 2024

boustrophedon commented Mar 17, 2024

boustrophedon commented Mar 18, 2024

AaronDewes commented Mar 18, 2024

boustrophedon commented Mar 18, 2024 • edited Loading

boustrophedon commented Apr 4, 2024

AaronDewes commented Apr 5, 2024

boustrophedon commented Apr 5, 2024

AaronDewes commented Mar 17, 2024 •

edited

Loading

boustrophedon commented Mar 18, 2024 •

edited

Loading