rootfs: handle nested procfs mounts for MS_MOVE #2647
Merged
Conversation
This file contains 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
|
/cc @giuseppe Does this change make sense to you? |
giuseppe
previously approved these changes
Oct 13, 2020
kolyshkin
reviewed
Oct 13, 2020
|
LGTM except for a single nit. |
In a case where the host /proc mount has already been overmounted, the MS_MOVE handling would get ENOENT when trying to hide (for instance) "/proc/bus" because it had already hidden away "/proc". This revealed two issues in the previous implementation of this hardening feaure: 1. No checks were done to make sure the mount was a "full" mount (it is a mount of the root of the filesystem), but the kernel doesn't permit a non-full mount to be converted to a full mount (for reference, see mnt_already_visible). This just removes extra busy-work during setup. 2. ENOENT was treated as a critical error, even though it actually indicates the mount doesn't exist and thus isn't a problem. A more theoretically pure solution would be to store the set of mountpoints to be hidden and only ignore the error if an ancestor directory of the current mountpoint was already hidden, but that would just add complexity with little justification. In addition, better document the reasoning behind this logic so that folks aren't confused when looking at it. Fixes: 28a697c ("rootfs: umount all procfs and sysfs with --no-pivot") Signed-off-by: Aleksa Sarai <[email protected]>
|
@AkihiroSuda PTAL |
mrunalp
approved these changes
Oct 19, 2020
AkihiroSuda
added a commit
that referenced
this pull request
Oct 20, 2020
This reverts commit b8bf572. Revert #2647 Reopen #2339 Signed-off-by: Akihiro Suda <[email protected]>
cyphar
added a commit
to cyphar/moby-sys
that referenced
this pull request
Oct 20, 2020
Previously, the filter would be run before all of the fields were parsed (this behaviour also was not documented) -- this resulted in users of the function accidentally assuming that fields like fsinfo.Root would actually be filled correctly. It seems that the performance overhead of parsing a few extra fields is not exorbitant, and optimising this just leads to incorrect user code. For a concrete example, this optimisation actually made this runc change[1] regress a security hardening feature because it relied on fsinfo.Root being filled correctly. [1]: opencontainers/runc#2647 Signed-off-by: Aleksa Sarai <[email protected]>
AkihiroSuda
added a commit
that referenced
this pull request
Oct 20, 2020
For preventing regression like #2647 Signed-off-by: Akihiro Suda <[email protected]>
cyphar
added a commit
to cyphar/moby-sys
that referenced
this pull request
Oct 21, 2020
Previously, the filter would be run before all of the fields were parsed (this behaviour also was not documented) -- this resulted in users of the function accidentally assuming that fields like fsinfo.Root would actually be filled correctly. It seems that the performance overhead of parsing a few extra fields is not exorbitant, and optimising this just leads to incorrect user code. For a concrete example, this optimisation actually made this runc change[1] regress a security hardening feature because it relied on fsinfo.Root being filled correctly. [1]: opencontainers/runc#2647 Signed-off-by: Aleksa Sarai <[email protected]>
kolyshkin
pushed a commit
to kolyshkin/runc
that referenced
this pull request
Oct 22, 2020
For preventing regression like opencontainers#2647 Signed-off-by: Akihiro Suda <[email protected]> Signed-off-by: Aleksa Sarai <[email protected]>
kolyshkin
pushed a commit
to kolyshkin/runc
that referenced
this pull request
Oct 22, 2020
For preventing regression like opencontainers#2647 Signed-off-by: Akihiro Suda <[email protected]> Signed-off-by: Aleksa Sarai <[email protected]> Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin
pushed a commit
to kolyshkin/runc
that referenced
this pull request
Oct 22, 2020
For preventing regression like opencontainers#2647 Signed-off-by: Akihiro Suda <[email protected]> Signed-off-by: Aleksa Sarai <[email protected]> Signed-off-by: Kir Kolyshkin <[email protected]>
dqminh
pushed a commit
to dqminh/runc
that referenced
this pull request
Feb 3, 2021
For preventing regression like opencontainers#2647 Signed-off-by: Akihiro Suda <[email protected]> Signed-off-by: Aleksa Sarai <[email protected]> Signed-off-by: Kir Kolyshkin <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
In a case where the host /proc mount has already been overmounted, the
MS_MOVE handling would get ENOENT when trying to hide (for instance)
"/proc/bus" because it had already hidden away "/proc". This revealed
two issues in the previous implementation of this hardening feaure:
No checks were done to make sure the mount was a "full" mount (it is
a mount of the root of the filesystem), but the kernel doesn't permit
a non-full mount to be converted to a full mount (for reference, see
mnt_already_visible). This just removes extra busy-work during setup.
ENOENT was treated as a critical error, even though it actually
indicates the mount doesn't exist and thus isn't a problem. A more
theoretically pure solution would be to store the set of mountpoints
to be hidden and only ignore the error if an ancestor directory of
the current mountpoint was already hidden, but that would just add
complexity with little justification.
In addition, better document the reasoning behind this logic so that
folks aren't confused when looking at it.
Fixes #2639
Fixes: 28a697c ("rootfs: umount all procfs and sysfs with --no-pivot")
Signed-off-by: Aleksa Sarai [email protected]