Take more care post launch

Lorenz Bauer found a race condition in the cap.Launcher teardown
process and reported it here:

   https://bugzilla.kernel.org/show_bug.cgi?id=215283

This seems to significantly improve the situation. I'm going to
study the test case some more, but this is definitely part of the
solution.

Signed-off-by: Andrew G. Morgan <morgan@kernel.org>

4 files changed, 72 insertions, 6 deletions

diff --git a/cap/launch.go b/cap/launch.go
index 63959b4..b67a856 100644
--- a/cap/launch.go
+++ b/cap/launch.go
@@ -257,6 +257,12 @@ func launch(result chan<- lResult, attr *Launcher, data interface{}, quit chan<-
 		return
 	}
 
+	// Provide a way to serialize the caller on the thread
+	// completing. This should be done by the one locked tid that
+	// does the ForkExec(). All the other threads have a different
+	// security context.
+	defer close(result)
+
 	// By never releasing the LockOSThread here, we guarantee that
 	// the runtime will terminate the current OS thread once this
 	// function returns.
@@ -266,10 +272,6 @@ func launch(result chan<- lResult, attr *Launcher, data interface{}, quit chan<-
 	// the callbackFn or something else hangs up.
 	singlesc.prctlrcall(prSetName, uintptr(unsafe.Pointer(&lName[0])), 0)
 
-	// Provide a way to serialize the caller on the thread
-	// completing.
-	defer close(result)
-
 	var pa *syscall.ProcAttr
 	var err error
 	var needChroot bool
@@ -394,6 +396,7 @@ func (attr *Launcher) Launch(data interface{}) (int, error) {
 	for {
 		select {
 		case v, ok := <-result:
+			<-result // blocks until the launch() goroutine exits
 			if !ok {
 				return -1, ErrLaunchFailed
 			}
diff --git a/go/.gitignore b/go/.gitignore
index ac6056f..bdb47b5 100644
--- a/go/.gitignore
+++ b/go/.gitignore
@@ -5,6 +5,8 @@ try-launching-cgo
 psx-signals
 psx-signals-cgo
 b210613
+b215283
+b215283-cgo
 mknames
 web
 setid
diff --git a/go/Makefile b/go/Makefile
index 67ded78..ea1db7f 100644
--- a/go/Makefile
+++ b/go/Makefile
@@ -94,6 +94,14 @@ endif
 b210613: b210613.go CAPGOPACKAGE
 	CC="$(CC)" CGO_ENABLED="$(CGO_REQUIRED)" $(CGO_LDFLAGS_ALLOW) CGO_CFLAGS="$(CGO_CFLAGS)" CGO_LDFLAGS="$(CGO_LDFLAGS)" $(GO) build $(GO_BUILD_FLAGS) -mod=vendor $<
 
+b215283: b215283.go CAPGOPACKAGE
+	CC="$(CC)" CGO_ENABLED="$(CGO_REQUIRED)" $(CGO_LDFLAGS_ALLOW) CGO_CFLAGS="$(CGO_CFLAGS)" CGO_LDFLAGS="$(CGO_LDFLAGS)" $(GO) build $(GO_BUILD_FLAGS) -mod=vendor $<
+
+ifeq ($(CGO_REQUIRED),0)
+b215283-cgo: b215283.go CAPGOPACKAGE
+	CC="$(CC)" CGO_ENABLED="1" $(CGO_LDFLAGS_ALLOW) CGO_CFLAGS="$(CGO_CFLAGS)" CGO_LDFLAGS="$(CGO_LDFLAGS)" $(GO) build $(GO_BUILD_FLAGS) -mod=vendor -o $@ $<
+endif
+
 test: setid gowns captree $(TESTS)
 	CC="$(CC)" CGO_ENABLED="$(CGO_REQUIRED)" $(CGO_LDFLAGS_ALLOW) $(GO) test -mod=vendor $(IMPORTDIR)/psx
 	CC="$(CC)" CGO_ENABLED="$(CGO_REQUIRED)" $(CGO_LDFLAGS_ALLOW) $(GO) test -mod=vendor $(IMPORTDIR)/cap
@@ -110,7 +118,7 @@ endif
 # Note, the user namespace doesn't require sudo, but I wanted to avoid
 # requiring that the hosting kernel supports user namespaces for the
 # regular test case.
-sudotest: test ../progs/tcapsh-static b210613
+sudotest: test ../progs/tcapsh-static b210613 b215283
 	../progs/tcapsh-static --has-b=cap_sys_admin || exit 0 && ./gowns --ns -- -c "echo gowns runs with user namespace"
 	./try-launching
 ifeq ($(CGO_REQUIRED),0)
@@ -121,6 +129,12 @@ ifeq ($(CGO_REQUIRED),0)
 	$(SUDO) ./try-launching-cgo
 endif
 	$(SUDO) ../progs/tcapsh-static --cap-uid=$$(id -u) --caps="cap_setpcap=ep" --iab="^cap_setpcap" -- -c ./b210613
+	$(SUDO) ./b215283
+ifeq ($(CGO_REQUIRED),0)
+	$(MAKE) b215283-cgo
+	$(SUDO) ./b215283-cgo
+endif
+
 
 # As of libcap-2.55 We stopped installing the cap and psx packages as
 # part of the install.  Most distribution's packagers skip the Go
@@ -142,5 +156,5 @@ clean:
 	rm -f web setid gowns captree
 	rm -f compare-cap try-launching try-launching-cgo
 	rm -f $(topdir)/cap/*~ $(topdir)/psx/*~
-	rm -f b210613 psx-signals psx-signals-cgo
+	rm -f b210613 b215283 b215283-cgo psx-signals psx-signals-cgo
 	rm -fr vendor CAPGOPACKAGE PSXGOPACKAGE go.sum
diff --git a/go/b215283.go b/go/b215283.go
new file mode 100644
index 0000000..26596b6
--- /dev/null
+++ b/go/b215283.go
@@ -0,0 +1,47 @@
+// Program b215283 requires privilege to execute and is a minimally adapted
+// version of a test case provided by Lorenz Bauer as a reproducer for a
+// problem he found and reported in:
+//
+//    https://bugzilla.kernel.org/show_bug.cgi?id=215283
+package main
+
+import (
+	"fmt"
+	"os"
+
+	"kernel.org/pub/linux/libs/security/libcap/cap"
+)
+
+func main() {
+	const secbits = cap.SecbitNoRoot | cap.SecbitNoSetUIDFixup
+
+	if v, err := cap.GetProc().GetFlag(cap.Permitted, cap.SETPCAP); err != nil {
+		panic(fmt.Sprintf("failed to get flag value: %v", err))
+		os.Exit(1)
+	} else if !v {
+		fmt.Printf("test requires cap_setpcap: found %q\n", cap.GetProc())
+		os.Exit(1)
+	}
+	if bits := cap.GetSecbits(); bits != 0 {
+		fmt.Printf("test expects secbits=0 to run; found: 0%o\n", bits)
+		os.Exit(1)
+	}
+
+	fmt.Println("secbits:", cap.GetSecbits(), " caps:", cap.GetProc())
+
+	l := cap.FuncLauncher(func(interface{}) error {
+		return cap.NewSet().SetProc()
+	})
+
+	if _, err := l.Launch(nil); err != nil {
+		fmt.Printf("launch failed: %v\n", err)
+		os.Exit(1)
+	}
+
+	fmt.Println("secbits:", cap.GetSecbits(), " caps:", cap.GetProc())
+
+	if err := secbits.Set(); err != nil {
+		fmt.Printf("set securebits: %v", err.Error())
+		os.Exit(1)
+	}
+}