From b7fdb688483a628a1484248ff5b7f4c559f17950 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Fri, 21 Jan 2022 17:35:52 -0800 Subject: [PATCH 1/3] libct: fixStdioPermissions: minor refactoring Use os/file Chown method instead of bare unix.Fchown as it already have access to underlying fd, and produces nice-looking errors. This allows us to remove our error wrapping and some linter annotations. We still use unix.Fstat since os.Stat access to os-specific fields like uid/gid is not very straightforward. The only change here is to use file name (rather than fd) in the error text. Signed-off-by: Kir Kolyshkin --- libcontainer/init_linux.go | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/libcontainer/init_linux.go b/libcontainer/init_linux.go index cb862a6a5..3db96d66d 100644 --- a/libcontainer/init_linux.go +++ b/libcontainer/init_linux.go @@ -8,7 +8,6 @@ import ( "io" "net" "os" - "strconv" "strings" "unsafe" @@ -406,14 +405,10 @@ func fixStdioPermissions(u *user.ExecUser) error { if err := unix.Stat("/dev/null", &null); err != nil { return &os.PathError{Op: "stat", Path: "/dev/null", Err: err} } - for _, fd := range []uintptr{ - os.Stdin.Fd(), - os.Stderr.Fd(), - os.Stdout.Fd(), - } { + for _, file := range []*os.File{os.Stdin, os.Stdout, os.Stderr} { var s unix.Stat_t - if err := unix.Fstat(int(fd), &s); err != nil { - return &os.PathError{Op: "fstat", Path: "fd " + strconv.Itoa(int(fd)), Err: err} + if err := unix.Fstat(int(file.Fd()), &s); err != nil { + return &os.PathError{Op: "fstat", Path: file.Name(), Err: err} } // Skip chown of /dev/null if it was used as one of the STDIO fds. @@ -427,7 +422,7 @@ func fixStdioPermissions(u *user.ExecUser) error { // that users expect to be able to actually use their console. Without // this code, you couldn't effectively run as a non-root user inside a // container and also have a console set up. - if err := unix.Fchown(int(fd), u.Uid, int(s.Gid)); err != nil { + if err := file.Chown(u.Uid, int(s.Gid)); err != nil { // If we've hit an EINVAL then s.Gid isn't mapped in the user // namespace. If we've hit an EPERM then the inode's current owner // is not mapped in our user namespace (in particular, @@ -435,11 +430,10 @@ func fixStdioPermissions(u *user.ExecUser) error { // are in a configuration where it's better for us to just not // touch the stdio rather than bail at this point. - // nolint:errorlint // unix errors are bare - if err == unix.EINVAL || err == unix.EPERM { + if errors.Is(err, unix.EINVAL) || errors.Is(err, unix.EPERM) { continue } - return &os.PathError{Op: "fchown", Path: "fd " + strconv.Itoa(int(fd)), Err: err} + return err } } return nil From 18c4760aeda1ecade2711c246d092fad2568eb90 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Fri, 21 Jan 2022 17:43:09 -0800 Subject: [PATCH 2/3] libct: fixStdioPermissions: skip chown if not needed Since we already called fstat, we know the current file uid. In case it is the same as the one we want it to be, there's no point in trying chown. Remove the specific /dev/null check, as the above also covers it (comparing /dev/null uid with itself is true). This also fixes runc exec with read-only /dev for root user. Signed-off-by: Kir Kolyshkin --- libcontainer/init_linux.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/libcontainer/init_linux.go b/libcontainer/init_linux.go index 3db96d66d..2d7af1d4d 100644 --- a/libcontainer/init_linux.go +++ b/libcontainer/init_linux.go @@ -411,12 +411,12 @@ func fixStdioPermissions(u *user.ExecUser) error { return &os.PathError{Op: "fstat", Path: file.Name(), Err: err} } - // Skip chown of /dev/null if it was used as one of the STDIO fds. - if s.Rdev == null.Rdev { + // Skip chown if uid is already the one we want. + if int(s.Uid) == u.Uid { continue } - // We only change the uid owner (as it is possible for the mount to + // We only change the uid (as it is possible for the mount to // prefer a different gid, and there's no reason for us to change it). // The reason why we don't just leave the default uid=X mount setup is // that users expect to be able to actually use their console. Without From 146c8c0c62dc30a08eb5ff302217d91dfd29d158 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Fri, 21 Jan 2022 17:53:03 -0800 Subject: [PATCH 3/3] libct: fixStdioPermissions: ignore EROFS In case of a read-only /dev, it's better to move on and let whatever is run in a container to handle any possible errors. This solves runc exec for a user with read-only /dev. Signed-off-by: Kir Kolyshkin --- libcontainer/init_linux.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/libcontainer/init_linux.go b/libcontainer/init_linux.go index 2d7af1d4d..1e5c394c3 100644 --- a/libcontainer/init_linux.go +++ b/libcontainer/init_linux.go @@ -426,11 +426,12 @@ func fixStdioPermissions(u *user.ExecUser) error { // If we've hit an EINVAL then s.Gid isn't mapped in the user // namespace. If we've hit an EPERM then the inode's current owner // is not mapped in our user namespace (in particular, - // privileged_wrt_inode_uidgid() has failed). In either case, we - // are in a configuration where it's better for us to just not - // touch the stdio rather than bail at this point. + // privileged_wrt_inode_uidgid() has failed). Read-only + // /dev can result in EROFS error. In any case, it's + // better for us to just not touch the stdio rather + // than bail at this point. - if errors.Is(err, unix.EINVAL) || errors.Is(err, unix.EPERM) { + if errors.Is(err, unix.EINVAL) || errors.Is(err, unix.EPERM) || errors.Is(err, unix.EROFS) { continue } return err