diff options
author | Andrew G. Morgan <morgan@kernel.org> | 2021-10-22 15:21:58 -0700 |
---|---|---|
committer | Andrew G. Morgan <morgan@kernel.org> | 2021-10-22 15:33:36 -0700 |
commit | 140fa8438bde4e6affe8aefa6fbf077eae968686 (patch) | |
tree | a6e38e090ac2212b279747bfcdb221ea469be7eb | |
parent | 73194f5369286e10c9e19cbd71de59c3b45b5789 (diff) | |
download | libcap-140fa8438bde4e6affe8aefa6fbf077eae968686.tar.gz |
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>
-rw-r--r-- | cap/cap_test.go | 8 | ||||
-rw-r--r-- | cap/iab.go | 64 | ||||
-rw-r--r-- | cap/launch.go | 48 |
3 files changed, 105 insertions, 15 deletions
diff --git a/cap/cap_test.go b/cap/cap_test.go index 54f1e3f..52afd43 100644 --- a/cap/cap_test.go +++ b/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]) } } @@ -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 } } diff --git a/cap/launch.go b/cap/launch.go index 6145f3e..63959b4 100644 --- a/cap/launch.go +++ b/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 } |