Ensure that modified params file mount does not leak to host

This change ensures that the tmpfs mount created for the modified
NVIDIA params file does not leak to the host.

Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>
Co-authored-by: Evan Lezar <[email protected]>

Author: ArangoGutierrez

cmd/nvidia-cdi-hook/disable-device-node-modification/params_linux.go

@@ -20,39 +20,57 @@
 package disabledevicenodemodification
 
 import (
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
 
+	securejoin "github.com/cyphar/filepath-securejoin"
 	"github.com/opencontainers/runc/libcontainer/utils"
 	"golang.org/x/sys/unix"
 )
 
 func createParamsFileInContainer(containerRootDirPath string, contents []byte) error {
-	tmpRoot, err := os.MkdirTemp("", "nvct-empty-dir*")
-	if err != nil {
-		return fmt.Errorf("failed to create temp root: %w", err)
+	hookScratchDirPath := "/var/run/nvidia-ctk-hook"
+	if err := utils.MkdirAllInRoot(containerRootDirPath, hookScratchDirPath, 0755); err != nil {
+		return fmt.Errorf("error creating hook scratch folder: %w", err)
 	}
 
-	if err := createTmpFs(tmpRoot, len(contents)); err != nil {
-		return fmt.Errorf("failed to create tmpfs mount for params file: %w", err)
-	}
+	err := utils.WithProcfd(containerRootDirPath, hookScratchDirPath, func(hookScratchDirFdPath string) error {
+		return createTmpFs(hookScratchDirFdPath, len(contents))
 
-	modifiedParamsFile, err := os.OpenFile(filepath.Join(tmpRoot, "nvct-params"), os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0444)
+	})
 	if err != nil {
-		return fmt.Errorf("failed to open modified params file: %w", err)
+		return fmt.Errorf("failed to create tmpfs mount for params file: %w", err)
 	}
-	defer modifiedParamsFile.Close()
 
-	if _, err := modifiedParamsFile.Write(contents); err != nil {
-		return fmt.Errorf("failed to write temporary params file: %w", err)
+	modifiedParamsFilePath := filepath.Join(hookScratchDirPath, "nvct-params")
+	if _, err := createFileInRoot(containerRootDirPath, modifiedParamsFilePath, 0444); err != nil {
+		return fmt.Errorf("error creating modified params file: %w", err)
 	}
 
-	err = utils.WithProcfd(containerRootDirPath, nvidiaDriverParamsPath, func(nvidiaDriverParamsFdPath string) error {
-		return unix.Mount(modifiedParamsFile.Name(), nvidiaDriverParamsFdPath, "", unix.MS_BIND|unix.MS_RDONLY|unix.MS_NODEV|unix.MS_PRIVATE|unix.MS_NOSYMFOLLOW, "")
+	err = utils.WithProcfd(containerRootDirPath, modifiedParamsFilePath, func(modifiedParamsFileFdPath string) error {
+		modifiedParamsFile, err := os.OpenFile(modifiedParamsFileFdPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0444)
+		if err != nil {
+			return fmt.Errorf("failed to open modified params file: %w", err)
+		}
+		defer modifiedParamsFile.Close()
+
+		if _, err := modifiedParamsFile.Write(contents); err != nil {
+			return fmt.Errorf("failed to write temporary params file: %w", err)
+		}
+
+		err = utils.WithProcfd(containerRootDirPath, nvidiaDriverParamsPath, func(nvidiaDriverParamsFdPath string) error {
+			return unix.Mount(modifiedParamsFileFdPath, nvidiaDriverParamsFdPath, "", unix.MS_BIND|unix.MS_RDONLY|unix.MS_NODEV|unix.MS_PRIVATE|unix.MS_NOSYMFOLLOW, "")
+		})
+		if err != nil {
+			return fmt.Errorf("failed to mount modified params file: %w", err)
+		}
+
+		return nil
 	})
 	if err != nil {
-		return fmt.Errorf("failed to mount modified params file: %w", err)
+		return err
 	}
 
 	return nil
@@ -61,3 +79,32 @@ func createParamsFileInContainer(containerRootDirPath string, contents []byte) e
 func createTmpFs(target string, size int) error {
 	return unix.Mount("tmpfs", target, "tmpfs", 0, fmt.Sprintf("size=%d", size))
 }
+
+// TODO(ArangoGutierrez): This function also exists in internal/ldconfig we should move this to a separate package.
+func createFileInRoot(containerRootDirPath string, destinationPath string, mode os.FileMode) (string, error) {
+	dest, err := securejoin.SecureJoin(containerRootDirPath, destinationPath)
+	if err != nil {
+		return "", err
+	}
+	// Make the parent directory.
+	destDir, destBase := filepath.Split(dest)
+	destDirFd, err := utils.MkdirAllInRootOpen(containerRootDirPath, destDir, 0755)
+	if err != nil {
+		return "", fmt.Errorf("error creating parent dir: %w", err)
+	}
+	defer destDirFd.Close()
+	// Make the target file. We want to avoid opening any file that is
+	// already there because it could be a "bad" file like an invalid
+	// device or hung tty that might cause a DoS, so we use mknodat.
+	// destBase does not contain any "/" components, and mknodat does
+	// not follow trailing symlinks, so we can safely just call mknodat
+	// here.
+	if err := unix.Mknodat(int(destDirFd.Fd()), destBase, unix.S_IFREG|uint32(mode), 0); err != nil {
+		// If we get EEXIST, there was already an inode there and
+		// we can consider that a success.
+		if !errors.Is(err, unix.EEXIST) {
+			return "", fmt.Errorf("error creating empty file: %w", err)
+		}
+	}
+	return dest, nil
+}

tests/e2e/nvidia-container-toolkit_test.go

@@ -267,6 +267,8 @@ var _ = Describe("docker", Ordered, ContinueOnFailure, func() {
 			Expect(err).ToNot(HaveOccurred())
 
 			tmpDirPath = GinkgoT().TempDir()
+			_, _, err = runner.Run("mkdir -p " + tmpDirPath)
+			Expect(err).ToNot(HaveOccurred())
 
 			output, _, err := runner.Run("mount | sort")
 			Expect(err).ToNot(HaveOccurred())
@@ -280,17 +282,17 @@ var _ = Describe("docker", Ordered, ContinueOnFailure, func() {
 			Expect(mountsAfter).To(Equal(mountsBefore))
 		})
 
-		It("the nvidia-container-runtime-hook should not leak mounts", Label("legacy"), func(ctx context.Context) {
+		It("should not leak mounts when using the nvidia-container-runtime-hook", Label("legacy"), func(ctx context.Context) {
 			_, _, err := runner.Run("docker run --rm -i --runtime=runc -e NVIDIA_VISIBLE_DEVICES=all -e NVIDIA_DRIVER_CAPABILITIES=all --mount type=bind,source=" + tmpDirPath + ",target=/empty,bind-propagation=shared ubuntu true")
 			Expect(err).ToNot(HaveOccurred())
 		})
 
-		It("the nvidia-container-runtime should not leak mounts", func(ctx context.Context) {
+		It("should not leak mounts when using the nvidia-container-runtime", func(ctx context.Context) {
 			_, _, err := runner.Run("docker run --rm -i --runtime=nvidia -e NVIDIA_VISIBLE_DEVICES=all -e NVIDIA_DRIVER_CAPABILITIES=all --mount type=bind,source=" + tmpDirPath + ",target=/empty,bind-propagation=shared ubuntu true")
 			Expect(err).ToNot(HaveOccurred())
 		})
 
-		It("CDI mode should not leak mounts", func(ctx context.Context) {
+		It("should not leak mounts when using CDI mode", func(ctx context.Context) {
 			_, _, err := runner.Run("docker run --rm -i --runtime=nvidia -e NVIDIA_VISIBLE_DEVICES=all -e NVIDIA_DRIVER_CAPABILITIES=runtime.nvidia.com/gpu=all --mount type=bind,source=" + tmpDirPath + ",target=/empty,bind-propagation=shared ubuntu true")
 			Expect(err).ToNot(HaveOccurred())
 		})