seccomp: allow to override default errno return code

[giuseppe]

the specs already support overriding the errno code for the syscalls
but the default value is hardcoded to EPERM.

Add a new attribute to override the default value.

Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com

[rhatdan]

LGTM
@mrunalp @cyphar PTAL

[mrunalp]

LGTM

[fweimer-rh]

This makes the runc workaround explicitly non-compliant, doesn't it? It sometimes returns EPERM, and sometimes ENOSYS, and not always EPERM.

As previously discussed, it is not entirely clear when that default should be applied. To any system call not listed in the policy?

Is EPERM really the right default here?

@richfelker

[richfelker]

I have always believed (1) ENOSYS should be the default and (2) the system should not have any method for causing as-yet-unknown syscalls (at the time of profile authorship) to fail with any error code other than ENOSYS. Of these, (2) is the most important, and I understand there's a backwards-compatibility argument for returning EPERM as default for syscalls that were in the known range when the filter was written. I'm indifferent to adding the ability to change the default for this range as long as the unknown range is always forcibly ENOSYS.

[giuseppe]

Is EPERM really the right default here?

Probably not, I've picked it just for backward compatibility.

I have always believed (1) ENOSYS should be the default and (2) the system should not have any method for causing as-yet-unknown syscalls (at the time of profile authorship) to fail with any error code other than ENOSYS. Of these, (2) is the most important, and I understand there's a backwards-compatibility argument for returning EPERM as default for syscalls that were in the known range when the filter was written. I'm indifferent to adding the ability to change the default for this range as long as the unknown range is always forcibly ENOSYS.

I personally think the default for other actions should still be EPERM as they are known at the time the profile was written but it is just a matter of taste. If a container engine uses the new defaultErrnoRet then it knows about its meaning and we can pick the behavior we prefer and a seccomp profile can already override the errno value for each specified syscall.

[thaJeztah]

LGTM, thanks!

Actually had a branch with this, but didn't find the time yet to open 😂

[giuseppe]

thanks anyone for the reviews. Before merging let's wait for @cyphar to weight in as he was looking the most at this issue.

[vrothberg]

LGTM

[cyphar]

I'm fine with this PR, but as I've discussed before -- just to make it completely explicit -- this does not solve the -ENOSYS problem. It's good that we're doing this for completeness's sake, but you would need all profiles to switch to ENOSYS and even then you're going to end up in scenarios where socket(2) returns -ENOSYS spuriously.

I think it would've been nicer to do this as one big changeset -- I think it would be a bad idea to release a new runtime-spec which adds this and doesn't address the larger -ENOSYS problem.

(2) the system should not have any method for causing as-yet-unknown syscalls (at the time of profile authorship) to fail with any error code other than ENOSYS

I agree, and the reason I wanted to wait before we did this change is because I wanted us to include all of the other changes we need to solve this problem properly in the spec. But we can take this change now and do the other necessary changes later.

[h-vetinari]

@cyphar: I think it would be a bad idea to release a new runtime-spec which adds this and doesn't address the larger -ENOSYS problem.

By that argument, merging this PR would make the spec "unreleaseable". Given how thorny this problem seems to be, and how there have been calls for a new version for ~8 months, would it be feasible to release 1.0.3 before merging this?

[richfelker]

Why can't the spec just be fixed to specify what runc is doing to fix the problem?

[giuseppe]

It's good that we're doing this for completeness's sake, but you would need all profiles to switch to ENOSYS and even then you're going to end up in scenarios where socket(2) returns -ENOSYS spuriously.

would this happen when a syscall is not fully specified in the profile and the defaultErrnoRet is used or are there other cases?

[cyphar]

@giuseppe

would this happen when a syscall is not fully specified in the profile and the defaultErrnoRet is used or are there other cases?

That is the main issue -- but given that libseccomp today doesn't let you create fully-specified rules (depending on how complicated the allow rule is), that's sufficient to say that defaultErrnoRet cannot alone be used to solve the -ENOSYS problem.

