From 8b8aad91d581ee5f600f5ec0b7fb407b36d07db1 Mon Sep 17 00:00:00 2001
From: Adin Scannell <ascannell@google.com>
Date: Thu, 19 Jul 2018 15:48:08 -0700
Subject: kernel: mutations on creds now require a copy.

PiperOrigin-RevId: 205315612
Change-Id: I9a0a1e32c8abfb7467a38743b82449cc92830316
---
 pkg/sentry/kernel/fasync/fasync.go    |  2 +-
 pkg/sentry/kernel/task.go             |  4 +++-
 pkg/sentry/kernel/task_identity.go    | 16 +++++++++++++---
 pkg/sentry/syscalls/linux/sys_file.go |  4 ++--
 4 files changed, 19 insertions(+), 7 deletions(-)

(limited to 'pkg')

diff --git a/pkg/sentry/kernel/fasync/fasync.go b/pkg/sentry/kernel/fasync/fasync.go
index 028d6766f..15218fb5a 100644
--- a/pkg/sentry/kernel/fasync/fasync.go
+++ b/pkg/sentry/kernel/fasync/fasync.go
@@ -35,7 +35,7 @@ func New() fs.FileAsync {
 type FileAsync struct {
 	mu        sync.Mutex
 	e         waiter.Entry
-	requester auth.Credentials
+	requester *auth.Credentials
 
 	// Only one of the following is allowed to be non-nil.
 	recipientPG *kernel.ProcessGroup
diff --git a/pkg/sentry/kernel/task.go b/pkg/sentry/kernel/task.go
index 7763050a5..7f6735320 100644
--- a/pkg/sentry/kernel/task.go
+++ b/pkg/sentry/kernel/task.go
@@ -334,7 +334,9 @@ type Task struct {
 
 	// creds is the task's credentials.
 	//
-	// creds is protected by mu.
+	// creds is protected by mu, however the value itself is immutable and
+	// can only be changed by a copy. After reading the pointer, access
+	// will proceed outside the scope of mu.
 	creds *auth.Credentials
 
 	// utsns is the task's UTS namespace.
diff --git a/pkg/sentry/kernel/task_identity.go b/pkg/sentry/kernel/task_identity.go
index a51fa9d7e..b0921b2eb 100644
--- a/pkg/sentry/kernel/task_identity.go
+++ b/pkg/sentry/kernel/task_identity.go
@@ -20,11 +20,13 @@ import (
 	"gvisor.googlesource.com/gvisor/pkg/syserror"
 )
 
-// Credentials returns t's credentials by value.
-func (t *Task) Credentials() auth.Credentials {
+// Credentials returns t's credentials.
+//
+// This value must be considered immutable.
+func (t *Task) Credentials() *auth.Credentials {
 	t.mu.Lock()
 	defer t.mu.Unlock()
-	return *t.creds // Copy out with lock held.
+	return t.creds
 }
 
 // UserNamespace returns the user namespace associated with the task.
@@ -162,6 +164,7 @@ func (t *Task) SetRESUID(r, e, s auth.UID) error {
 func (t *Task) setKUIDsUncheckedLocked(newR, newE, newS auth.KUID) {
 	root := t.creds.UserNamespace.MapToKUID(auth.RootUID)
 	oldR, oldE, oldS := t.creds.RealKUID, t.creds.EffectiveKUID, t.creds.SavedKUID
+	t.creds = t.creds.Fork() // See doc for creds.
 	t.creds.RealKUID, t.creds.EffectiveKUID, t.creds.SavedKUID = newR, newE, newS
 
 	// "1. If one or more of the real, effective or saved set user IDs was
@@ -297,6 +300,7 @@ func (t *Task) SetRESGID(r, e, s auth.GID) error {
 
 func (t *Task) setKGIDsUncheckedLocked(newR, newE, newS auth.KGID) {
 	oldE := t.creds.EffectiveKGID
+	t.creds = t.creds.Fork() // See doc for creds.
 	t.creds.RealKGID, t.creds.EffectiveKGID, t.creds.SavedKGID = newR, newE, newS
 
 	// Not documented, but compare Linux's kernel/cred.c:commit_creds().
@@ -321,6 +325,7 @@ func (t *Task) SetExtraGIDs(gids []auth.GID) error {
 		}
 		kgids[i] = kgid
 	}
+	t.creds = t.creds.Fork() // See doc for creds.
 	t.creds.ExtraKGIDs = kgids
 	return nil
 }
@@ -352,6 +357,7 @@ func (t *Task) SetCapabilitySets(permitted, inheritable, effective auth.Capabili
 	if inheritable & ^(t.creds.InheritableCaps|t.creds.BoundingCaps) != 0 {
 		return syserror.EPERM
 	}
+	t.creds = t.creds.Fork() // See doc for creds.
 	t.creds.PermittedCaps = permitted
 	t.creds.InheritableCaps = inheritable
 	t.creds.EffectiveCaps = effective
@@ -384,6 +390,7 @@ func (t *Task) SetUserNamespace(ns *auth.UserNamespace) error {
 		return syserror.EPERM
 	}
 
+	t.creds = t.creds.Fork() // See doc for creds.
 	t.creds.UserNamespace = ns
 	// "The child process created by clone(2) with the CLONE_NEWUSER flag
 	// starts out with a complete set of capabilities in the new user
@@ -407,6 +414,7 @@ func (t *Task) SetUserNamespace(ns *auth.UserNamespace) error {
 func (t *Task) SetKeepCaps(k bool) {
 	t.mu.Lock()
 	defer t.mu.Unlock()
+	t.creds = t.creds.Fork() // See doc for creds.
 	t.creds.KeepCaps = k
 }
 
@@ -491,6 +499,8 @@ func (t *Task) updateCredsForExecLocked() {
 		}
 	}
 
+	t.creds = t.creds.Fork() // See doc for creds.
+
 	// Now we enter poorly-documented, somewhat confusing territory. (The
 	// accompanying comment in Linux's security/commoncap.c:cap_bprm_set_creds
 	// is not very helpful.) My reading of it is:
diff --git a/pkg/sentry/syscalls/linux/sys_file.go b/pkg/sentry/syscalls/linux/sys_file.go
index 490649f87..66e6fd9d4 100644
--- a/pkg/sentry/syscalls/linux/sys_file.go
+++ b/pkg/sentry/syscalls/linux/sys_file.go
@@ -415,14 +415,14 @@ func Creat(t *kernel.Task, args arch.SyscallArguments) (uintptr, *kernel.Syscall
 // accessContext should only be used for access(2).
 type accessContext struct {
 	context.Context
-	creds auth.Credentials
+	creds *auth.Credentials
 }
 
 // Value implements context.Context.
 func (ac accessContext) Value(key interface{}) interface{} {
 	switch key {
 	case auth.CtxCredentials:
-		return &ac.creds
+		return ac.creds
 	default:
 		return ac.Context.Value(key)
 	}
-- 
cgit v1.2.3