Fix --preserve-fds validation logic.

[mgelde]

The --preserve-fds option can be used to preserve file descriptors and pass them into the container. There is a check that should verify if those file-descriptors are actually open, but its results are tainted by file descriptors opened by Go code after execve and before the check runs. This is also mentioned in #5474.

Currently, running runc with --preserve-fds set to 2 whilst not actually passing any descriptors reproducibly leaks runc-internal file descriptors into the container. Instead, an error message would be more helpful in this situation.

This patch moves the validation logic to a point before any file descriptors related to cgroups-handling or similar are opened by runc.

Closes: #5474

[kolyshkin]

Since Go runtime and runc itself opens all FDs with CLOEXEC, we can just replace unix.Faccessat to unix.Fctnl(F_GETFD) and check there's no CLOEXEC flag, and error out otherwise (basically the check you added to checkPreserveFDs can be done right here).

This will simplify the logic a lot (no cli parsing, no NumFiles etc) without any downsides.

[kolyshkin]

I mean, the fix itself (modulo tests) can be as simple as this:

--- a/utils_linux.go
+++ b/utils_linux.go
@@ -15,7 +15,6 @@ import (
      "github.com/urfave/cli/v3"
      "golang.org/x/sys/unix"

-     "github.com/opencontainers/runc/internal/pathrs"
      "github.com/opencontainers/runc/internal/third_party/systemd/activation"
      "github.com/opencontainers/runc/libcontainer"
      "github.com/opencontainers/runc/libcontainer/configs"
@@ -245,16 +244,15 @@ func (r *runner) run(config *specs.Process) (_ int, retErr error) {
              process.ExtraFiles = append(process.ExtraFiles, r.listenFDs...)
      }
      baseFd := 3 + len(process.ExtraFiles)
-     procSelfFd, closer, err := pathrs.ProcThreadSelfOpen("fd/", unix.O_DIRECTORY|unix.O_CLOEXEC)
-     if err != nil {
-             return -1, err
-     }
-     defer closer()
-     defer procSelfFd.Close()
      for i := baseFd; i < baseFd+r.preserveFDs; i++ {
-             err := unix.Faccessat(int(procSelfFd.Fd()), strconv.Itoa(i), unix.F_OK, 0)
-             if err != nil {
-                     return -1, fmt.Errorf("unable to stat preserved-fd %d (of %d): %w", i-baseFd, r.preserveFDs, err)
+             // Check that the fd was really inherited from runc's caller. Merely
+             // checking that the fd is open is not sufficient, as the fd number
+             // could have been reused by runc itself (or the Go runtime). Any such
+             // fd has the close-on-exec flag set, while an inherited one can not
+             // have it, as it would have been closed by execve.
+             flags, err := unix.FcntlInt(uintptr(i), unix.F_GETFD, 0)
+             if err != nil || flags&unix.FD_CLOEXEC != 0 {
+                     return -1, fmt.Errorf("preserved-fd %d (of %d) was not passed to runc", i-baseFd, r.preserveFDs)
              }
              process.ExtraFiles = append(process.ExtraFiles, os.NewFile(uintptr(i), "PreserveFD:"+strconv.Itoa(i)))
      }

(note I simplified the error message as well)

[mgelde]

I see no way that should fail, as long as all those FDs really always set CLOEXEC; now and forever AND this check continues to happen both before execve AND before joining the container namespaces e.g. during runc run --preserve-fds=X (otherwise we'd have a race). At present that appears to be the case.

So if that is how it's done (i.e. CLOEXEC is always set), I agree the extra safety of checking early does not outweigh the added complexity.

I'll push an updated version.

[kolyshkin]

nit: this is fcntl not stat

[mgelde]

Fixed that as well.

[kolyshkin]

looks like integration tests fail. Also, can you add Co-authored-by: Kir Kolyshkin <kolyshkin@gmail.com>?

[mgelde]

Yes, I somehow cannot get the integration tests to run locally. They always fail, even on main. Even in a VM with another Linux distribution I could not get them to run. So I hope what I gather from the runner logs here will suffice.

From what I can see in the runner results, the failures are due to the changed position of the check:

• The log exists after the failed run (test checked for absence). The check now occurs later than previously and the log file is now created before the check.
• and the create test fails because another check preempts ours and terminates the call with another error message than what the test expected. At least what I was able to reproduce locally points that way.

I have pushed another version in the hopes that fixes it. For now I cannot spend more time on getting those tests to run locally. If this patch does not resolve the issue, I'll revisit.