Backend implementation #7

alindima · 2021-07-29T14:28:29Z

This PR implements the Rust IR and the logic for compiling it into BPF code.

In order for the design to be fully implemented, we need to add the JSON frontend, which will come in a future PR.

Signed-off-by: alindima <alindima@amazon.com>

andreeaflorescu

Did an initial pass. I still need to take a closer look at the tests.

src/backend/mod.rs

src/lib.rs

andreeaflorescu · 2021-08-04T08:55:13Z

src/backend/mod.rs

+
+/// Comparison to perform when matching a condition.
+#[derive(Clone, Debug, PartialEq)]
+pub enum SeccompCmpOp {


nit: not all names follow the same convention. Some names have Seccomp as a prefix, others do not. I would drop Seccomp from all of them.

The reasoning behind this is so that we can easily distinguish the types that make up the IR (intermediate representation). All such types have Seccomp* embedded in their name.

This will make for more readable code once we add the JSON frontend types, those being named like JsonRule, JsonFilter, etc.

src/backend/rule.rs

andreeaflorescu · 2021-08-04T09:30:41Z

src/backend/condition.rs

+    /// * `offset` - The given jump offset to the start of the next rule.
+    ///
+    fn into_eq_bpf(self, offset: u8) -> Vec<sock_filter> {
+        let (msb, lsb) = self.split_value();


Having you looked into de-duplicating the into_ functions? I wonder if we could reduce the duplicated code while still keep the readability.

I have, but I couldn't find a better way

andreeaflorescu · 2021-08-04T11:23:21Z

src/backend/filter.rs

+                bpf
+            })
+            .collect();
+        let chain_len: usize = chain.iter().map(std::vec::Vec::len).sum();


nit: Vec::len instead of std::vec::...

andreeaflorescu · 2021-08-04T11:25:39Z

src/backend/filter.rs

