Bugfix for (*IAB).Fill() and improve atomicity of API.

Improve atomicity of Launcher and IAB use within the cap package.

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

cap/cap_test.go

@@ -200,17 +200,19 @@ func TestIAB(t *testing.T) {
 		t.Fatalf("failed to get init's capabilities: %v", err)
 	}
 	iab := NewIAB()
-	iab.Fill(Amb, one, Permitted)
+	if err := iab.Fill(Amb, one, Permitted); err != nil {
+		t.Fatalf("failed to fill Amb from Permitted: %v", err)
+	}
 	for i := 0; i < words; i++ {
 		if iab.i[i] != iab.a[i] {
-			t.Errorf("[%d] i=0x%08x != a=0x%08x", i, iab.i[i], iab.a[i])
+			t.Errorf("[%d: %q] i=0x%08x != a=0x%08x", i, one, iab.i[i], iab.a[i])
 		}
 	}
 	one.ClearFlag(Inheritable)
 	iab.Fill(Inh, one, Inheritable)
 	for i := 0; i < words; i++ {
 		if iab.i[i] != iab.a[i] {
-			t.Errorf("[%d] i=0x%08x != a=0x%08x", i, iab.i[i], iab.a[i])
+			t.Errorf("[%d: %q] i=0x%08x != a=0x%08x", i, one, iab.i[i], iab.a[i])
 		}
 	}
 

cap/iab.go

@@ -5,6 +5,7 @@ import (
 	"io/ioutil"
 	"strconv"
 	"strings"
+	"sync"
 )
 
 // omask returns the offset and mask for a specific capability.
@@ -20,6 +21,7 @@ func omask(c Value) (uint, uint32) {
 // Value from the process' Bounding set. This convention is used to
 // support the empty IAB as being mostly harmless.
 type IAB struct {
+	mu       sync.RWMutex
 	a, i, nb []uint32
 }
 
@@ -79,6 +81,20 @@ func NewIAB() *IAB {
 	}
 }
 
+// Dup returns a duplicate copy of the IAB.
+func (iab *IAB) Dup() (*IAB, error) {
+	if iab == nil {
+		return nil, ErrBadValue
+	}
+	v := NewIAB()
+	iab.mu.RLock()
+	defer iab.mu.RUnlock()
+	copy(v.i, iab.i)
+	copy(v.a, iab.a)
+	copy(v.nb, iab.nb)
+	return v, nil
+}
+
 // IABInit allocates a new IAB tuple.
 //
 // Deprecated: Replace with NewIAB.
@@ -161,6 +177,8 @@ func (iab *IAB) String() string {
 		return "<invalid>"
 	}
 	var vs []string
