Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.11] add support for seccomp flags #57

Draft
wants to merge 9 commits into
base: rhaos-4.11
Choose a base branch
from

Conversation

@kolyshkin kolyshkin marked this pull request as draft August 29, 2022 23:58
@kolyshkin

This comment was marked as outdated.

kolyshkin and others added 3 commits August 29, 2022 17:03
This is to include Linux seccomp flags.

Identical to upstream commit c152e83.

Signed-off-by: Kir Kolyshkin <[email protected]>
List of seccomp flags defined in runtime-spec:
* SECCOMP_FILTER_FLAG_TSYNC
* SECCOMP_FILTER_FLAG_LOG
* SECCOMP_FILTER_FLAG_SPEC_ALLOW

Note that runc does not apply SECCOMP_FILTER_FLAG_TSYNC. It does not
make sense to apply the seccomp filter on only one thread; other threads
will be terminated after exec anyway.

See similar commit in crun:
containers/crun@fefabff

Note that SECCOMP_FILTER_FLAG_WAIT_KILLABLE_RECV (introduced by
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?id=c2aa2dfef243
in Linux 5.19-rc1) is not added yet because Linux 5.19 is not released
yet.

Signed-off-by: Alban Crequy <[email protected]>
(cherry picked from commit 58ea21d)
Signed-off-by: Kir Kolyshkin <[email protected]>
Commit 58ea21d added support for seccomp flags such as
SPEC_ALLOW, but it does not work as expected, because since commit
7a8d716 we do not use libseccomp-golang's Load(), but
handle flags separately in patchbfp.

This fixes setting SPEC_ALLOW flag.

Add a comment to not forget to amend filterFlags when adding new flags.

Signed-off-by: Kir Kolyshkin <[email protected]>
(cherry picked from commit c7dc8b1)
Signed-off-by: Kir Kolyshkin <[email protected]>
@haircommander
Copy link
Collaborator

LGTM, once the upstream PR is approved

Add a debug print of seccomp flags value, so the test can check
those (without using something like strace, that is).

Amend the flags setting test with the numeric values expected, and the
logic to check those.

Signed-off-by: Kir Kolyshkin <[email protected]>
(cherry picked from commit 26dc55e)
Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin
Copy link
Collaborator Author

Refreshed, added backport of opencontainers/runc#3581

1. This valid warning is reported by shellcheck v0.8.0:

	In tests/integration/helpers.bash line 38:
	KERNEL_MINOR="${KERNEL_VERSION#$KERNEL_MAJOR.}"
				       ^-----------^ SC2295 (info): Expansions inside ${..} need to be quoted separately, otherwise they match as patterns.

	Did you mean:
	KERNEL_MINOR="${KERNEL_VERSION#"$KERNEL_MAJOR".}"

Fix this.

2. These (invalid) warnings are also reported by the new version:

	In tests/integration/events.bats line 13:
	@test "events --stats" {
	^-- SC2030 (info): Modification of status is local (to subshell caused by @BATS test).

	In tests/integration/events.bats line 41:
		[ "$status" -eq 0 ]
		   ^-----^ SC2031 (info): status was modified in a subshell. That change might be lost.

Basically, this is happening because shellcheck do not really track
the call tree and/or local variables. This is a known (and reported)
deficiency, and the alternative to disabling these warnings is moving
the code around, which is worse due to more changes in git history.

So we have to silence/disable these.

3. Update shellcheck to 0.8.0.

Signed-off-by: Kir Kolyshkin <[email protected]>
(cherry picked from commit be00ae0)
Signed-off-by: Kir Kolyshkin <[email protected]>
(cherry picked from commit 631343689d08dd7d4d4ba79027af9a1b8e93184f)
Signed-off-by: Kir Kolyshkin <[email protected]>
Fix a few copy-paste errors.

Fixes: 520702d
Signed-off-by: Kir Kolyshkin <[email protected]>
(cherry picked from commit e45f75ff654ec51dad8c71c7cd2b0dd2220c31bd)
Signed-off-by: Kir Kolyshkin <[email protected]>
Amend runc features to print seccomp flags. Two set of flags are added:
 * known flags are those that this version of runc is aware of;
 * supported flags are those that can be set; normally, this is the same
   set as known flags, but due to older version of kernel and/or
   libseccomp, some known flags might be unsupported.

This commit also consolidates three different switch statements dealing
with flags into one, in func setFlag. A note is added to this function
telling what else to look for when adding new flags.

Unfortunately, it also adds a list of known flags, that should be
kept in sync with the switch statement.

Signed-off-by: Kir Kolyshkin <[email protected]>
(cherry picked from commit cb15546f50c04f375d30bde87be77a8fd3b73e72)
Signed-off-by: Kir Kolyshkin <[email protected]>
If no seccomps flags are set in OCI runtime spec (not even the empty
set), set SPEC_ALLOW as the default (if it's supported).

Otherwise, use the flags as they are set (that includes no flags for
empty seccomp.Flags array).

This mimics the crun behavior, and makes runc seccomp performance on par
with crun.

Signed-off-by: Kir Kolyshkin <[email protected]>
(cherry picked from commit c162ecc3a1dc314ae78797c83b3adac7bb6f0374)
Signed-off-by: Kir Kolyshkin <[email protected]>
This test (initially added by commit 58ea21d and later amended in
commit 26dc55e) currently has two major deficiencies:

1. All possible flag combinations, and their respective numeric values,
   have to be explicitly listed. Currently we support 3 flags, so
   there is only 2^3 - 1 = 7 combinations, but adding more flags will
   become increasingly difficult (for example, 5 flags will result in
   31 combinations).

2. The test requires kernel 4.17 (for SECCOMP_FILTER_FLAG_SPEC_ALLOW),
   and not doing any tests when running on an older kernel. This, too,
   will make it more difficult to add extra flags in the future.

Both issues can be solved by using runc features which now prints all
known and supported runc flags. We still have to hardcode the numeric
values of all flags, but most of the other work is coded now.

In particular:

 * The test only uses supported flags, meaning it can be used with
   older kernels, removing the limitation (2) above.

 * The test calculates the powerset (all possible combinations) of
   flags and their numeric values. This makes it easier to add more
   flags, removing the limitation (1) above.

 * The test will fail (in flags_value) if any new flags will be added
   to runc but the test itself is not amended.

Signed-off-by: Kir Kolyshkin <[email protected]>
(cherry picked from commit c7f672428d810c0428b53d76903d0fdc4f6f6c9c)
Signed-off-by: Kir Kolyshkin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants