exeseal: do not use F_SEAL_FUTURE_WRITE#4641
Merged
cyphar merged 1 commit intoopencontainers:mainfrom Feb 26, 2025
Merged
Conversation
Member
|
Thanks for the patch and figuring out the issue! Honestly, I think it would be simpler just to drop To be honest, I didn't read the man page very closely when adding it (I thought it restricted dirty page writebacks or something like that), but it seems that it is basically a no-op with |
934d060 to
fc1f8f1
Compare
cyphar
reviewed
Feb 25, 2025
16cef58 to
fded781
Compare
kolyshkin
reviewed
Feb 26, 2025
Contributor
kolyshkin
left a comment
There was a problem hiding this comment.
A nit, but perhaps it's better to remove the code entirely, rather than commenting it out. At most, add a comment telling something like
// Don't add F_SEAL_FUTURE_WRITE, it is not needed (and is buggy for Linux < 5.5).
lifubang
reviewed
Feb 26, 2025
Closed
e4cff82 to
869905f
Compare
Prior to kernel Linux 5.5, F_SEAL_FUTURE_WRITE has a bug which maps
memory as shared between processes even if it is set as private. See
kernel commit 05d351102dbe ("mm, memfd: fix COW issue on MAP_PRIVATE and
F_SEAL_FUTURE_WRITE mappings") for more details.
According to the fcntl(2) man pages, F_SEAL_WRITE is enough:
> Furthermore, trying to create new shared, writable memory-mappings via
> mmap(2) will also fail with EPERM.
>
> Using the F_ADD_SEALS operation to set the F_SEAL_WRITE seal fails
> with EBUSY if any writable, shared mapping exists. Such mappings must
> be unmapped before you can add this seal.
F_SEAL_FUTURE_WRITE only makes sense if a read-write shared mapping in
one process should be read-only in another process. This is not case for
runc, especially not for the /proc/self/exe we are protecting.
Signed-off-by: Tomasz Duda <tomaszduda23@gmail.com>
(cyphar: improve the comment regarding F_SEAL_FUTURE_WRITE)
(cyphar: improve commit message)
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
cyphar
approved these changes
Feb 26, 2025
Contributor
|
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Prior to kernel Linux 5.5 F_SEAL_FUTURE_WRITE has bug which maps memory as shared
between processes even if it is set as private.
torvalds/linux@05d3511
Accoring to https://man.archlinux.org/man/fcntl64.2.en F_SEAL_WRITE is enough.
F_SEAL_FUTURE_WRITE make sens only if R/W shared mapping in one process should be
R/O in another process. This is not case for runc.
Fixes #4640