[20.10 backport] seccomp: add support for "clone3" syscall in default policy

[tianon]

This is a backport of 9f6b562 (#42681, see also #42680), adapted to avoid the refactoring that happened in d927397 (#42005).

fixes #42963
fixes #42876

[tianon]

Doh, build_userns_linux_test.go:73: assertion failed: error is not nil: OCI runtime create failed: container_linux.go:380: starting container process caused: error adding seccomp filter rule for syscall clone3: requested action matches default action of filter: unknown looks very related. 😞

[thaJeztah]

hmmmm yes. ISTR there was a recent change in runc related to that (at least, I recall a PR about ignoring defaults)

[thaJeztah]

Yup; fix for that is in v1.0.2. But that seems to indicate we may not need this patch / backport? opencontainers/runc#3173

[tianon]

Now that I'm reading that error message with a fresh eye, I realize it's complaining that SCMP_ACT_ERRNO is the default action, which ... yep, what of it? The errnoRet is different?? 😕

[tianon]

That makes me wonder if the "fix" in opencontainers/runc#3129 is actually correct? Skipping rules like this doesn't feel like the right answer.

Scratch that; looking at the implementation there, I see it does take errnoRet into account.

[tianon]

I think I found what was wrong. 🤞

[tianon]

Got it -- the only failure on the last run was gofmt, which I've corrected, so this one should be good.

[tianon]

FWIW, @lucaskanashiro tested this and reported in https://bugs.launchpad.net/cloud-images/+bug/1943049/comments/25 that it does fix the problem 😄

[tianon]

Re-reviewing this, it felt silly to still have the createSpecsSyscall function which is only invoked once (and with an unused argument to boot, because I didn't notice in my refactoring 😂), so I've updated again to remove that completely.

[thaJeztah]

LGTM

[gotmax23]

Hi,

Can you please consider merging this change and expediting a release containing the fix? This clone3 issue has caused a lot of problems over the past couple months.

Thanks you for your work. I apologize if I'm being too pushy.

-- Maxwell 😀

[tianon]

@zhsj brought https://buildd.debian.org/status/fetch.php?pkg=docker.io&arch=mips64el&ver=20.10.10%2Bdfsg1-1&stamp=1636015943&raw=0 to my attention in which TestUnmarshalDefaultProfile fails on mips64le because ENOSYS has a different numeric value (because of course it does, right? we can't possibly have nice things)

=== FAIL: profiles/seccomp TestUnmarshalDefaultProfile (0.19s)
    seccomp_test.go:68: assertion failed: 
        --- expected.Syscalls
        +++ profile.Syscalls
          []*seccomp.Syscall{
          	... // 14 identical elements
          	&{Names: {"clone"}, Action: "SCMP_ACT_ALLOW", Args: {&{Value: 2114060288, Op: "SCMP_CMP_MASKED_EQ"}}, Excludes: {Caps: {"CAP_SYS_ADMIN"}, Arches: {"s390", "s390x"}}},
          	&{Names: {"clone"}, Action: "SCMP_ACT_ALLOW", Args: {&{Index: 1, Value: 2114060288, Op: "SCMP_CMP_MASKED_EQ"}}, Comment: "s390 parameter ordering for clone is different", ...},
          	&{
          		Name:     "",
          		Names:    {"clone3"},
          		Action:   "SCMP_ACT_ERRNO",
        - 		ErrnoRet: &89,
        + 		ErrnoRet: &38,
          		Args:     {},
          		Comment:  "",
          		... // 2 identical fields
          	},
          	&{Names: {"reboot"}, Action: "SCMP_ACT_ALLOW", Args: {}, Includes: {Caps: {"CAP_SYS_BOOT"}}, ...},
          	&{Names: {"chroot"}, Action: "SCMP_ACT_ALLOW", Args: {}, Includes: {Caps: {"CAP_SYS_CHROOT"}}, ...},
          	... // 8 identical elements
          }

I think this test will probably fail on MIPS arches on the version that made it into master, too (for the same reason).

Edit: I've raised this further up the chain at opencontainers/runtime-spec#1087 (comment) now

[tianon]

On the plus side, at least this only affects MIPS* and sparc64? 😬

$ git grep -inE 'ENOSYS[[:space:]]+=' | grep linux | column -t
unix/zerrors_linux_386.go:588:       ENOSYS  =  syscall.Errno(0x26)
unix/zerrors_linux_amd64.go:588:     ENOSYS  =  syscall.Errno(0x26)
unix/zerrors_linux_arm.go:594:       ENOSYS  =  syscall.Errno(0x26)
unix/zerrors_linux_arm64.go:585:     ENOSYS  =  syscall.Errno(0x26)
unix/zerrors_linux_mips.go:591:      ENOSYS  =  syscall.Errno(0x59)
unix/zerrors_linux_mips64.go:591:    ENOSYS  =  syscall.Errno(0x59)
unix/zerrors_linux_mips64le.go:591:  ENOSYS  =  syscall.Errno(0x59)
unix/zerrors_linux_mipsle.go:591:    ENOSYS  =  syscall.Errno(0x59)
unix/zerrors_linux_ppc.go:646:       ENOSYS  =  syscall.Errno(0x26)
unix/zerrors_linux_ppc64.go:650:     ENOSYS  =  syscall.Errno(0x26)
unix/zerrors_linux_ppc64le.go:650:   ENOSYS  =  syscall.Errno(0x26)
unix/zerrors_linux_riscv64.go:575:   ENOSYS  =  syscall.Errno(0x26)
unix/zerrors_linux_s390x.go:650:     ENOSYS  =  syscall.Errno(0x26)
unix/zerrors_linux_sparc64.go:639:   ENOSYS  =  syscall.Errno(0x5a)

Edit: I've raised this further up the chain at opencontainers/runtime-spec#1087 (comment) now

[thaJeztah]

Ahm.. yes, so we could adjust the test and use a different default.json for that architecture, or (bit of a hack), make the rule conditional, and used a hard-coded value for mips and sparc64. Looks like for clone, we also had to use a conditional rule;

moby/profiles/seccomp/default_linux.go

Lines 613 to 619 in 2b70006

	Comment: "s390 parameter ordering for clone is different",
	Includes: &Filter{
	Arches: []string{"s390", "s390x"},
	},
	Excludes: &Filter{
	Caps: []string{"CAP_SYS_ADMIN"},
	},

[thaJeztah]

But I guess the "portable" way would be to use a string for this, and have the conversion done by the runtime that sets the actual profile