libct: switch final WithProcfd users to WithProcfdFile

This probably should've been done as part of commit d40b343
("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.

Signed-off-by: Aleksa Sarai <[email protected]>

Author: cyphar

internal/pathrs/mkdirall_pathrslite.go

@@ -87,13 +87,3 @@ func MkdirAllInRootOpen(root, unsafePath string, mode os.FileMode) (*os.File, er
 		return pathrs.MkdirAllHandle(rootDir, unsafePath, mode)
 	})
 }
-
-// MkdirAllInRoot is a wrapper around MkdirAllInRootOpen which closes the
-// returned handle, for callers that don't need to use it.
-func MkdirAllInRoot(root, unsafePath string, mode os.FileMode) error {
-	f, err := MkdirAllInRootOpen(root, unsafePath, mode)
-	if err == nil {
-		_ = f.Close()
-	}
-	return err
-}

libcontainer/criu_linux.go

@@ -25,6 +25,7 @@ import (
 	"google.golang.org/protobuf/proto"
 
 	"github.com/opencontainers/cgroups"
+	"github.com/opencontainers/runc/internal/pathrs"
 	"github.com/opencontainers/runc/libcontainer/configs"
 	"github.com/opencontainers/runc/libcontainer/utils"
 )
@@ -555,16 +556,22 @@ func (c *Container) prepareCriuRestoreMounts(mounts []*configs.Mount) error {
 	umounts := []string{}
 	defer func() {
 		for _, u := range umounts {
-			_ = utils.WithProcfd(c.config.Rootfs, u, func(procfd string) error {
-				if e := unix.Unmount(procfd, unix.MNT_DETACH); e != nil {
-					if e != unix.EINVAL {
+			mntFile, err := pathrs.OpenInRoot(c.config.Rootfs, u, unix.O_PATH)
+			if err != nil {
+				logrus.Warnf("Error during cleanup unmounting %s: open handle: %v", u, err)
+				continue
+			}
+			_ = utils.WithProcfdFile(mntFile, func(procfd string) error {
+				if err := unix.Unmount(procfd, unix.MNT_DETACH); err != nil {
+					if err != unix.EINVAL {
 						// Ignore EINVAL as it means 'target is not a mount point.'
 						// It probably has already been unmounted.
-						logrus.Warnf("Error during cleanup unmounting of %s (%s): %v", procfd, u, e)
+						logrus.Warnf("Error during cleanup unmounting of %s (%s): %v", procfd, u, err)
 					}
 				}
 				return nil
 			})
+			_ = mntFile.Close()
 		}
 	}()
 	// Now go through all mounts and create the required mountpoints.

libcontainer/rootfs_linux.go

@@ -329,12 +329,15 @@ func mountCgroupV1(m mountEntry, c *mountConfig) error {
 			// We just created the tmpfs, and so we can just use filepath.Join
 			// here (not to mention we want to make sure we create the path
 			// inside the tmpfs, so we don't want to resolve symlinks).
+			// TODO: Why not just use b.Destination (c.root is the root here)?
 			subsystemPath := filepath.Join(c.root, b.Destination)
 			subsystemName := filepath.Base(b.Destination)
-			if err := pathrs.MkdirAllInRoot(c.root, subsystemPath, 0o755); err != nil {
+			subsystemDir, err := pathrs.MkdirAllInRootOpen(c.root, subsystemPath, 0o755)
+			if err != nil {
 				return err
 			}
-			if err := utils.WithProcfd(c.root, b.Destination, func(dstFd string) error {
+			defer subsystemDir.Close()
+			if err := utils.WithProcfdFile(subsystemDir, func(dstFd string) error {
 				flags := defaultMountFlags
 				if m.Flags&unix.MS_RDONLY != 0 {
 					flags = flags | unix.MS_RDONLY
@@ -1454,9 +1457,7 @@ func (m *mountEntry) mountPropagate(rootfs string, mountLabel string) error {
 	_ = m.dstFile.Close()
 	m.dstFile = newDstFile
 
-	// We have to apply mount propagation flags in a separate WithProcfd() call
-	// because the previous call invalidates the passed procfd -- the mount
-	// target needs to be re-opened.
+	// Apply the propagation flags on the new mount.
 	if err := utils.WithProcfdFile(m.dstFile, func(dstFd string) error {
 		for _, pflag := range m.PropagationFlags {
 			if err := mountViaFds("", nil, m.Destination, dstFd, "", uintptr(pflag), ""); err != nil {

libcontainer/utils/utils_unix.go

@@ -6,13 +6,11 @@ import (
 	"fmt"
 	"math"
 	"os"
-	"path/filepath"
 	"runtime"
 	"strconv"
 	"sync"
 	_ "unsafe" // for go:linkname
 
-	securejoin "github.com/cyphar/filepath-securejoin"
 	"github.com/sirupsen/logrus"
 	"golang.org/x/sys/unix"
 
@@ -146,45 +144,13 @@ func NewSockPair(name string) (parent, child *os.File, err error) {
 	return os.NewFile(uintptr(fds[1]), name+"-p"), os.NewFile(uintptr(fds[0]), name+"-c"), nil
 }
 
-// WithProcfd runs the passed closure with a procfd path (/proc/self/fd/...)
-// corresponding to the unsafePath resolved within the root. Before passing the
-// fd, this path is verified to have been inside the root -- so operating on it
-// through the passed fdpath should be safe. Do not access this path through
-// the original path strings, and do not attempt to use the pathname outside of
-// the passed closure (the file handle will be freed once the closure returns).
-func WithProcfd(root, unsafePath string, fn func(procfd string) error) error {
-	// Remove the root then forcefully resolve inside the root.
-	unsafePath = pathrs.LexicallyStripRoot(root, unsafePath)
-	fullPath, err := securejoin.SecureJoin(root, unsafePath)
-	if err != nil {
-		return fmt.Errorf("resolving path inside rootfs failed: %w", err)
-	}
-
-	procSelfFd, closer := ProcThreadSelf("fd/")
-	defer closer()
-
-	// Open the target path.
-	fh, err := os.OpenFile(fullPath, unix.O_PATH|unix.O_CLOEXEC, 0)
-	if err != nil {
-		return fmt.Errorf("open o_path procfd: %w", err)
-	}
-	defer fh.Close()
-
-	procfd := filepath.Join(procSelfFd, strconv.Itoa(int(fh.Fd())))
-	// Double-check the path is the one we expected.
-	if realpath, err := os.Readlink(procfd); err != nil {
-		return fmt.Errorf("procfd verification failed: %w", err)
-	} else if realpath != fullPath {
-		return fmt.Errorf("possibly malicious path detected -- refusing to operate on %s", realpath)
-	}
-
-	return fn(procfd)
-}
-
-// WithProcfdFile is a very minimal wrapper around [ProcThreadSelfFd], intended
-// to make migrating from [WithProcfd] and [WithProcfdPath] usage easier. The
+// WithProcfdFile is a very minimal wrapper around [ProcThreadSelfFd]. The
 // caller is responsible for making sure that the provided file handle is
 // actually safe to operate on.
+//
+// TODO: Migrate the mount logic towards a more move_mount(2)-friendly design
+// where this is kind of /proc/self/... tomfoolery is only done in a fallback
+// path for old kernels.
 func WithProcfdFile(file *os.File, fn func(procfd string) error) error {
 	fdpath, closer := ProcThreadSelfFd(file.Fd())
 	defer closer()