+	iab.mu.RLock()
+	defer iab.mu.RUnlock()
 	for c := Value(0); c < Value(maxValues); c++ {
 		offset, mask := omask(c)
 		i := (iab.i[offset] & mask) != 0
@@ -182,6 +200,8 @@ func (iab *IAB) String() string {
 	return strings.Join(vs, ",")
 }
 
+// iabSetProc uses a syscaller to apply an IAB tuple to the process.
+// The iab is known to be locked by the caller.
 func (sc *syscaller) iabSetProc(iab *IAB) (err error) {
 	temp := GetProc()
 	var raising uint32
@@ -234,17 +254,24 @@ func (sc *syscaller) iabSetProc(iab *IAB) (err error) {
 // other bits, so this function carefully performs the the combined
 // operation in the most flexible manner.
 func (iab *IAB) SetProc() error {
+	if iab == nil {
+		return ErrBadValue
+	}
 	state, sc := scwStateSC()
 	defer scwSetState(launchBlocked, state, -1)
+	iab.mu.RLock()
+	defer iab.mu.RUnlock()
 	return sc.iabSetProc(iab)
 }
 
 // GetVector returns the raised state of the specific capability bit
 // of the indicated vector.
 func (iab *IAB) GetVector(vec Vector, val Value) (bool, error) {
-	if val >= MaxBits() {
+	if val >= MaxBits() || iab == nil {
 		return false, ErrBadValue
 	}
+	iab.mu.RLock()
+	defer iab.mu.RUnlock()
 	offset, mask := omask(val)
 	switch vec {
 	case Inh:
@@ -266,6 +293,11 @@ func (iab *IAB) GetVector(vec Vector, val Value) (bool, error) {
 // equivalent to lowering the Bounding vector of the process (when
 // successfully applied with (*IAB).SetProc()).
 func (iab *IAB) SetVector(vec Vector, raised bool, vals ...Value) error {
+	if iab == nil {
+		return ErrBadValue
+	}
+	iab.mu.Lock()
+	defer iab.mu.Unlock()
 	for _, val := range vals {
 		if val >= Value(maxValues) {
 			return ErrBadValue
@@ -306,18 +338,25 @@ func (iab *IAB) SetVector(vec Vector, raised bool, vals ...Value) error {
 // the bits are inverted from what you might expect - that is lowered
 // bits from the Set will be raised in the Bound vector.
 func (iab *IAB) Fill(vec Vector, c *Set, flag Flag) error {
-	if len(c.flat) != 0 || flag > Inheritable {
-		return ErrBadSet
+	if iab == nil {
+		return ErrBadValue
 	}
+	// work with a copy to avoid potential deadlock.
+	s, err := c.Dup()
+	if err != nil {
+		return err
+	}
+	iab.mu.Lock()
+	defer iab.mu.Unlock()
 	for i := 0; i < words; i++ {
-		flat := c.flat[i][flag]
+		flat := s.flat[i][flag]
 		switch vec {
 		case Inh:
 			iab.i[i] = flat
-			iab.a[i] &= ^flat
+			iab.a[i] &= flat
 		case Amb:
 			iab.a[i] = flat
-			iab.i[i] |= ^flat
+			iab.i[i] |= flat
 		case Bound:
 			iab.nb[i] = ^flat
 		default:
@@ -337,16 +376,23 @@ func (iab *IAB) Cf(alt *IAB) (IABDiff, error) {
 	if iab == nil || alt == nil || len(iab.i) != words || len(alt.i) != words || len(iab.a) != words || len(alt.a) != words || len(iab.nb) != words || len(alt.nb) != words {
 		return 0, ErrBadValue
 	}
+	// Avoid holding two locks at once.
+	ref, err := alt.Dup()
+	if err != nil {
+		return 0, ErrBadValue
+	}
+	iab.mu.RLock()
+	defer iab.mu.RUnlock()
 
 	var cf IABDiff
 	for i := 0; i < words; i++ {
-		if iab.i[i] != alt.i[i] {
+		if iab.i[i] != ref.i[i] {
 			cf |= iBits
 		}
-		if iab.a[i] != alt.a[i] {
+		if iab.a[i] != ref.a[i] {
 			cf |= aBits
 		}
-		if iab.nb[i] != alt.nb[i] {
+		if iab.nb[i] != ref.nb[i] {
 			cf |= bBits
 		}
 	}

cap/launch.go

@@ -4,6 +4,7 @@ import (
 	"errors"
 	"os"
 	"runtime"
+	"sync"
 	"syscall"
 	"unsafe"
 )
@@ -15,6 +16,8 @@ import (
 // Note, go1.10 is the earliest version of the Go toolchain that can
 // support this abstraction.
 type Launcher struct {
+	mu sync.RWMutex
+
 	// Note, path and args must be set, or callbackFn. They cannot
 	// both be empty. In such cases .Launch() will error out.
 	path string
@@ -108,11 +111,21 @@ func FuncLauncher(fn func(interface{}) error) *Launcher {
 // Launch() invocation and can communicate contextual info to and from
 // the callback and the main process.
 func (attr *Launcher) Callback(fn func(*syscall.ProcAttr, interface{}) error) {
+	if attr == nil {
+		return
+	}
+	attr.mu.Lock()
+	defer attr.mu.Unlock()
 	attr.callbackFn = fn
 }
 
 // SetUID specifies the UID to be used by the launched command.
 func (attr *Launcher) SetUID(uid int) {
+	if attr == nil {
+		return
+	}
+	attr.mu.Lock()
+	defer attr.mu.Unlock()
 	attr.changeUIDs = true
 	attr.uid = uid
 }
@@ -120,6 +133,11 @@ func (attr *Launcher) SetUID(uid int) {
 // SetGroups specifies the GID and supplementary groups for the
 // launched command.
 func (attr *Launcher) SetGroups(gid int, groups []int) {
+	if attr == nil {
+		return
+	}
+	attr.mu.Lock()
+	defer attr.mu.Unlock()
 	attr.changeGIDs = true
 	attr.gid = gid
 	attr.groups = groups
@@ -127,20 +145,37 @@ func (attr *Launcher) SetGroups(gid int, groups []int) {
 
 // SetMode specifies the libcap Mode to be used by the launched command.
 func (attr *Launcher) SetMode(mode Mode) {
+	if attr == nil {
+		return
+	}
+	attr.mu.Lock()
+	defer attr.mu.Unlock()
 	attr.changeMode = true
 	attr.mode = mode
 }
 
-// SetIAB specifies the AIB capability vectors to be inherited by the
+// SetIAB specifies the IAB capability vectors to be inherited by the
 // launched command. A nil value means the prevailing vectors of the
-// parent will be inherited.
+// parent will be inherited. Note, a duplicate of the provided IAB
+// tuple is actually stored, so concurrent modification of the iab
+// value does not affect the launcher.
 func (attr *Launcher) SetIAB(iab *IAB) {
-	attr.iab = iab
+	if attr == nil {
+		return
+	}
+	attr.mu.Lock()
+	defer attr.mu.Unlock()
+	attr.iab, _ = iab.Dup()
 }
 
 // SetChroot specifies the chroot value to be used by the launched
 // command. An empty value means no-change from the prevailing value.
 func (attr *Launcher) SetChroot(root string) {
+	if attr == nil {
+		return
+	}
+	attr.mu.Lock()
+	defer attr.mu.Unlock()
 	attr.chroot = root
 }
 
@@ -283,6 +318,8 @@ func launch(result chan<- lResult, attr *Launcher, data interface{}, quit chan<-
 		}
 	}
 	if attr.iab != nil {
+		// Note, since .iab is a private copy we don't need to
+		// lock it around this call.
 		if err = singlesc.iabSetProc(attr.iab); err != nil {
 			goto abort
 		}
@@ -343,6 +380,11 @@ func (attr *Launcher) Launch(data interface{}) (int, error) {
 	if !LaunchSupported {
 		return -1, ErrNoLaunch
 	}
+	if attr == nil {
+		return -1, ErrLaunchFailed
+	}
+	attr.mu.RLock()
+	defer attr.mu.RUnlock()
 	if attr.callbackFn == nil && (attr.path == "" || len(attr.args) == 0) {
 		return -1, ErrLaunchFailed
 	}