As with /dev symlinks, this was missed in commit d40b3439a9 ("rootfs:
switch to fd-based handling of mountpoint targets"). It's not really
clear to what extent this was exploitable (/sys/fs/cgroup is a tmpfs we
create) but it's better to just fix this anyway.
Fixes: d40b3439a9 ("rootfs: switch to fd-based handling of mountpoint targets")
Signed-off-by: Aleksa Sarai <aleksa@amutable.com>
(cherry picked from commit 66acd48f9d)
Signed-off-by: Aleksa Sarai <aleksa@amutable.com>
These codepaths are very old and operate on pure paths but before
pivot_root(2), meaning that a bad image with a malicious /dev symlink
could cause us to operate on host paths instead.
In practice this means that we could be tricked into removing a file
called "ptmx" (note that /dev/pts/ptmx and /dev/ptmx are both immune for
different reasons) or creating a very restricted set of symlinks (with
fixed targets and names). The scope of these bugs is thus quite limited,
but we definitely need to harden against it.
These codepaths were unfortunately missed during the fd-based rework in
commit d40b3439a9 ("rootfs: switch to fd-based handling of mountpoint
targets") -- I must've assumed they were called after pivot_root(2)...
Fixes: GHSA-xjvp-4fhw-gc47
Fixes: CVE-2026-41579
Fixes: d40b3439a9 ("rootfs: switch to fd-based handling of mountpoint targets")
Signed-off-by: Aleksa Sarai <aleksa@amutable.com>
(cherry picked from commit 864db8042d)
Signed-off-by: Aleksa Sarai <aleksa@amutable.com>
This argument order matches most other helpers we have and will also
match the changes we are about to make to setupPtmx and
setupDevSymlinks.
Signed-off-by: Aleksa Sarai <aleksa@amutable.com>
(cherry picked from commit fcf04eb41b)
Signed-off-by: Aleksa Sarai <aleksa@amutable.com>
The test checked for the exact BusyBox ash diagnostic "sh: can't fork".
With BusyBox 1.38, ash reports the failure as:
/bin/sh: line 0: can't fork: Resource temporarily unavailable
Match the stable "can't fork" part of the error message instead.
Signed-off-by: Ricardo Branco <rbranco@suse.de>
(cherry picked from commit de39d5e79b)
Signed-off-by: Ricardo Branco <rbranco@suse.de>
TestPids used long hand-written /bin/true pipelines for the 4-, 32- and
64-command cases. This made the test easy to typo and hard to review, as
seen by the earlier "bin/true" entries.
Build the shell pipelines instead, preserving the existing test coverage
while making the command counts explicit.
Signed-off-by: Ricardo Branco <rbranco@suse.de>
(cherry picked from commit 3acb097f93)
Signed-off-by: Ricardo Branco <rbranco@suse.de>
Close the root file descriptor immediately after use in maskPaths to
reduce the window during which an attacker could potentially exploit
an open fd to access or manipulate the root filesystem. This follows
the principle of least privilege and mitigates risks in compromised
or malicious container scenarios.
Co-authored-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: lifubang <lifubang@acmcoder.com>
(cherry picked from commit b88635e57e)
Signed-off-by: lifubang <lifubang@acmcoder.com>
This is a follow-up to #5275. That change reused a single tmpfs mount
to mask multiple directories, which is efficient when masking more than
one path. However, it introduced unnecessary overhead when only one
directory is masked. This commit restores the original behavior for the
single-path case while preserving shared tmpfs logic for multiple paths.
Signed-off-by: lifubang <lifubang@acmcoder.com>
(cherry picked from commit e7e2f00248)
Signed-off-by: lifubang <lifubang@acmcoder.com>
Kubernetes may add one sysfs thermal_throttle entry per CPU to
maskedPaths. On large Intel systems this can produce many directory
masks for a single container. runc currently handles each directory
mask with a separate read-only tmpfs mount, and therefore a separate
tmpfs superblock.
On Linux 4.18/RHEL 8 kernels, creating and tearing down many tmpfs
superblocks can contend on the global shrinker_rwsem when containers
start or stop concurrently.
Use one read-only tmpfs for directory masks and bind-mount it over the
remaining directory targets. The first non-procfs-fd directory mount is
reopened through the container root fd before it is reused. File masks
still bind /dev/null, and procfs fd targets keep the existing
one-tmpfs-per-target behaviour because they are fd aliases rather than
stable rootfs paths.
If the bind-mount of the shared source fails (e.g. due to kernel
restrictions), fall back to individual tmpfs mounts for all remaining
directories. Tmpfs mounts use nr_blocks=1,nr_inodes=1 to minimise
kernel resource usage.
The bind mounts do not create additional tmpfs superblocks. They also
retain the read-only mount flag inherited from the source vfsmount, so
the masking semantics remain unchanged.
xref: kubernetes/kubernetes#138512
xref: kubernetes/kubernetes#138388
xref: kubernetes/kubernetes#131018
Co-authored-by: Davanum Srinivas <davanum@gmail.com>
Refactored-by: lifubang <lifubang@acmcoder.com>
Signed-off-by: lifubang <lifubang@acmcoder.com>
(cherry picked from commit c046c9b973)
Signed-off-by: lifubang <lifubang@acmcoder.com>
Previously, masked directories (e.g., /proc/acpi, /proc/scsi) were
mounted as read-only tmpfs without explicit size or inode limits.
Although these mounts are meant to be empty and unwritable, the lack
of resource constraints means that—should an attacker bypass the
read-only protection (e.g., via container escape, mount namespace
manipulation, or a kernel vulnerability)—the tmpfs could consume up
to 50% of system memory by default (the kernel's default tmpfs limit).
To mitigate this risk in high-density container environments and
adhere to the principle of least privilege, we now explicitly set:
- nr_blocks=1 (sufficient for at most one block size)
- nr_inodes=1 (sufficient for at most one inode)
Ref: https://man7.org/linux/man-pages/man5/tmpfs.5.html
These limits ensure that even if compromised, kernel memory usage
remains strictly bounded and negligible.
This change aligns with best practices used by other container
runtimes and strengthens defense-in-depth for sensitive masked paths.
Co-authored-by: Davanum Srinivas <davanum@gmail.com>
Refactored-by: lifubang <lifubang@acmcoder.com>
Signed-off-by: lifubang <lifubang@acmcoder.com>
(cherry picked from commit e57a7a4c8f)
Signed-off-by: lifubang <lifubang@acmcoder.com>
A lot of filesystem-related stuff happens inside the container root
directory, and we have used its name before. It makes sense to pre-open
it and use a *os.File handle instead.
Function names in internal/pathrs are kept as is for simplicity (and it
is an internal package), but they now accept root as *os.File.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 28cb321887)
Signed-off-by: lifubang <lifubang@acmcoder.com>
No change in functionality, just a preparation for the next patch.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 78b80677f6)
Signed-off-by: lifubang <lifubang@acmcoder.com>
Indeed, it does not make sense to prepend c.root once we started using
MkdirAllInRoot in commit 63c29081.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 60352524d3)
Signed-off-by: lifubang <lifubang@acmcoder.com>
This commit factors out setupAndMountToRootfs without changing any
logic. Use "Hide whitespace changes" during review to focus on the
actual changes.
The refactor ensures the mount source file descriptor is closed via
defer in each loop iteration, reducing the total number of open FDs
in runc. This helps avoid hitting the file descriptor limit under
high concurrency or when handling many mounts.
Signed-off-by: lifubang <lifubang@acmcoder.com>
(cherry picked from commit c77e71a3e7)
Signed-off-by: lifubang <lifubang@acmcoder.com>
This function calls Init what normally never returns, so the defer only
works if there is an error and we can safely use it to close those fds
we opened. This was done for most but not all fds.
Reported in issue 5008.
Reported-by: Arina Cherednik <arinacherednik034@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 88f897160c)
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
There is a chance of panic here -- eliminate it.
Add a test case (which panics before the fix).
Reported-by: Luke Hinds <luke@stacklok.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit a48a7cef96)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Use t.Run for individual tests. Add missing desc fields.
Best reviewed with --ignore-all-space.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 392a221293)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This fixes random failures to start a container in conmon integration
tests (see issue 5151).
I guess we need to find another way to fix issue 4645.
This reverts commit 1b39997e73.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 5996fe143a)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
unix.CPUSet is limited to 1024 CPUs. Calling
unix.SchedSetaffinity(pid, cpuset) removes all CPUs starting from 1024
from allowed CPUs of pid, even if cpuset is all ones. As a
consequence, when runc tries to reset CPU affinity to "allow all" by
default, it prevents all containers from CPUs 1024 onwards.
This change uses a huge CPU mask to play safe and get all possible
CPUs enabled with a single sched_setaffinity call.
Fixes: #5023
Signed-off-by: Antti Kervinen <antti.kervinen@intel.com>
(cherry picked from commit 700c944c4d)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Previously, when prepareCgroupFD would not open container's cgroup
(as configured in config.json and saved to state.json), it returned
a fatal error, as we presumed a container can't exist without its own
cgroup.
Apparently, it can. In a case when container is configured without
cgroupns (i.e. it uses hosts cgroups), and /sys/fs/cgroup is mounted
read-write, a rootful container's init can move itself to an entirely
different cgroup (even a new one that it just created), and then the
original container cgroup is removed by the kernel (or systemd?) as
it has no processes left. By the way, from the systemd point of view
the container is gone. And yet it is still there, and users want
runc exec to work!
And it worked, thanks to the "let's try container init's cgroup"
fallback as added by commit c91fe9aeba ("cgroup2: exec: join the
cgroup of the init process on EBUSY"). The fallback was added for
the entirely different reason, but it happened to work in this very
case, too.
This behavior was broken with the introduction of CLONE_INTO_CGROUP
support.
While it is debatable whether this is a valid scenario when a container
moves itself into a different cgroup, this very setup is used by e.g.
buildkitd running in a privileged kubernetes container (see issue 5089).
To restore the way things are expected to work, add the same "try
container init's cgroup" fallback into prepareCgroupFD.
While at it, simplify the code flow.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 6c07a37a58)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. Refactor addIntoCgroupV2 in an attempt to simplify it.
2. Fix the bug of not trying the init cgroup fallback if
rootlessCgroup is set. This is a bug because rootlessCgroup
tells to ignore cgroup join errors, not to never try the fallback.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 1d030fab7d)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Separate initProcessCgroupPath code out of addIntoCgroupV2.
To be used by the next patch.
While at it, describe the new scenario in which the container's
configured cgroup might not be available.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 94133fab97)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since [PR 4812], runc exec tries to use clone3 syscall with
CLONE_INTO_CGROUP, falling back to the old method if it is not
supported.
One issue with that approach is, a
> Cmd cannot be reused after calling its [Cmd.Start], [Cmd.Run],
> [Cmd.Output], or [Cmd.CombinedOutput] methods.
(from https://pkg.go.dev/os/exec#Cmd).
This is enforced since Go 1.26, see [CL 728642], and so runc exec
actually fails in specific scenarios (go1.26 and no CLONE_INTO_CGROUP
support).
The easiest workaround is to pre-copy the p.cmd structure (copy = *cmd).
From the [CL 734200] it looks like it is an acceptable way, but it might
break in the future as it also copies the private fields, so let's do a
proper field-by-field copy. If the upstream will add cmd.Clone method,
we will switch to it.
Also, we can probably be fine with a post-copy (once the first Start has
failed), but let's be conservative here and do a pre-copy.
[PR 4812]: https://github.com/opencontainers/runc/pull/4812
[CL 728642]: https://go.dev/cl/728642
[CL 734200]: https://go.dev/cl/734200
Reported-by: Efim Verzakov <efimverzakov@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit cb31d62f1c)
Signed-off-by: lifubang <lifubang@acmcoder.com>
Theoretically, exec.Command can set cmd.Err.
Practically, this should never happen (Linux, Go <= 1.26, exePath is
absolute), but in the unlikely case it does, let's fail early.
This is related to the cloneCmd (to be introduced by the following
commit) which chooses to not copy the Err field. Theoretically,
exec.Command can set Err and so the first call to cmd.Start will fail
(since Err != nil), and the second call to cmd.Start may succeed because
Err == nil. Yet, this scenario is highly unlikely, but better be safe
than sorry.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 82b7597a26)
Signed-off-by: lifubang <lifubang@acmcoder.com>
When parsing mount options into recAttrSet and recAttrClr,
the code sets attr_clr to individual atime flags (e.g.
MOUNT_ATTR_NOATIME or MOUNT_ATTR_STRICTATIME) when clearing
atime attributes. However, this violates the kernel's
requirement documented in mount_setattr(2)[1]:
> Note that, since the access-time values are an enumeration
> rather than bit values, a caller wanting to transition to a
> different access-time setting cannot simply specify the
> access-time setting in attr_set, but must also include
> MOUNT_ATTR__ATIME in the attr_clr field. The kernel will
> verify that MOUNT_ATTR__ATIME isn't partially set in
> attr_clr (i.e., either all bits in the MOUNT_ATTR__ATIME
> bit field are either set or clear), and that attr_set
> doesn't have any access-time bits set if MOUNT_ATTR__ATIME
> isn't set in attr_clr.
Passing only a single atime flag (e.g. MOUNT_ATTR_RELATIME) in
attr_clr causes mount_setattr() to fail with EINVAL.
This change ensures that whenever an atime mode is updated,
attr_clr includes MOUNT_ATTR__ATIME to properly reset the
entire access-time attribute field before applying the new mode.
[1] https://man7.org/linux/man-pages/man2/mount_setattr.2.html
Signed-off-by: lifubang <lifubang@acmcoder.com>
(cherry picked from commit 5560d55bfd)
Signed-off-by: lifubang <lifubang@acmcoder.com>
Alas, these new constants are already in v1.4.0 release so we can't
remove those right away, but we can mark them as deprecated now
and target removal for v1.5.0.
So,
- mark them as deprecated;
- redefine via unix.MPOL_* counterparts;
- fix the validator code to use unix.MPOL_* directly.
This amends commit a0e809a8.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 3741f9186d)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This is mostly a mechanical change, but we also need to change some
types to match the "mode int" argument that golang.org/x/sys/unix
decided to use.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
(cherry picked from commit a0e809a8ba)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit b2f8a74d "clothed" the naked return as inflicted by gofumpt
v0.9.0. Since gofumpt v0.9.2 this rule was moved to "extra" category,
not enabled by default. The only other "extra" rule is to group adjacent
parameters with the same type, which also makes sense.
Enable gofumpt "extra" rules, and reformat the code accordingly.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 67840cce4b)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The commit mentioned below has missed these changes.
Fixes: 17570625 ("Use for range over integers")
Signed-off-by: Ariel Otilibili <otilibil@eurecom.fr>
(cherry picked from commit 34da991298)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In case early stage of runc init (nsenter) fails for some reason, it
logs error(s) with FATAL log level, via bail().
The runc init log is read by a parent (runc create/run/exec) and is
logged via normal logrus mechanism, which is all fine and dandy, except
when `runc init` fails, we return the error from the parent (which is
usually not too helpful, for example):
runc run failed: unable to start container process: can't get final child's PID from pipe: EOF
Now, the actual underlying error is from runc init and it was logged
earlier; here's how full runc output looks like:
FATA[0000] nsexec-1[3247792]: failed to unshare remaining namespaces: No space left on device
FATA[0000] nsexec-0[3247790]: failed to sync with stage-1: next state
ERRO[0000] runc run failed: unable to start container process: can't get final child's PID from pipe: EOF
The problem is, upper level runtimes tend to ignore everything except
the last line from runc, and thus error reported by e.g. docker is not
very helpful.
This patch tries to improve the situation by collecting FATAL errors
from runc init and appending those to the error returned (instead of
logging). With it, the above error will look like this:
ERRO[0000] runc run failed: unable to start container process: can't get final child's PID from pipe: EOF; runc init error(s): nsexec-1[141549]: failed to unshare remaining namespaces: No space left on device; nsexec-0[141547]: failed to sync with stage-1: next state
Yes, it is long and ugly, but at least the upper level runtime will
report it.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit f944ccecb2)
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Introduce and use iobail, xread, and xwrite wrappers so that we can
properly check read/write return value and call either bail or bailx on
error, with proper diagnostics (distinguishing failed read/write from a
short read/write).
This prevents the "Success" prefix in errors like:
failed to sync with stage-1: next state: Success
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 6c18b25cdc)
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Add a few missing sane_kill calls where they make sense.
Remove one useless sane_kill of stage2_pid, as during SYNC_USERMAP stage2
is not yet started. It is harmless yet it makes the code slightly harder
to read.
Set the child pid to -1 upon receiving SYNC_CHILD_FINISH
to minimize the chances of killing an unrelated process.
When a child sends SYNC_CHILD_FINISH it is about to exit
(although theoretically it could be stuck during debug logging).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit aea52d0ab0)
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
We use bail to report fatal errors, and bail always append %m
(aka strerror(errno)). In case an error condition did not set
errno, the log message will end up with ": Success" or an error
from a stale errno value. Either case is confusing for users.
Introduce bailx which is the same as bail except it does not
append %m, and use it where appropriate.
The naming follows libc's err(3) and errx(3).
PS we still use bail in a few cases after read or write, even
if that read/write did not return an error, because the code
does not distinguish between short read/write and error (-1).
This will be addressed by the next commit.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 067b8335e7)
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Since sane_kill after a failed read or write, but before reporting the
error from that read or write, it may change the errno value in case
kill(2) fails.
Save and restore the errno around the call to kill.
While at it,
- change the code to return early;
- don't return kill return value as no one is using it, and the errno
value no longer correlates.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 9c8f476cb6)
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Since the recent CVE fixes, TestFdLeaksSystemd sometimes fails:
=== RUN TestFdLeaksSystemd
exec_test.go:1750: extra fd 9 -> /12224/task/13831/fd
exec_test.go:1753: found 1 extra fds after container.Run
--- FAIL: TestFdLeaksSystemd (0.10s)
It might have been caused by the change to the test code in commit
ff6fe13 ("utils: use safe procfs for /proc/self/fd loop code") -- we are
now opening a file descriptor during the logic to get a list of file
descriptors. If the file descriptor happens to be allocated to a
different number, you'll get an error.
Let's try to filter out the fd used to read a directory.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 5fbc3bb019)
Signed-off-by: lifubang <lifubang@acmcoder.com>
While CreateInRoot supports hallucinating the target path, we do not use
it directly when constructing device inode targets because we need to
have different handling for mknod and bind-mounts.
The solution is to simply have a more generic MkdirAllParentInRoot
helper that MkdirAll's the parent directory of the target path and then
allows the caller to create the trailing component however they like.
(This can be used by CreateInRoot internally as well!)
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
(cherry picked from commit 195e9551e4)
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
In order to maintain compatibility with previous releases of runc (which
permitted dangling symlinks as path components by permitting
non-existent path components to be treated like real directories) we
have to first do SecureJoin to construct a target path that is
compatible with the old behaviour but has all dangling symlinks (or
other invalid paths like ".." components after non-existent directories)
removed.
This is effectively a more generic verison of commit 3f925525b4
("rootfs: re-allow dangling symlinks in mount targets") and will let us
remove the need for open-coding SecureJoin workarounds.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
(cherry picked from commit cfb74326be)
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Now that MkdirAllInRoot has been removed, we can make MkdirAllInRootOpen
less wordy by renaming it to MkdirAllInRoot. This is a non-functional
change.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
(cherry picked from commit 20c5a8ec4a)
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
This probably should've been done as part of commit d40b3439a9
("rootfs: switch to fd-based handling of mountpoint targets") but it
seems I missed them when doing the rest of the conversions.
This also lets us remove utils.WithProcfd entirely, as well as
pathrs.MkdirAllInRoot. Unfortunately, WithProcfd was exposed in the
externally-importable "libcontainer/utils" package and so we need to
have a deprecation notice to remove it in runc 1.5.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
(cherry picked from commit 9dbd37e06f)
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
These helpers will be needed for the compatibility code added in future
patches in this series, but because "internal/pathrs" is imported by
"libcontainer/utils" we need to move them so that we can avoid circular
dependencies.
Because the old functions were in a non-internal package it is possible
some downstreams use them, so add some wrappers but mark them as
deprecated.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
(cherry picked from commit 42a1e19d67)
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Aleksa Sarai