Skip to content
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

exeseal: do not use F_SEAL_FUTURE_WRITE #4641

Merged
merged 1 commit into from
Feb 26, 2025

Conversation

tomaszduda23
Copy link
Contributor

@tomaszduda23 tomaszduda23 commented Feb 24, 2025

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.

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

@cyphar
Copy link
Member

cyphar commented Feb 25, 2025

Thanks for the patch and figuring out the issue! Honestly, I think it would be simpler just to drop F_SEAL_FUTURE_WRITE entirely.

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 F_SEAL_WRITE aside from bugs like this.

@cyphar cyphar added the backport/1.2-todo A PR in main branch which needs to be backported to release-1.2 label Feb 25, 2025
@cyphar cyphar added this to the 1.3.0-rc.1 milestone Feb 25, 2025
@tomaszduda23 tomaszduda23 changed the title Use F_SEAL_FUTURE_WRITE only if kernel >= Linux 5.5. Do not use F_SEAL_FUTURE_WRITE. Feb 25, 2025
@tomaszduda23 tomaszduda23 force-pushed the memfd branch 2 times, most recently from 934d060 to fc1f8f1 Compare February 25, 2025 07:15
Copy link
Member

@cyphar cyphar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@tomaszduda23 tomaszduda23 force-pushed the memfd branch 2 times, most recently from 16cef58 to fded781 Compare February 25, 2025 07:25
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@cyphar cyphar mentioned this pull request Feb 26, 2025
@cyphar cyphar force-pushed the memfd branch 4 times, most recently from e4cff82 to 869905f Compare February 26, 2025 05:40
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 <[email protected]>
(cyphar: improve the comment regarding F_SEAL_FUTURE_WRITE)
(cyphar: improve commit message)
Signed-off-by: Aleksa Sarai <[email protected]>
@cyphar cyphar changed the title Do not use F_SEAL_FUTURE_WRITE. exeseal: do not use F_SEAL_FUTURE_WRITE Feb 26, 2025
@cyphar cyphar merged commit cfb643e into opencontainers:main Feb 26, 2025
34 checks passed
@kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin added backport/1.2-done A PR in main branch which has been backported to release-1.2 and removed backport/1.2-todo A PR in main branch which needs to be backported to release-1.2 labels Feb 26, 2025
@rata rata mentioned this pull request Mar 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.2-done A PR in main branch which has been backported to release-1.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

runc gets stuck
4 participants