+        let mut built_syscall = Vec::with_capacity(chain_len + 2);
+        built_syscall.push(BPF_JUMP(
+            BPF_JMP | BPF_JEQ | BPF_K,
+            // Safe because linux system call numbers are nowhere near the u32::MAX value.


Why do we define them as i64 then?

because that's what libc expects for the syscall and that's what's used for the syscall numbers in rust's libc crate: https://docs.rs/libc/0.2.98/libc/constant.SYS_open.html.
I think it's better to have the cast here than duplicating it across countless system calls in user filters.

src/backend/bpf.rs

tests/integration_tests.rs

coverage_config_x86_64.json

This struct will be used to parametrise the filters with the target architecture they are compiled for. The compilation logic will embed the architecture check so that we make sure the BPF programs are not installed on different architecures (which may have different system call numbers). Signed-off-by: alindima <alindima@amazon.com>

Module containing constants, types and helper functions used in the BPF codegen. The TargetArch::get_audit_value uses constants from the BPF module for associating the TargetArch variant to the right audit value used in the BPF compilation. Signed-off-by: alindima <alindima@amazon.com>

Signed-off-by: alindima <alindima@amazon.com>

src/lib.rs

likebreath · 2021-08-11T07:16:29Z

@alindima Thank you for the great work. I finished a draft of integrating Cloud Hypervisor with your PR, and can confirm it works as expected with both SeccompAction Trap and Log.

Overall, I think the new interface (APIs and Types) are pretty good and cleaner. Admittedly it requires lots of changes due to the dramatic interface changes.

Only one comment/open-question: I wonder whether we should include some similar macros to make it easier (or more readable) to define complex SeccompCondition, for example the kvm ioctls rules in Cloud Hypervisor.

Signed-off-by: alindima <alindima@amazon.com>

Renamed the filter_action to match_action and the default_action to mismatch_action, to make them slightly more expressive and easier to understand. Signed-off-by: alindima <alindima@amazon.com>

Signed-off-by: alindima <alindima@amazon.com>

alindima · 2021-08-11T14:20:56Z

@alindima Thank you for the great work. I finished a draft of integrating Cloud Hypervisor with your PR, and can confirm it works as expected with both SeccompAction Trap and Log.

Thank you @likebreath for your work on integrating seccompiler in Cloud hypervisor, it's great to hear it works as expected!
I've taken a quick look over your draft PR and it looks good.

Overall, I think the new interface (APIs and Types) are pretty good and cleaner. Admittedly it requires lots of changes due to the dramatic interface changes.

Great to hear the interface has been indeed improved. Unfortunately it does require quite a lot of changes, but most of them are repetitive (and automatable with regex find&replace) and the one-time character of the changes I think make the effort worth it 😄

Only one comment/open-question: I wonder whether we should include some similar macros to make it easier (or more readable) to define complex SeccompCondition, for example the kvm ioctls rules in Cloud Hypervisor.

That is a good question. I've given the issue some thought. The two macros have a pretty small implementation and are quite an opinionated way of instantiating the objects. For example, other users of the library may not want to unwrap() the result of SeccompRule::new(vec![$($x),*]), like you are doing in your integration.

Given the above, I think it would be fine to keep those small macros in CH for now. What do you think?

Of course, we can revisit this choice later on if we come up with better abstractions that make the library easier to use for everybody.

likebreath · 2021-08-11T16:58:44Z

That is a good question. I've given the issue some thought. The two macros have a pretty small implementation and are quite an opinionated way of instantiating the objects. For example, other users of the library may not want to unwrap() the result of SeccompRule::new(vec![$($x),*]), like you are doing in your integration.

Right. That's something I wanted to improve hopefully at the time of landing the integration to CH (after landing your PR of course).

Given the above, I think it would be fine to keep those small macros in CH for now. What do you think?

This sounds good to me.

Of course, we can revisit this choice later on if we come up with better abstractions that make the library easier to use for everybody.

Agree. Improving abstractions over creating complex conditions definitely worth a revisit.

andreeaflorescu

There are still a few std::u32::max calls that should be replaced with u32::max.

andreeaflorescu · 2021-08-12T14:31:24Z

tests/integration_tests.rs

+fn validate_seccomp_filter(
+    rules: Vec<(i64, Vec<SeccompRule>)>,
+    validation_fn: fn(),
+    should_fail: Option<bool>,


nit: It is a bit hard to understand how to use this parameter, specifically what is the meaning of None. We could define an enum in which we have 3 options (with some better names maybe):

enum Return { Errno(u32), NoErrno, NA, }

alindima added 3 commits July 15, 2021 13:02

restrict builds for only little endian targets

01241e7

Signed-off-by: alindima <alindima@amazon.com>

minor documentation updates

2456b99

Signed-off-by: alindima <alindima@amazon.com>

Add backend module

27e9698

Signed-off-by: alindima <alindima@amazon.com>

alindima self-assigned this Jul 29, 2021

alindima force-pushed the master branch from 6e64a24 to b14d5f2 Compare July 29, 2021 14:39

alindima mentioned this pull request Jul 29, 2021

Bump rust-vmm-ci from 24d66cd to 7693628 #6

Closed

alindima requested a review from andreeaflorescu July 29, 2021 14:43

sboeuf mentioned this pull request Aug 3, 2021

Move to rust-vmm/seccompiler crate cloud-hypervisor/cloud-hypervisor#2929

Closed

andreeaflorescu reviewed Aug 4, 2021

View reviewed changes

coverage_config_x86_64.json Outdated Show resolved Hide resolved

alindima changed the base branch from master to main August 9, 2021 09:33

alindima added 10 commits August 9, 2021 13:18

Add CmpOp, ArgLen and Action data types

726ec42

Signed-off-by: alindima <alindima@amazon.com>

Add SeccompCondition type and compilation logic

6defee2

Signed-off-by: alindima <alindima@amazon.com>

Add SeccompRule type and compilation logic

f5f0cc0

Signed-off-by: alindima <alindima@amazon.com>

Add SeccompFilter type and compilation logic

a4ef820

Signed-off-by: alindima <alindima@amazon.com>

Add rust integration tests

b0e4254

Signed-off-by: alindima <alindima@amazon.com>

Add integration test for invalid architecture

5eaa9c4

Signed-off-by: alindima <alindima@amazon.com>

Add recommendation for BPF JIT compiler

1180260

Signed-off-by: alindima <alindima@amazon.com>

Bump rust-vmm-ci to a00c7d2

4b4bd5c

Signed-off-by: alindima <alindima@amazon.com>

alindima force-pushed the master branch from b14d5f2 to 3b4ae90 Compare August 9, 2021 12:27

alindima mentioned this pull request Aug 10, 2021

Bump rust-vmm-ci from 24d66cd to a00c7d2 #8

Closed

andreeaflorescu reviewed Aug 10, 2021

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

likebreath previously approved these changes Aug 11, 2021

View reviewed changes

alindima added 4 commits August 11, 2021 17:05

lib: add apply_filter function

14afd24

Signed-off-by: alindima <alindima@amazon.com>

update coverage

c87a947

Signed-off-by: alindima <alindima@amazon.com>

update arch image with white background

fb00427

Signed-off-by: alindima <alindima@amazon.com>

improve documentation

3c5df8c

Signed-off-by: alindima <alindima@amazon.com>

alindima added 4 commits August 11, 2021 17:06

fix minor clippy warnings

80db623

Signed-off-by: alindima <alindima@amazon.com>

Add copyright notices

ce299a9

Signed-off-by: alindima <alindima@amazon.com>

rename filter and default actions

4562c5b

Renamed the filter_action to match_action and the default_action to mismatch_action, to make them slightly more expressive and easier to understand. Signed-off-by: alindima <alindima@amazon.com>

lowercase BPF_STMT & BPF_JUMP

399fe4e

Signed-off-by: alindima <alindima@amazon.com>

alindima dismissed likebreath’s stale review via 399fe4e August 11, 2021 14:07

alindima force-pushed the master branch from 3b4ae90 to 399fe4e Compare August 11, 2021 14:07

andreeaflorescu approved these changes Aug 12, 2021

View reviewed changes

sboeuf approved these changes Aug 13, 2021

View reviewed changes

alindima merged commit da5788d into rust-vmm:main Aug 13, 2021

andreeaflorescu mentioned this pull request Aug 25, 2021

Implement json frontend #9

Merged

alindima deleted the master branch October 7, 2021 06:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backend implementation #7

Backend implementation #7

alindima commented Jul 29, 2021 •

edited

Loading

andreeaflorescu left a comment

andreeaflorescu Aug 4, 2021

alindima Aug 9, 2021

andreeaflorescu Aug 4, 2021

alindima Aug 9, 2021

andreeaflorescu Aug 4, 2021

alindima Aug 9, 2021

andreeaflorescu Aug 4, 2021

alindima Aug 9, 2021

likebreath commented Aug 11, 2021

alindima commented Aug 11, 2021

likebreath commented Aug 11, 2021

andreeaflorescu left a comment

andreeaflorescu Aug 12, 2021

Backend implementation #7

Backend implementation #7

Conversation

alindima commented Jul 29, 2021 • edited Loading

andreeaflorescu 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

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

likebreath commented Aug 11, 2021

alindima commented Aug 11, 2021

likebreath commented Aug 11, 2021

andreeaflorescu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alindima commented Jul 29, 2021 •

edited

Loading