-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Fix memory_deny_write_execute on x86 and s390 with libseccomp 2.4.2 #14167
Fix memory_deny_write_execute on x86 and s390 with libseccomp 2.4.2 #14167
Conversation
src/shared/seccomp-util.c
Outdated
|
||
if (loaded == 0) { | ||
log_debug_errno(r, "Failed to install any seccomp rules for MemoryDenyWriteExecute="); | ||
return 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We prefer to return 1if something happened, and 0 if nothing did. Also 8 space indentation. Maybe just do:
if (loaded == 0)
log_debug_errno(r, "Failed to install any seccomp rules for MemoryDenyWriteExecute=");
return loaded;
This return value is not used by anything? I'm not sure what you are trying to do here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking a look @keszybz
While src/core/execute.c doesn't care about the retval at least the tests in src/test/test-seccomp.c would and that way could spot issues like the main issue the first commit tries to address.
The already existing semantic of the retval is following seccomp_rule_*
which is to "return zero on success, negative errno values on failure". Your suggestion would still into that as a positive value (the rules loaded) would match well that pattern. So I'm adopting your suggestion.
Also I fixed the 8 space indent, thanks for the hint.
This will be in a coming push to the branch, but I see that @poettering just added review feedback as well. I'll go through that first before pushing anything.
src/shared/seccomp-util.c
Outdated
if (multiplexed) | ||
r = seccomp_rule_add(seccomp, SCMP_ACT_ERRNO(EPERM), nr, arg_cnt, arg); | ||
else | ||
r = seccomp_rule_add_exact(seccomp, SCMP_ACT_ERRNO(EPERM), nr, arg_cnt, arg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we stopped using the non-exact versions a long to ago since you never knew what you#d get, when used interchangably in whitelists and blacklists... i'd prefer if we can just deal with this issue in our own code.
indentation is off btw, please stick to 8ch, see https://systemd.io/CODING_STYLE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(also, we use bool
for booleans, except in public, exposed APIs, which this is not)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @poettering,
indention already fixed a minute ago after @keszybz pointing it out.
I'll carry the preference to stick to _excat
calls to the discussion in libseccomp project.
If there is a way to stick to _exact
that works I'm happy to use that and will refresh this pull request.
But ahead of time if there isn't a way what would we want to do about s390 and x86 then instead. We could obviously just ignore the retval in those cases, but that means less protection. Do you have any alternative ideas in mind already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I asked a similar question before and the answer I just got was
I would suggest using seccomp_rule_add(...) as it will handle all the architecture oddities for you; in fact for 99% of the cases out there seccomp_rule_add(...) is preferred over seccomp_rule_add_exact(...).
To me that seems that like only way to go unless we switch to more drastic changes (after all TBH who cares about 31/32bit - lets maybe just remove their shmat call, /me runs away ...)
I asked @pcmoore to come over and participate in the discussion here directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(also, we use
bool
for booleans, except in public, exposed APIs, which this is not)
Fixed as well, thanks
4cb7293
to
0e2db6b
Compare
After speaking out loud my heretic thought of "TBH who cares about 31/32bit" I gave this a new check from this POV. We already have this on ppc:
So we might just make |
BTW, we used to use the non-_exact() calls, until I ripped it all out: 469830d. It was not maintainable since we never knew what libseccomp would actually do if a rule could not be expressed on some arch: i.e. apply the rule incompletely or not at all, and the difference matters in some cases, i.e. when you do whitelisting rather than blacklisting. the strategy used by the non-_exact() stuff was a blackbox for us and too often not the strategy we actually want. I am not convinced we should start with that now. |
(we also started to write out rules per-arch ourselves in the same rework, instead of relying on libseccomp doing that, for similar reasons) (and we actually need the info what happens on which arch anyway, so that we can test for it. having libseccomp hide that in a black box and changing strategy behind our backs can't work if we test for our sandboxing to work correctly) |
anyway, i don't grok the actual problem here in the first place? is this just some multiplexing issue exactly like on i386? if so, then handle it the same way. |
libseccomp 2.4.2 got stricter on those multiplexed cases which breaks the behavior of systems secomp code for shmat on i386+s390+s390x.
Could I have a pointer to which same way you are referring - as i386 is broken at the moment with the new libseccomp. |
Since libseccomp 2.4.2 more architectures have shmat handled as multiplexed call. Those will fail to be added due to seccomp_rule_add_exact failing on them since they'd need to add multiple rules [1]. See the discussion at seccomp/libseccomp#193 After discussions about the options rejected [2][3] the initial thought of a fallback to the non '_exact' version of the seccomp rule adding the next option is to handle those now affected (i386, s390, s390x) the same way as ppc which ignores and does not block shmat. [1]: seccomp/libseccomp#193 [2]: systemd#14167 (comment) [3]: systemd@469830d1
0e2db6b
to
3296695
Compare
Rework and MotivationI reworked the PR according to the feedback of @poettering. That means I avoided the non- And since there formerly were questions about "why/what we'd need to fix here" - maybe it is too present in my head - let me add a few references to help with that:
I have built this on top of Ubuntus systemd (243) and re-ran the self tests. I also ran the No-Fix + new libseccomp (2.4.2)
Note: the invalid argument in these examples is the effect of seccomp_rule_add_exact on multiplexed calls Fix + old libseccomp (2.4.1)
Note: working with older libseccomp, our test already had a relaxed check: "Depending on kernel, libseccomp, and glibc versions, other architectures might fail or not. Let's not assert success." Fix + new libseccomp (2.4.2)
Now force-pushing the new fix onto this PR for re-review. |
PR looks good to me, but CI doesn't like it:
|
I'm finally coming back to this, sorry for the delay. So lets see how to make CI happy... I found that it actually is because I based my branch on #14162 as I'm testing and working on Ubuntu 20.04. The CI seems to run into exactly the issue discussed on that PR as well. As I outlined there, commit 67fb5f3 changed the shmat test as it is unable to predict if it will work/fail. The mmap test might need the same treatment to e.g. work well on Ubuntu 18.04 and 20.04 (and others) at the same time. I rebased to latest master and pushed the branch with the mentioned test cleanups (replacing the one in #14162 so I'm no more basing/depending on that branch/PR). |
3296695
to
1c57fde
Compare
Since libseccomp 2.4.2 more architectures have shmat handled as multiplexed call. Those will fail to be added due to seccomp_rule_add_exact failing on them since they'd need to add multiple rules [1]. See the discussion at seccomp/libseccomp#193 After discussions about the options rejected [2][3] the initial thought of a fallback to the non '_exact' version of the seccomp rule adding the next option is to handle those now affected (i386, s390, s390x) the same way as ppc which ignores and does not block shmat. [1]: seccomp/libseccomp#193 [2]: systemd#14167 (comment) [3]: systemd@469830d1
src/shared/seccomp-util.c
Outdated
@@ -1619,7 +1620,6 @@ int seccomp_memory_deny_write_execute(void) { | |||
case SCMP_ARCH_X86_64: | |||
case SCMP_ARCH_X32: | |||
case SCMP_ARCH_AARCH64: | |||
case SCMP_ARCH_S390X: | |||
filter_syscall = SCMP_SYS(mmap); /* amd64, x32, s390x, and arm64 have only mmap */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comments needs minor updating
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely, good catch - dropping s390x from this.
I only wanted to wait with the push to see the result of the s390x CI run on Bionic to be able to rework it right away if there are still issues. But that Test now completed and worked.
Pushing with the comment cleanup now ...
Since libseccomp 2.4.2 more architectures have shmat handled as multiplexed call. Those will fail to be added due to seccomp_rule_add_exact failing on them since they'd need to add multiple rules [1]. See the discussion at seccomp/libseccomp#193 After discussions about the options rejected [2][3] the initial thought of a fallback to the non '_exact' version of the seccomp rule adding the next option is to handle those now affected (i386, s390, s390x) the same way as ppc which ignores and does not block shmat. [1]: seccomp/libseccomp#193 [2]: systemd#14167 (comment) [3]: systemd@469830d1
1c57fde
to
63b3e30
Compare
Hmpf... So for some reason the ubuntu CI disappeared from this PR? I'd really like to see its results though, since it compiles on s390 and other more exotic archs which none of the other CIs do. We can't really restart the CI though, hence can you rebase on current master and repush? That should make the CIs pick this PR up anew again? |
Since libseccomp 2.4.2 more architectures have shmat handled as multiplexed call. Those will fail to be added due to seccomp_rule_add_exact failing on them since they'd need to add multiple rules [1]. See the discussion at seccomp/libseccomp#193 After discussions about the options rejected [2][3] the initial thought of a fallback to the non '_exact' version of the seccomp rule adding the next option is to handle those now affected (i386, s390, s390x) the same way as ppc which ignores and does not block shmat. [1]: seccomp/libseccomp#193 [2]: systemd#14167 (comment) [3]: systemd@469830d1
If seccomp_memory_deny_write_execute was fatally failing to load rules it already returned a bad retval. But if any adding filters failed it skipped the subsequent seccomp_load and always returned an rc of 0 even if no rule was loaded at all. Lets fix this requiring to (non fatally-failing) load at least one rule set. Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
At the beginning of seccomp_memory_deny_write_execute architectures can set individual filter_syscall, block_syscall, shmat_syscall values. The former two are then used in the call to add_seccomp_syscall_filter but shmat_syscall is not. Right now all shmat_syscall values are the same, so the change is a no-op, but if ever an architecture is added/modified this would be a subtle source for a mistake so fix it by using shmat_syscall later. Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Like with shmat already the actual results of the test test_memory_deny_write_execute_mmap depend on kernel/libseccomp/glibc of the platform it is running on. There are known-good platforms, but on the others do not assert success (which implies test has actually failed as no seccomp blocking was achieved), but instead make the check dependent to the success of the mmap call on that platforms. Finally the assert of the munmap on that valid pointer should return ==0, so that is what the check should be for in case of p != MAP_FAILED. Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Odd that "some" are missing. Maybe because I was waiting for the former CI runs to complete and I force-pushed when it was done on Bionic-s390x but still waiting on others? Anyway - rebased and repushed for another run. |
63b3e30
to
49219b5
Compare
The CI infra is a bit flakey it seems, there's no system behind it when CI jobs disappear. At least none I recognize... But this time they got picked up properly, all 12 CIs are chugging away on this PR. |
LGTM. |
I'm glad that bionix-s390x completed as a good example this time, since when I look for 49219b5 on the link that Details leads to the tests look to me as if they'd hang after being fished with some build task (all of them, not just mine). It doesn't seem to be triggered by my changes and I can't really fix what those CI runs do :-/ |
Ok, we got the CI please \o/ The remaining "requested changes" are from @keszybz but he later gave his LGTM, so we can mark that resolved as well. With that this should be good to go right? |
#14265 makes the code "correct", but it still is not doing anything useful except a debug message. |
@keszybz - on that particular commit the intention was two fold.
The initial thought was to always ensure rules are loaded, but that got changed later. please remember that the interim version we have discussed here made the rules work on affected architectures. And from there the idea was to eventually ensure that it always had rules loaded. But in the discussion we changed that - as we didn't wan't to use the non '_exact' libseccomp calls. With that we'd have diverged from that idea leaving the not-wrong but admittedly currently unused retval there. Currently a check of it could easily be added at:
Thanks for the fixup on the errno btw! |
OK. So essentially the commit message wasn't updated, but the code is fine as is. Thanks for the explanation. |
Since libseccomp 2.4.2 more architectures have shmat handled as multiplexed call. Those will fail to be added due to seccomp_rule_add_exact failing on them since they'd need to add multiple rules [1]. See the discussion at seccomp/libseccomp#193 After discussions about the options rejected [2][3] the initial thought of a fallback to the non '_exact' version of the seccomp rule adding the next option is to handle those now affected (i386, s390, s390x) the same way as ppc which ignores and does not block shmat. [1]: seccomp/libseccomp#193 [2]: systemd/systemd#14167 (comment) [3]: systemd/systemd@469830d1 (cherry picked from commit bed4668)
Hi,
for now this is meant to start a discussion and make people aware.
Discussions with libseccomp upstream are still ongoing and also I'm not sure on your preferences in regard to falling back to non
_exact
seccomp calls.Background
systemd commit 67fb5f already tried to fix expectations for
test_memory_deny_write_execute_shmat
.But it turns out on s390x/s390 and 32bit x86 the same nature of being multiplexed (or not) makes
seccomp_rule_add_exact
fail on some combinations of kernel/libseccomp - in particular since libseccomp 2.4.2.This was discussed with libseccomp in an issue on their tracker.
And it seems that is intended behavior and for rules on shmat we can't use
_exact
at all anymore.Issue to systemd
That leads to a bad rc returned
seccomp_rule_add_exact
->add_seccomp_syscall_filter
->seccomp_memory_deny_write_execute
which will then in turn skip the loading of the rules as the continue due to the bad rc will skip&loop over seccomp_load(seccomp).As a suggestion from libseccomp upstream we could use the non
_exact
versions of those calls for items we know to be affected. This makes it "work again" on x86 and "work" on s390x (which it did before on some combinations of glibc/libseccomp but not on others).It is possible (see libseccomp discussion) be that with libseccomp 2.4.3 s390x will not need this anymore, but it seems x86 and s390 will continue to do so.
With the changes applied I retested s390x and x86 in Ubuntu 20.04 (where I had the initial issues).
I haven't touched older kernels / glibcs in my tests yet, these tests ran on Ubuntu 20.04 with 5.3.0-18-generic and 2.30-0ubuntu2 respectively.
Extra fixes around seccomp handling in this MR
The first commit in the series tries to fix the issue I hit when moving to libseccomp 2.4.2, the other two in the MR try to fix issues I spotted along the way - the commit messages explain in detail what they are about.
Based on #14162
P.S. I first revoked commit a81f7aa from rbalint on my Ubuntu tests as the move to libseccomp 2.4.2 broke s390, but with my fixes it works again therefore this series is for now intentionally based on-top of his MR.
The whole situation is convoluted in between kernel/glibc/libseccomp versions, so please let me know what you think. Also please feel free to chime in on the libseccomp discussion. I'll ask there to do the same here.