Fix for Sysbox issue #712.

ctalledo · ctalledo · commit 9cf74e4cbf3a · 2023-11-19T16:22:29.000-08:00
The problem manifested itself when Docker Engine (&gt;= v24.0), running inside a
Sysbox container, created a bind-mount using a source path such as
/proc/self/task/&lt;tid&gt;/ns/net. That mount unexpectedly failed with EPERM.

Turns out the problem was caused by a bug in sysbox-fs' mount syscall
interception, where it was not properly resolving mount paths such as
"/proc/self/task/&lt;tid&gt;/...". The reason is that &lt;tid&gt; is a thread-ID in the
container's pid-namespace, not in sysbox's pid-namespace, so sysbox-fs could not
resolve it properly in function process.ResolveProcSelf() (i.e., when it did
stat() on the path it failed).

I've not found a proper way to translate the &lt;tid&gt; from the container's pid-ns
to Sysbox's pid-ns. That would have been the proper fix.

For now, this commit works around the problem by assuming that &lt;tid&gt; = &lt;pid&gt;;
that's not ideal, but it's usually (likely always) the case for mount syscalls
we normally intercept.

Signed-off-by: Cesar Talledo &lt;cesar.talledo@docker.com&gt;
diff --git a/process/process.go b/process/process.go
@@ -632,6 +632,23 @@ func (p *process) getStatus(fields []string) error {
 	return nil
 }
 
+// Replaces the given path such as "/proc/self/*" with "/proc/<pid>/*", or
+// "/proc/self/task/<tid>/*" with "/proc/<pid>/task/<tid>/*".
+func replaceProcSelfWithProcPid(path string, pid uint32, tid uint32) string {
+	var repl, p string
+
+	pidMatch := regexp.MustCompile(`^/proc/self/(.*)`)
+	tidMatch := regexp.MustCompile(`^/proc/self/task/[0-9]+/(.*)`)
+
+	repl = fmt.Sprintf("/proc/self/task/%d/${1}", tid)
+	p = tidMatch.ReplaceAllString(path, repl)
+
+	repl = fmt.Sprintf("/proc/%d/${1}", pid)
+	p = pidMatch.ReplaceAllString(p, repl)
+
+	return p
+}
+
 // Given a path "/proc/self/path/to/symlink" it resolves it to the location
 // pointed to by symlink. For example, if path is "/proc/self/fd/3" and
 // "/proc/self/fd/3" is a symlink to "/some/path", then this function returns
@@ -643,8 +660,16 @@ func (p *process) getStatus(fields []string) error {
 
 func (p *process) ResolveProcSelf(path string) (string, error) {
 
-	// TODO: this function is not capable of dealing with relative paths
-	// "../../proc/self/fd/X". It also assumes procfs is mounted on /proc.
+	// NOTE: this function assumes procfs is mounted on /proc and path is
+	// absolute.
+
+	if !filepath.IsAbs(path) {
+		return path, nil
+	}
+
+	if !strings.HasPrefix(path, "/proc/self/") {
+		return path, nil
+	}
 
 	currPath := path
 	linkCnt := 0
@@ -654,6 +679,17 @@ func (p *process) ResolveProcSelf(path string) (string, error) {
 			break
 		}
 
+		// Note: for paths such as /proc/self/task/<tid>/*, it's easy to replace
+		// /proc/self with /proc/<pid> since we have the container's process pid
+		// in sysbox's pid-namespace. However, that's not the case for the <tid>,
+		// which is in the container's pid namespace and we have no good/easy way
+		// to translate it sysbox's pid-ns. For now, assume that <tid> = <pid>.
+		// It's not ideal, but it's normally the case when we receive such paths in
+		// mount syscalls.
+
+		tid := p.pid
+		currPath = replaceProcSelfWithProcPid(currPath, p.pid, tid)
+
 		fi, err := os.Lstat(currPath)
 		if err != nil {
 			return "", err
@@ -669,12 +705,8 @@ func (p *process) ResolveProcSelf(path string) (string, error) {
 			return "", syscall.ELOOP
 		}
 
-		// path starts with "/proc/self/" and is a symlink, proceed to resolve it
-
-		procPid := fmt.Sprintf("/proc/%d/", p.pid)
-		procPidPath := strings.Replace(currPath, "/proc/self/", procPid, 1)
-
-		currPath, err = os.Readlink(procPidPath)
+		// path starts with "/proc/self/" and it's a symlink, resolve it
+		currPath, err = os.Readlink(currPath)
 		if err != nil {
 			return "", err
 		}
diff --git a/process/process_test.go b/process/process_test.go
@@ -920,6 +920,31 @@ func TestPathAccessSymlink(t *testing.T) {
 	}
 }
 
+func TestReplaceProcSelfWithProcPid(t *testing.T) {
+
+	type testInput struct {
+		path string
+		pid  uint32
+		tid  uint32
+	}
+
+	test := map[testInput]string{
+		{"/proc/self/mem", 10, 100}:         "/proc/10/mem",
+		{"/proc/self/task/123/io", 20, 200}: "/proc/20/task/200/io",
+		{"/proc/123/task/456/mem", 20, 200}: "/proc/123/task/456/mem",
+		{"/some/other/path", 20, 200}:       "/some/other/path",
+	}
+
+	for k, v := range test {
+		res := replaceProcSelfWithProcPid(k.path, k.pid, k.tid)
+		if res != v {
+			t.Fatalf("failed: replaceProcSelfWithProcPid(%s, %d, %d): got %s, want %s",
+				k.path, k.pid, k.tid, res, v)
+		}
+	}
+}
+
 // TODO:
+// Improve PathAccess tests:
 // * test symlink resolution limit
 // * test long path
diff --git a/seccomp/mount.go b/seccomp/mount.go
@@ -28,6 +28,8 @@ import (
 	"golang.org/x/sys/unix"
 )
 
+var interceptedMountTypes = []string{"proc", "sysfs", "overlay", "nfs", "cifs"}
+
 // MountSyscall information structure.
 type mountSyscallInfo struct {
 	syscallCtx                  // syscall generic info

Original file line number	Diff line number	Diff line change
`@@ -28,6 +28,8 @@ import (`
`28`	`28`	`"golang.org/x/sys/unix"`
`29`	`29`	`)`
`30`	`30`
	`31`	`+var interceptedMountTypes = []string{"proc", "sysfs", "overlay", "nfs", "cifs"}`
	`32`	`+`
`31`	`33`	`// MountSyscall information structure.`
`32`	`34`	`type mountSyscallInfo struct {`
`33`	`35`	`syscallCtx // syscall generic info`