From f1d32f2cd3bc0eef09d84256d6cb5ead2675d460 Mon Sep 17 00:00:00 2001 From: Ievgen Popovych Date: Sun, 19 Nov 2023 23:22:59 +0200 Subject: [PATCH 1/3] nvidia-ctk hook chmod: Only chmod if desired permissions are different This is to avoid any unnecessary potential errors (e.g. due to permissions). Fixes: #143 Signed-off-by: Ievgen Popovych --- cmd/nvidia-ctk/hook/chmod/chmod.go | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/cmd/nvidia-ctk/hook/chmod/chmod.go b/cmd/nvidia-ctk/hook/chmod/chmod.go index 90ea8107..8dbdf86a 100644 --- a/cmd/nvidia-ctk/hook/chmod/chmod.go +++ b/cmd/nvidia-ctk/hook/chmod/chmod.go @@ -18,8 +18,10 @@ package chmod import ( "fmt" + "io/fs" "os" "path/filepath" + "strconv" "strings" "syscall" @@ -112,7 +114,13 @@ func (m command) run(c *cli.Context, cfg *config) error { return fmt.Errorf("empty container root detected") } - paths := m.getPaths(containerRoot, cfg.paths.Value()) + desiredModeInt, err := strconv.ParseUint(cfg.mode, 8, 32) + if err != nil { + return fmt.Errorf("failed to parse mode as octal: %v", err) + } + desiredMode := fs.FileMode(desiredModeInt) + + paths := m.getPaths(containerRoot, cfg.paths.Value(), desiredMode) if len(paths) == 0 { m.logger.Debugf("No paths specified; exiting") return nil @@ -132,14 +140,19 @@ func (m command) run(c *cli.Context, cfg *config) error { } // getPaths updates the specified paths relative to the root. -func (m command) getPaths(root string, paths []string) []string { +func (m command) getPaths(root string, paths []string, desiredMode fs.FileMode) []string { var pathsInRoot []string for _, f := range paths { path := filepath.Join(root, f) - if _, err := os.Stat(path); err != nil { + stat, err := os.Stat(path) + if err != nil { m.logger.Debugf("Skipping path %q: %v", path, err) continue } + if (stat.Mode()&(fs.ModePerm|fs.ModeSetuid|fs.ModeSetgid|fs.ModeSticky))^desiredMode == 0 { + m.logger.Debugf("Skipping path %q: already desired mode", path) + continue + } pathsInRoot = append(pathsInRoot, path) } From eb35d9b30a29f37244be3bf828dcbb1f86f02371 Mon Sep 17 00:00:00 2001 From: Ievgen Popovych Date: Sun, 19 Nov 2023 23:15:32 +0200 Subject: [PATCH 2/3] nvidia-ctk hook chmod: Ignore permission errors In some cases we might get a permission error trying to chmod - most likely this is due to something beyond our control like whole `/dev` being mounted. Do not fail container creation in this case. Due to loosing control of the program after `exec()`-ing `chmod(1)` program and therefore not being able to handle errors - refactor to use `chmod(2)` syscall instead of `exec()` `chmod(1)` program. Fixes: #143 Signed-off-by: Ievgen Popovych --- cmd/nvidia-ctk/hook/chmod/chmod.go | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/cmd/nvidia-ctk/hook/chmod/chmod.go b/cmd/nvidia-ctk/hook/chmod/chmod.go index 8dbdf86a..90b4968f 100644 --- a/cmd/nvidia-ctk/hook/chmod/chmod.go +++ b/cmd/nvidia-ctk/hook/chmod/chmod.go @@ -17,16 +17,15 @@ package chmod import ( + "errors" "fmt" "io/fs" "os" "path/filepath" "strconv" "strings" - "syscall" "github.com/NVIDIA/nvidia-container-toolkit/internal/logger" - "github.com/NVIDIA/nvidia-container-toolkit/internal/lookup" "github.com/NVIDIA/nvidia-container-toolkit/internal/oci" "github.com/urfave/cli/v2" ) @@ -126,17 +125,16 @@ func (m command) run(c *cli.Context, cfg *config) error { return nil } - locator := lookup.NewExecutableLocator(m.logger, "") - targets, err := locator.Locate("chmod") - if err != nil { - return fmt.Errorf("failed to locate chmod: %v", err) + for _, path := range paths { + err = os.Chmod(path, desiredMode) + // in some cases this is not an issue (e.g. whole /dev mounted), see #143 + if errors.Is(err, fs.ErrPermission) { + m.logger.Debugf("Ignoring permission error with chmod: %v", err) + err = nil + } } - chmodPath := targets[0] - args := append([]string{filepath.Base(chmodPath), cfg.mode}, paths...) - - //nolint:gosec // TODO: Can we harden this so that there is less risk of command injection - return syscall.Exec(chmodPath, args, nil) + return err } // getPaths updates the specified paths relative to the root. From 9085cb7dd56c36127e0574c0ba968c9154cbbc59 Mon Sep 17 00:00:00 2001 From: Ievgen Popovych Date: Mon, 20 Nov 2023 14:49:29 +0200 Subject: [PATCH 3/3] nvidia-ctk hook chmod: Move file mode parsing into flag validation function Signed-off-by: Ievgen Popovych --- cmd/nvidia-ctk/hook/chmod/chmod.go | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/cmd/nvidia-ctk/hook/chmod/chmod.go b/cmd/nvidia-ctk/hook/chmod/chmod.go index 90b4968f..0caa37b9 100644 --- a/cmd/nvidia-ctk/hook/chmod/chmod.go +++ b/cmd/nvidia-ctk/hook/chmod/chmod.go @@ -36,7 +36,8 @@ type command struct { type config struct { paths cli.StringSlice - mode string + modeStr string + mode fs.FileMode containerSpec string } @@ -73,7 +74,7 @@ func (m command) build() *cli.Command { &cli.StringFlag{ Name: "mode", Usage: "Specify the file mode", - Destination: &cfg.mode, + Destination: &cfg.modeStr, }, &cli.StringFlag{ Name: "container-spec", @@ -86,10 +87,16 @@ func (m command) build() *cli.Command { } func validateFlags(c *cli.Context, cfg *config) error { - if strings.TrimSpace(cfg.mode) == "" { + if strings.TrimSpace(cfg.modeStr) == "" { return fmt.Errorf("a non-empty mode must be specified") } + modeInt, err := strconv.ParseUint(cfg.modeStr, 8, 32) + if err != nil { + return fmt.Errorf("failed to parse mode as octal: %v", err) + } + cfg.mode = fs.FileMode(modeInt) + for _, p := range cfg.paths.Value() { if strings.TrimSpace(p) == "" { return fmt.Errorf("paths must not be empty") @@ -113,20 +120,14 @@ func (m command) run(c *cli.Context, cfg *config) error { return fmt.Errorf("empty container root detected") } - desiredModeInt, err := strconv.ParseUint(cfg.mode, 8, 32) - if err != nil { - return fmt.Errorf("failed to parse mode as octal: %v", err) - } - desiredMode := fs.FileMode(desiredModeInt) - - paths := m.getPaths(containerRoot, cfg.paths.Value(), desiredMode) + paths := m.getPaths(containerRoot, cfg.paths.Value(), cfg.mode) if len(paths) == 0 { m.logger.Debugf("No paths specified; exiting") return nil } for _, path := range paths { - err = os.Chmod(path, desiredMode) + err = os.Chmod(path, cfg.mode) // in some cases this is not an issue (e.g. whole /dev mounted), see #143 if errors.Is(err, fs.ErrPermission) { m.logger.Debugf("Ignoring permission error with chmod: %v", err)