seccomp: add support for flags #3390

alban · 2022-02-22T15:33:19Z

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.

Tests:

$ make localintegration
...
ok 103 runc run [seccomp] (SECCOMP_FILTER_FLAG_*)

cc @rata @saschagrunert

rata

I'm no maintainer here, but this LGTM. Just a minor comment on the integration test

tests/integration/seccomp.bats

kolyshkin

I didn't add support for SECCOMP_FILTER_FLAG_SPEC_ALLOW (#2430) because libseccomp-golang does not support it yet.

It does; calling filter.SetSSB(true) will enable SECCOMP_FILTER_FLAG_SPEC_ALLOW.

kolyshkin · 2022-02-22T20:57:03Z

libcontainer/specconv/spec_linux.go

+ switch flag {
+ case "SECCOMP_FILTER_FLAG_LOG":


It would be nice to not have this set of flags handled in two separate places. Perhaps we can add some common part to libcontainer/seccomp?

While at it, adding SCMP_FLTATR_CTL_OPTIMIZE would be awesome

Right now we have four supported flags following the same naming convention supported by the SECCOMP_SET_MODE_FILTER operation.

SECCOMP_FILTER_FLAG_LOG
SECCOMP_FILTER_FLAG_NEW_LISTENER
SECCOMP_FILTER_FLAG_SPEC_ALLOW
SECCOMP_FILTER_FLAG_TSYNC

How would SCMP_FLTATR_CTL_OPTIMIZE map to that?

While at it, adding SCMP_FLTATR_CTL_OPTIMIZE would be awesome

The set of flags listed in the runtime-spec is a subset of the flag supported by the Linux API in the seccomp syscall:

flags (array of strings, OPTIONAL) - list of flags to use with seccomp(2).

A valid list of constants is shown below.

SECCOMP_FILTER_FLAG_TSYNC
SECCOMP_FILTER_FLAG_LOG
SECCOMP_FILTER_FLAG_SPEC_ALLOW

SCMP_FLTATR_CTL_OPTIMIZE is part of libseccomp API but not part of the Linux API. It is not in the runtime-spec because container runtimes don't have to use libseccomp. Are you suggesting runc should support it even if it's not in the spec?

It would be nice to not have this set of flags handled in two separate places. Perhaps we can add some common part to libcontainer/seccomp?

Now that I have added the 2 additional flags, the code looks different, and I don't see how to add some common part to libcontainer/seccomp... any suggestions?

Re SCMP_FLTATR_CTL_OPTIMIZE.

Perhaps runc should just set it to 2 unconditionally. Also, this is orthogonal to this PR, so nevermind.

saschagrunert · 2022-02-23T09:54:07Z

libcontainer/specconv/spec_linux.go

 newConfig := new(configs.Seccomp)
 newConfig.Syscalls = []*configs.Syscall{}

+ // We don't currently support all seccomp flags.
+ for _, flag := range config.Flags {
+ switch flag {


Let's add support for SECCOMP_FILTER_FLAG_SPEC_ALLOW as well as SECCOMP_FILTER_FLAG_TSYNC

SECCOMP_FILTER_FLAG_TSYNC is there already and it's on by default (in libseccomp-golang). How would it be supported? Do you mean "let's explicitly ignore it", as I see no other way.

crun supports seccomp filter flags since containers/crun@fefabff runc will get them with opencontainers/runc#3390 youki will get them with containers/youki#733 To support them generally, we now copy the flags during the seccomp setup, otherwise they will get lost. Signed-off-by: Sascha Grunert <[email protected]>

saschagrunert · 2022-02-23T11:35:15Z

libcontainer/seccomp/seccomp_linux.go

+ // Add extra flags
+ for _, flag := range config.Flags {
+ switch flag {
+ case "SECCOMP_FILTER_FLAG_TSYNC":


Note: I think we should have them as consts in the runtime spec. I proposed that change in opencontainers/runtime-spec#1138

Note that seccomp-golang sets this flag unconditionally, and we (runc/libcontainer/seccomp/patchbpf) do not set this at all (not sure if this was by design or just forgotten; @cyphar WDYT?)

From what I see, it seems that for runc it does not make sense to do so, as we execute runc init right after applying seccomp rules, and it does not matter much whether these rules were applied to all threads or not.

There was an issue with setting it in patchbpf, I can't remember the exact details.

SECCOMP_FILTER_FLAG_TSYNC is not a flag on the bpf instructions but a flag for the seccomp() syscall, so I don't think patchbpf can do something with it.

TSYNC is set by libseccomp-golang by default, and there's no way to remove it. So adding it here does not make sense.

Ah, OK, here we're ignoring it, which I guess is a good thing to do.

kolyshkin · 2022-02-23T18:26:48Z

tests/integration/seccomp.bats

+@test "runc run [seccomp] (SECCOMP_FILTER_FLAG_TSYNC)" {
+ update_config ' .process.args = ["/bin/sh", "-c", "mkdir /dev/shm/foo"]
+ | .process.noNewPrivileges = false
+ | .linux.seccomp = {
+ "defaultAction":"SCMP_ACT_ALLOW",
+ "architectures":["SCMP_ARCH_X86","SCMP_ARCH_X32"],
+ "flags":["SECCOMP_FILTER_FLAG_TSYNC"],
+ "syscalls":[{"names":["mkdir"], "action":"SCMP_ACT_ERRNO"}]
+ }'
+
+ # This test checks that the tsync flag is accepted
+ runc run test_busybox
+ [ "$status" -ne 0 ]
+ [[ "$output" == *"mkdir:"*"/dev/shm/foo"*"Operation not permitted"* ]]
+}
+


I don't like the same code being repeated 3 times. Can probably factor it out to a function accepting the list of flags. That way we can also test a combination of flags, not just individual ones.

Updated with a loop to test all set of flags possible

libcontainer/specconv/spec_linux.go

libcontainer/configs/config.go

kolyshkin · 2022-03-14T17:59:14Z

One other thing -- I don't think it makes any sense to expose SECCOMP_FILTER_FLAG_TSYNC.

alban · 2022-05-06T13:21:18Z

Sorry I didn't follow up on this PR.

As a side note, I noticed we will have a new flag soon: SECCOMP_FILTER_FLAG_WAIT_KILLABLE_RECV
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?id=c2aa2dfef243
But it's not in a Linux release yet. So runc should add it later in a different PR.
Edit: it is in Linux 5.19-rc1

alban · 2022-06-27T14:38:12Z

I rebased, updated the PR description and commitmsg. I think it is ready for another review.

tests/integration/seccomp.bats

vnsavage · 2022-07-15T14:38:05Z

It seems like all requested changes are implemented on this PR? Is there anything else that needs doing before this gets merged?

alban · 2022-07-19T15:09:03Z

It seems my last update in this PR reliably breaks TestUsernsCheckpoint. Any ideas why?

kolyshkin · 2022-07-19T23:27:07Z

The error is EINVAL from move_mount(2) syscall. It can be returned if the kernel is too old, but it seems that

Linux v5.15 is used, which is sufficient;
criu does the check for the MOVE_MOUNT_SET_GROUP feature and finds it working.

There are other cases when the move mount set group can return EINVAL, but it does not seem likely.

Ah! It might be caused by the fact that criu ppa comes with criu 3.17-1ppa2.20.04 which is old (should be 3.17.1). I just saw that @rst0git updated that package 3 hours ago. So, let me restart the CI.

kolyshkin · 2022-07-19T23:29:03Z

TestUsernsCheckpoint also fails on CentOS Stream 9, although with a different diagnostics (from https://cirrus-ci.com/task/5426803694108672?logs=unit_tests#L1368):

    checkpoint_test.go:163: (00.041591) pie: 1: restoring lsm profile (current) unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
    checkpoint_test.go:163: (00.041654) Error (criu/cr-restore.c:1510): 65454 stopped by signal 11: Segmentation fault
    checkpoint_test.go:163: (00.042114) mnt: Switching to new ns to clean ghosts
    checkpoint_test.go:163: (00.042270) uns: calling exit_usernsd (-1, 1)
    checkpoint_test.go:163: (00.042312) uns: daemon calls 0x47fe40 (65452, -1, 1)
    checkpoint_test.go:163: (00.042319) uns: `- daemon exits w/ 0
    checkpoint_test.go:163: (00.042824) uns: daemon stopped
    checkpoint_test.go:163: (00.042829) Error (criu/cr-restore.c:2536): Restoring FAILED.
    checkpoint_test.go:163: === END ===

kolyshkin · 2022-07-19T23:29:32Z

@alban you also need to fix a linter warning

libcontainer/specconv/spec_linux.go

alban · 2022-07-20T09:52:14Z

PR updated and rebased.
The linter warning is fixed.
The TestUsernsCheckpoint issue is resolved in github-actions but there is still an issue in cirrus-ci with TestUsernsCheckpoint and TestCheckpoint.

rst0git · 2022-07-20T18:16:03Z

TestUsernsCheckpoint also fails on CentOS Stream 9, although with a different diagnostics (from https://cirrus-ci.com/task/5426803694108672?logs=unit_tests#L1368):

We recently updated the criu package in CentOS Stream (#3532)

kolyshkin

LGTM (CI failure is unrelated and is being addressed in #3532)

AkihiroSuda · 2022-07-21T02:45:06Z

Required statuses must pass before merging

Can't hit the merge button :(

rata · 2022-07-22T09:33:58Z

Again --- FAIL: TestUsernsCheckpoint (0.43s). I guess @alban needs to push again (or maybe rebase) so the tests are happy? Or is the problem on the CI not fixed?

alban · 2022-07-25T13:09:29Z

I don't have the rights to restart the Cirrus CI. I could force-push to run the CI again but I would lose the 2 approvals. Should I do that?

However I don't think that would work: #3532 (comment) says the criu RPM should show up in the CentOS repositories after 1 week but https://gitlab.com/redhat/centos-stream/rpms/criu/-/merge_requests/12 was merged on July 12th, so almost 2 weeks ago and I ran a test of this runc branch on a fork today and it still fail with the same error.

rata · 2022-07-28T13:53:08Z

@alban a fix was just merged for the test (#3539). If you rebase and push, the tests should re-start and work this time :)

Signed-off-by: Alban Crequy <[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]>

alban · 2022-07-28T14:27:13Z

@rata updated!

kolyshkin

LGTM

alban mentioned this pull request Feb 22, 2022

New gadget: audit-seccomp inspektor-gadget/inspektor-gadget#383

Merged

4 tasks

alban force-pushed the alban_log branch 2 times, most recently from 3bc5b6d to 006a4e3 Compare February 22, 2022 16:48

rata previously approved these changes Feb 22, 2022

View reviewed changes

tests/integration/seccomp.bats Outdated Show resolved Hide resolved

alban dismissed rata’s stale review via 40d8286 February 22, 2022 18:21

alban force-pushed the alban_log branch from 006a4e3 to 40d8286 Compare February 22, 2022 18:21

kolyshkin requested changes Feb 22, 2022

View reviewed changes

kolyshkin reviewed Feb 22, 2022

View reviewed changes

saschagrunert reviewed Feb 23, 2022

View reviewed changes

saschagrunert mentioned this pull request Feb 23, 2022

Add support for seccomp filter flags containers/common#937

Merged

alban force-pushed the alban_log branch from 35083f7 to a8ca927 Compare February 23, 2022 11:30

saschagrunert reviewed Feb 23, 2022

View reviewed changes

saschagrunert previously approved these changes Feb 23, 2022

View reviewed changes

alban dismissed saschagrunert’s stale review via f8fa8bf February 23, 2022 13:18

alban force-pushed the alban_log branch from a8ca927 to f8fa8bf Compare February 23, 2022 13:18

kolyshkin reviewed Feb 23, 2022

View reviewed changes

libcontainer/specconv/spec_linux.go Outdated Show resolved Hide resolved

kolyshkin reviewed Feb 23, 2022

View reviewed changes

libcontainer/configs/config.go Outdated Show resolved Hide resolved

saschagrunert mentioned this pull request Feb 24, 2022

Enable SECCOMP_FILTER_FLAG_SPEC_ALLOW per default containers/common#938

Closed

alban force-pushed the alban_log branch from f8fa8bf to e3de08f Compare May 13, 2022 10:19

alban force-pushed the alban_log branch from e3de08f to fb7f8df Compare June 27, 2022 14:14

alban changed the title ~~seccomp: add support for SECCOMP_FILTER_FLAG_LOG~~ seccomp: add support for flags Jun 27, 2022

kolyshkin reviewed Jun 27, 2022

View reviewed changes

tests/integration/seccomp.bats Outdated Show resolved Hide resolved

alban force-pushed the alban_log branch from fb7f8df to 38bb767 Compare June 28, 2022 15:45

AkihiroSuda added this to the 1.2.0 milestone Jul 16, 2022

kolyshkin reviewed Jul 19, 2022

View reviewed changes

libcontainer/specconv/spec_linux.go Outdated Show resolved Hide resolved

alban dismissed AkihiroSuda’s stale review via 63c33c3 July 20, 2022 09:39

alban force-pushed the alban_log branch from 691c817 to 63c33c3 Compare July 20, 2022 09:39

kolyshkin previously approved these changes Jul 20, 2022

View reviewed changes

AkihiroSuda previously approved these changes Jul 21, 2022

View reviewed changes

alban added 2 commits July 28, 2022 16:25

go.mod: update runtime-spec

c152e83

Signed-off-by: Alban Crequy <[email protected]>

alban dismissed stale reviews from AkihiroSuda and kolyshkin via 58ea21d July 28, 2022 14:26

alban force-pushed the alban_log branch from 63c33c3 to 58ea21d Compare July 28, 2022 14:26

kolyshkin approved these changes Jul 28, 2022

View reviewed changes

kolyshkin requested a review from AkihiroSuda July 28, 2022 19:32

AkihiroSuda approved these changes Jul 29, 2022

View reviewed changes

AkihiroSuda merged commit d11f4d7 into opencontainers:main Jul 29, 2022

alban deleted the alban_log branch July 29, 2022 21:14

This was referenced Aug 29, 2022

[4.11] add support for seccomp flags projectatomic/runc#57

Draft

seccomp: do not ignore SPEC_ALLOW flag #3580

Merged

seccomp flag handling is broken #3582

Closed

kolyshkin mentioned this pull request Oct 26, 2022

tests/int/seccomp: fix flags test on ARM #3650

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

seccomp: add support for flags #3390

seccomp: add support for flags #3390

alban commented Feb 22, 2022 •

edited

Loading

rata left a comment

kolyshkin left a comment

kolyshkin Feb 22, 2022

saschagrunert Feb 23, 2022

alban Feb 23, 2022

alban Feb 23, 2022

kolyshkin Feb 23, 2022

saschagrunert Feb 23, 2022

kolyshkin Feb 23, 2022

saschagrunert Feb 23, 2022

kolyshkin Mar 18, 2022

cyphar Jun 27, 2022

alban Jun 27, 2022

kolyshkin Jun 27, 2022

kolyshkin Jun 27, 2022

kolyshkin Feb 23, 2022

alban May 13, 2022

kolyshkin commented Mar 14, 2022

alban commented May 6, 2022 •

edited

Loading

alban commented Jun 27, 2022

vnsavage commented Jul 15, 2022 •

edited

Loading

alban commented Jul 19, 2022 •

edited

Loading

kolyshkin commented Jul 19, 2022

kolyshkin commented Jul 19, 2022

kolyshkin commented Jul 19, 2022

alban commented Jul 20, 2022

rst0git commented Jul 20, 2022

kolyshkin left a comment

AkihiroSuda commented Jul 21, 2022

rata commented Jul 22, 2022

alban commented Jul 25, 2022

rata commented Jul 28, 2022

alban commented Jul 28, 2022

kolyshkin left a comment

seccomp: add support for flags #3390

seccomp: add support for flags #3390

Conversation

alban commented Feb 22, 2022 • edited Loading

rata left a comment

Choose a reason for hiding this comment

kolyshkin 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

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

kolyshkin commented Mar 14, 2022

alban commented May 6, 2022 • edited Loading

alban commented Jun 27, 2022

vnsavage commented Jul 15, 2022 • edited Loading

alban commented Jul 19, 2022 • edited Loading

kolyshkin commented Jul 19, 2022

kolyshkin commented Jul 19, 2022

kolyshkin commented Jul 19, 2022

alban commented Jul 20, 2022

rst0git commented Jul 20, 2022

kolyshkin left a comment

Choose a reason for hiding this comment

AkihiroSuda commented Jul 21, 2022

rata commented Jul 22, 2022

alban commented Jul 25, 2022

rata commented Jul 28, 2022

alban commented Jul 28, 2022

kolyshkin left a comment

Choose a reason for hiding this comment

alban commented Feb 22, 2022 •

edited

Loading

alban commented May 6, 2022 •

edited

Loading

vnsavage commented Jul 15, 2022 •

edited

Loading

alban commented Jul 19, 2022 •

edited

Loading