In addition, in my view we should not be relying on profile authors to solve the -ENOSYS problem at all -- this is such a footgun that runtimes should handle it automatically (similarly to what runc is doing today). So in my view the complete solution for this problem is that we specify that the runtime should (or must) resolve this problem in a particular way. (Maybe there should be an opt-out from this behaviour, I'd have to think about exactly how that would work.)

To answer @richfelker, I tihnk that the spec-level solution should be a little bit less janky than the current runc solution. (Ideally we would do this through minimum kernel version information.)

@h-vetinari

Given how thorny this problem seems to be, and how there have been calls for a new version for ~8 months, would it be feasible to release 1.0.3 before merging this?

Yeah let's do that first.

[giuseppe]

@giuseppe

would this happen when a syscall is not fully specified in the profile and the defaultErrnoRet is used or are there other cases?

That is the main issue -- but given that libseccomp today doesn't let you create fully-specified rules (depending on how complicated the allow rule is), that's sufficient to say that defaultErrnoRet cannot alone be used to solve the -ENOSYS problem.

true that won't fully solve the issue with MASKED_EQ, that must be addressed in libseccomp first (and it seems it is likely going to be added soon (2.6.0?))

In addition, in my view we should not be relying on profile authors to solve the -ENOSYS problem at all -- this is such a footgun that runtimes should handle it automatically (similarly to what runc is doing today). So in my view the complete solution for this problem is that we specify that the runtime should (or must) resolve this problem in a particular way. (Maybe there should be an opt-out from this behaviour, I'd have to think about exactly how that would work.)

To answer @richfelker, I tihnk that the spec-level solution should be a little bit less janky than the current runc solution. (Ideally we would do this through minimum kernel version information.)

do you see the kernel version information as a replacement for defaultErrnoRet or the two features can coexist together?

[cyphar]

@giuseppe

true that won't fully solve the issue with MASKED_EQ

MASKED_EQ is a problem (though it is one you can work around in a fairly dodgy manner by generating 2^n MASKED_EQ rules). But the biggest issue is that you cannot construct multiple AND rules for the same argument with today's libseccomp (and there is no roadmap for fixing this). This means you cannot generate the inverse of (arg0 == 1 || arg1 == 2) because (arg0 != 1 && arg1 != 2) is an invalid rule -- and as far as I can tell there is no clever way to work around this without having to generate an exhaustive set of 2^64 rules in the pathological case (and that's only if one argument has || rules -- it'll get even more complicated with multiple arguments).

Do you see the kernel version information as a replacement for defaultErrnoRet or the two features can coexist together?

They can both co-exist -- the lack of defaultErrnoRet should be fixed purely for completeness's sake (and I can imagine a few cases where someone might want to use a different default errno).

[giuseppe]

MASKED_EQ is a problem (though it is one you can work around in a fairly dodgy manner by generating 2^n MASKED_EQ rules). But the biggest issue is that you cannot construct multiple AND rules for the same argument with today's libseccomp (and there is no roadmap for fixing this). This means you cannot generate the inverse of (arg0 == 1 || arg1 == 2) because (arg0 != 1 && arg1 != 2) is an invalid rule -- and as far as I can tell there is no clever way to work around this without having to generate an exhaustive set of 2^64 rules in the pathological case (and that's only if one argument has || rules -- it'll get even more complicated with multiple arguments).

Did you mean (arg0 == 1 || arg0 == 2)? Yeah, that is another possible problem with libseccomp :(

In the long term I really hope we can bypass the seccomp generation and add to the specs something like containers/crun#578 (in the seccomp section). Alternatively, at least agree on something nicer than JSON and that users are not expected to deal directly with the libseccomp limitations.

[cyphar]

Did you mean (arg0 == 1 || arg0 == 2)? Yeah, that is another possible problem with libseccomp :(

Oops, yeah that's what I meant. And practically this is more critical than MASKED_EQ because there is a workable fix for MASKED_EQ (not to mention it will be fixed soon since it's fairly simple to add) -- the lack of && support is pretty major.

In the long term I really hope we can bypass the seccomp generation and add to the specs something like containers/crun#578 (in the seccomp section).

I agree, though I am on the fence on whether we should still have -ENOSYS stubs be auto-added by runtimes even with custom seccomp profiles. Maybe it makes sense to allow users to opt-out of it, but I still think seccomp is far too footgun-friendly for us to leave it all up to profile generators.

HOWEVER there are upsides to the current system:

• It's possible to understand the seccomp profile logic -- inserting compiled BPF is not really ideal either.
• Profile generators can just use the syscall names rather than needing to know the actual number (in practice you can have the higher-level runtime just support a better generator -- but the issue is that part of the container configuration is now outside of the runtime-spec).

It should also be noted that while the current seccomp schema is based around libseccomp's limitations, nothing is stopping us from switching away from libseccomp internally.

[giuseppe]

I agree, though I am on the fence on whether we should still have -ENOSYS stubs be auto-added by runtimes even with custom seccomp profiles. Maybe it makes sense to allow users to opt-out of it, but I still think seccomp is far too footgun-friendly for us to leave it all up to profile generators.

I still think it is useful to allow the raw BPF. I have already considered the pre-compilation of BPF in the past, since most containers use the same BPF profile, to avoid the cost of looking up syscalls in libseccomp. The lookup function in 2.4.0 had O(n) complexity and it is used for each syscall specified in the profile (but then I preferred to go with seccomp/libseccomp#223 and address it in libseccomp itself).

The raw BPF feature should not replace the existing mechanism, neither be the default; but it would open up the possibility to experiment with different ways of generating the BPF. IMO, users should be able to shoot themselves in the foot if they wish so.

I'm fine with this PR, but as I've discussed before -- just to make it completely explicit -- this does not solve the -ENOSYS problem. It's good that we're doing this for completeness's sake, but you would need all profiles to switch to ENOSYS and even then you're going to end up in scenarios where socket(2) returns -ENOSYS spuriously

Could we move forward with it as it is just for completeness and we can address the ENOSYS issue separately? I'd expect the discussion to take some time, while this feature will at least help us to temporarily address the issue with most seccomp profiles.

[cyphar]

Could we move forward with it as it is just for completeness and we can address the ENOSYS issue separately? I'd expect the discussion to take some time, while this feature will at least help us to temporarily address the issue with most seccomp profiles.

Yeah we can merge this first.

[cyphar]

LGTM.

[mrunalp]

LGTM

[tianon]

Arg, we've run into some fun with this downstream in moby/moby thanks to errno values being a per-architecture thing: moby/moby#42836 (comment) / moby/moby#42681 (comment)

$ 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)

Perhaps this should've been a string? 😞

[utam0k]

Hi! I love this change. Is there any plan to add a check for this to the runtime-tools tests?

[giuseppe]

Arg, we've run into some fun with this downstream in moby/moby thanks to errno values being a per-architecture thing: moby/moby#42836 (comment) / moby/moby#42681 (comment)

$ 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)

Perhaps this should've been a string? disappointed

indeed, that was a mistake :/

Any suggestions on how we can fix it? Do we need a new field and obsolete the current one?

[thaJeztah]

The portable way would probably to have a string representation, so that the runtime that sets the actual profile can do the conversion 🤔

FWIW, I think this would only be a problem if a profile is read from a JSON, and the JSON is created on / with a non-matching architecture in mind, but I can see a string representation easier to use (when writing a profile).

[thaJeztah]

Do we need a new field and obsolete the current one?

If we decide to add a new field, we could make the new filed take precedence (if new field is set, old field must be ignored)

[giuseppe]

If we decide to add a new field, we could make the new filed take precedence (if new field is set, old field must be ignored)

yes, I agree with it. I've used the same technique for containers/common seccomp profiles: containers/common#821

[thaJeztah]

For the meantime, for moby, I opened moby/moby#43005, which uses conditional rules for MIPS / non-MIPS, with hard-coded values for ENOSYS. It's a bit dirty, but should do the trick for now (to allow users to save the profile in JSON).

[tianon]

From the perspective of the specification, this hasn't actually been released yet, so I think we have a small amount of latitude in fixing this "properly", right? I imagine actual users in the wild are limited to the two cases already identified, and probably only really ENOSYS or EPERM? 😅

(I doubt this has been an extremely popular feature for users to take advantage of in their custom profiles -- the number of times I've seen someone write a non-trivial custom seccomp profile that wasn't just "download the default from my runtime and change one minor thing" I can count on a single hand. 🙈)

[giuseppe]

From the perspective of the specification, this hasn't actually been released yet, so I think we have a small amount of latitude in fixing this "properly", right? I imagine actual users in the wild are limited to the two cases already identified, and probably only really ENOSYS or EPERM?

I think we can probably drop defaultErrnoRet and errnoRet once the new version that uses a string settles down.

Would you be fine if I add two fields defaultErrno and errno that accept a string and have higher precedence over defaultErrnoRet and errnoRet that will be marked as obsolete?

[thaJeztah]

From the perspective of the specification, this hasn't actually been released yet, so I think we have a small amount of latitude in fixing this "properly", right?

Good call; looks indeed like it was not in a release yet; v1.0.2...main

From that perspective, I'd be 👍 to fix it before it's released. With the added note that we need to check what projects already shipped with this feature in use (mostly concerned if, e.g., runc uses it already and/or what happens if containers were created with this option set, and it's upgraded 🤔)