[PATCH 1/8] p9auth: set fsuid

[PATCH 1/8] p9auth: set fsuid

[Serge Hallyn]

From: Serge E. Hallyn <serue@us.ibm.com>

fsuid should always trail euid changes.  So p9auth should
set fsuid as well when it sets ruid and euid.  Whether the
suid should also be set is an open question - keeping the
old uid in suid may be useful, or may just serve to trick
lazy userspace.

Note that so long as we do not also set suid, the setuid_fixup()
code will not (when we later switch to setresuid()) fully
fill/clear capability sets.  So while I had previously thought
that keeping suid unchanged would be useful, I think it is
better to change all uids.

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
Cc: Greg KH <greg@kroah.com>
cc: rsc@swtch.com
Cc: Ashwin Ganti <ashwin.ganti@gmail.com>
Cc: ericvh@gmail.com
Cc: devel@linuxdriverproject.org
Cc: linux-kernel@vger.kernel.org
Cc: Ron Minnich <rminnich@gmail.com>
---
 drivers/staging/p9auth/p9auth.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/p9auth/p9auth.c b/drivers/staging/p9auth/p9auth.c
index db79626..70ef45b 100644
--- a/drivers/staging/p9auth/p9auth.c
+++ b/drivers/staging/p9auth/p9auth.c
@@ -275,10 +275,14 @@ static ssize_t cap_write(struct file *filp, const char __user *buf,
 					goto out;
 				}
 				/*
-				 * What all id's need to be changed here? uid,
-				 * euid, fsid, savedids ??  Currently I am
-				 * changing the effective user id since most of
-				 * the authorisation decisions are based on it
+				 * Change all uids.  It might be useful to
+				 * keep suid unchanged, however that will
+				 * mean that changing from uid=0 to uid=!0
+				 * pP is not emptied (only pE is), and when
+				 * changing from  uid=!0 to  uid=0, sets are
+				 * not filled.  They will be correct after
+				 * the next exec, but this is IMO not
+				 * sufficient.  So change all uids.
 				 */
 				new = prepare_creds();
 				if (!new) {
@@ -286,7 +290,7 @@ static ssize_t cap_write(struct file *filp, const char __user *buf,
 					goto out;
 				}
 				new->uid = (uid_t) target_int;
-				new->euid = (uid_t) target_int;
+				new->suid = new->fsuid = new->euid = new->uid;
 				retval = commit_creds(new);
 				if (retval)
 					goto out;
-- 
1.6.1

[PATCH 2/8] p9auth: split core function out of some set*{u,g}id functions

[Serge Hallyn]

From: Serge E. Hallyn <serue@us.ibm.com>

Break the core functionality of set{fs,res}{u,g}id into cred_setX
which performs the access checks based on current_cred(), but performs
the requested change on a passed-in cred.

The helpers will be further broken down into permission-checks
and permission-less id-setting helpers.  The id-setting helpers
for resuid and resgid will be used by p9auth to perform the
authorized id changes.  Export the helpers, since p9auth can be
compiled as a module.

Really the setfs{u,g}id helper isn't needed, but move it as
well to keep the code consistent.

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
Cc: Greg KH <greg@kroah.com>
cc: rsc@swtch.com
Cc: Ashwin Ganti <ashwin.ganti@gmail.com>
Cc: ericvh@gmail.com
Cc: devel@linuxdriverproject.org
Cc: linux-kernel@vger.kernel.org
Cc: Ron Minnich <rminnich@gmail.com>
---
 include/linux/cred.h |    8 +++
 kernel/cred.c        |  118 +++++++++++++++++++++++++++++++++++++++++++++
 kernel/sys.c         |  131 ++++++++------------------------------------------
 3 files changed, 146 insertions(+), 111 deletions(-)

diff --git a/include/linux/cred.h b/include/linux/cred.h
index 4e3387a..e35631e 100644
--- a/include/linux/cred.h
+++ b/include/linux/cred.h
@@ -22,6 +22,9 @@ struct user_struct;
 struct cred;
 struct inode;
 
+/* defined in sys.c, used in cred_setresuid */
+extern int set_user(struct cred *new);
+
 /*
  * COW Supplementary groups list
  */
@@ -396,4 +399,9 @@ do {						\
 	*(_fsgid) = __cred->fsgid;		\
 } while(0)
 
+int cred_setresuid(struct cred *new, uid_t ruid, uid_t euid, uid_t suid);
+int cred_setresgid(struct cred *new, gid_t rgid, gid_t egid, gid_t sgid);
+int cred_setfsuid(struct cred *new, uid_t uid, uid_t *old_fsuid);
+int cred_setfsgid(struct cred *new, gid_t gid, gid_t *old_fsgid);
+
 #endif /* _LINUX_CRED_H */
diff --git a/kernel/cred.c b/kernel/cred.c
index 1ed8ca1..6eee59c 100644
--- a/kernel/cred.c
+++ b/kernel/cred.c
@@ -780,6 +780,124 @@ int set_create_files_as(struct cred *new, struct inode *inode)
 }
 EXPORT_SYMBOL(set_create_files_as);
 
+int cred_setresuid(struct cred *new, uid_t ruid, uid_t euid, uid_t suid)
+{
+	int retval;
+	const struct cred *old;
+
+	retval = security_task_setuid(ruid, euid, suid, LSM_SETID_RES);
+	if (retval)
+		return retval;
+	old = current_cred();
+
+	if (!capable(CAP_SETUID)) {
+		if (ruid != (uid_t) -1 && ruid != old->uid &&
+		    ruid != old->euid  && ruid != old->suid)
+			return -EPERM;
+		if (euid != (uid_t) -1 && euid != old->uid &&
+		    euid != old->euid  && euid != old->suid)
+			return -EPERM;
+		if (suid != (uid_t) -1 && suid != old->uid &&
+		    suid != old->euid  && suid != old->suid)
+			return -EPERM;
+	}
+
+	if (ruid != (uid_t) -1) {
+		new->uid = ruid;
+		if (ruid != old->uid) {
+			retval = set_user(new);
+			if (retval < 0)
+				return retval;
+		}
+	}
+	if (euid != (uid_t) -1)
+		new->euid = euid;
+	if (suid != (uid_t) -1)
+		new->suid = suid;
+	new->fsuid = new->euid;
+
+	return security_task_fix_setuid(new, old, LSM_SETID_RES);
+}
+EXPORT_SYMBOL_GPL(cred_setresuid);
+
+int cred_setresgid(struct cred *new, gid_t rgid, gid_t egid,
+			gid_t sgid)
+{
+	const struct cred *old = current_cred();
+	int retval;
+
+	retval = security_task_setgid(rgid, egid, sgid, LSM_SETID_RES);
+	if (retval)
+		return retval;
+
+	if (!capable(CAP_SETGID)) {
+		if (rgid != (gid_t) -1 && rgid != old->gid &&
+		    rgid != old->egid  && rgid != old->sgid)
+			return -EPERM;
+		if (egid != (gid_t) -1 && egid != old->gid &&
+		    egid != old->egid  && egid != old->sgid)
+			return -EPERM;
+		if (sgid != (gid_t) -1 && sgid != old->gid &&
+		    sgid != old->egid  && sgid != old->sgid)
+			return -EPERM;
+	}
+
+	if (rgid != (gid_t) -1)
+		new->gid = rgid;
+	if (egid != (gid_t) -1)
+		new->egid = egid;
+	if (sgid != (gid_t) -1)
+		new->sgid = sgid;
+	new->fsgid = new->egid;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(cred_setresgid);
+
+int cred_setfsuid(struct cred *new, uid_t uid, uid_t *old_fsuid)
+{
+	const struct cred *old;
+
+	old = current_cred();
+	*old_fsuid = old->fsuid;
+
+	if (security_task_setuid(uid, (uid_t)-1, (uid_t)-1, LSM_SETID_FS) < 0)
+		return -EPERM;
+
+	if (uid == old->uid  || uid == old->euid  ||
+	    uid == old->suid || uid == old->fsuid ||
+	    capable(CAP_SETUID)) {
+		if (uid != *old_fsuid) {
+			new->fsuid = uid;
+			if (security_task_fix_setuid(new, old, LSM_SETID_FS) == 0)
+				return 0;
+		}
+	}
+	return -EPERM;
+}
+EXPORT_SYMBOL_GPL(cred_setfsuid);
+
+int cred_setfsgid(struct cred *new, gid_t gid, gid_t *old_fsgid)
+{
+	const struct cred *old;
+
+	old = current_cred();
+	*old_fsgid = old->fsgid;
+
+	if (security_task_setgid(gid, (gid_t)-1, (gid_t)-1, LSM_SETID_FS))
+		return -EPERM;
+
+	if (gid == old->gid  || gid == old->egid  ||
+	    gid == old->sgid || gid == old->fsgid ||
+	    capable(CAP_SETGID)) {
+		if (gid != *old_fsgid) {
+			new->fsgid = gid;
+			return 0;
+		}
+	}
+	return -EPERM;
+}
+EXPORT_SYMBOL_GPL(cred_setfsgid);
+
 #ifdef CONFIG_DEBUG_CREDENTIALS
 
 bool creds_are_invalid(const struct cred *cred)
diff --git a/kernel/sys.c b/kernel/sys.c
index 26a6b73..1927edf 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -561,11 +561,11 @@ error:
 /*
  * change the user struct in a credentials set to match the new UID
  */
-static int set_user(struct cred *new)
+int set_user(struct cred *new)
 {
 	struct user_struct *new_user;
 
-	new_user = alloc_uid(current_user_ns(), new->uid);
+	new_user = alloc_uid(new->user->user_ns, new->uid);
 	if (!new_user)
 		return -EAGAIN;
 
@@ -713,7 +713,6 @@ error:
  */
 SYSCALL_DEFINE3(setresuid, uid_t, ruid, uid_t, euid, uid_t, suid)
 {
-	const struct cred *old;
 	struct cred *new;
 	int retval;
 
@@ -721,45 +720,10 @@ SYSCALL_DEFINE3(setresuid, uid_t, ruid, uid_t, euid, uid_t, suid)
 	if (!new)
 		return -ENOMEM;
 
-	retval = security_task_setuid(ruid, euid, suid, LSM_SETID_RES);
-	if (retval)
-		goto error;
-	old = current_cred();
+	retval = cred_setresuid(new, ruid, euid, suid);
+	if (retval == 0)
+		return commit_creds(new);
 
-	retval = -EPERM;
-	if (!capable(CAP_SETUID)) {
-		if (ruid != (uid_t) -1 && ruid != old->uid &&
-		    ruid != old->euid  && ruid != old->suid)
-			goto error;
-		if (euid != (uid_t) -1 && euid != old->uid &&
-		    euid != old->euid  && euid != old->suid)
-			goto error;
-		if (suid != (uid_t) -1 && suid != old->uid &&
-		    suid != old->euid  && suid != old->suid)
-			goto error;
-	}
-
-	if (ruid != (uid_t) -1) {
-		new->uid = ruid;
-		if (ruid != old->uid) {
-			retval = set_user(new);
-			if (retval < 0)
-				goto error;
-		}
-	}
-	if (euid != (uid_t) -1)
-		new->euid = euid;
-	if (suid != (uid_t) -1)
-		new->suid = suid;
-	new->fsuid = new->euid;
-
-	retval = security_task_fix_setuid(new, old, LSM_SETID_RES);
-	if (retval < 0)
-		goto error;
-
-	return commit_creds(new);
-
-error:
 	abort_creds(new);
 	return retval;
 }
@@ -781,43 +745,17 @@ SYSCALL_DEFINE3(getresuid, uid_t __user *, ruid, uid_t __user *, euid, uid_t __u
  */
 SYSCALL_DEFINE3(setresgid, gid_t, rgid, gid_t, egid, gid_t, sgid)
 {
-	const struct cred *old;
 	struct cred *new;
 	int retval;
 
 	new = prepare_creds();
 	if (!new)
 		return -ENOMEM;
-	old = current_cred();
 
-	retval = security_task_setgid(rgid, egid, sgid, LSM_SETID_RES);
-	if (retval)
-		goto error;
+	retval = cred_setresgid(new, rgid, egid, sgid);
+	if (retval == 0)
+		return commit_creds(new);
 
-	retval = -EPERM;
-	if (!capable(CAP_SETGID)) {
-		if (rgid != (gid_t) -1 && rgid != old->gid &&
-		    rgid != old->egid  && rgid != old->sgid)
-			goto error;
-		if (egid != (gid_t) -1 && egid != old->gid &&
-		    egid != old->egid  && egid != old->sgid)
-			goto error;
-		if (sgid != (gid_t) -1 && sgid != old->gid &&
-		    sgid != old->egid  && sgid != old->sgid)
-			goto error;
-	}
-
-	if (rgid != (gid_t) -1)
-		new->gid = rgid;
-	if (egid != (gid_t) -1)
-		new->egid = egid;
-	if (sgid != (gid_t) -1)
-		new->sgid = sgid;
-	new->fsgid = new->egid;
-
-	return commit_creds(new);
-
-error:
 	abort_creds(new);
 	return retval;
 }
@@ -843,35 +781,20 @@ SYSCALL_DEFINE3(getresgid, gid_t __user *, rgid, gid_t __user *, egid, gid_t __u
  */
 SYSCALL_DEFINE1(setfsuid, uid_t, uid)
 {
-	const struct cred *old;
 	struct cred *new;
 	uid_t old_fsuid;
+	int retval;
 
 	new = prepare_creds();
 	if (!new)
 		return current_fsuid();
-	old = current_cred();
-	old_fsuid = old->fsuid;
-
-	if (security_task_setuid(uid, (uid_t)-1, (uid_t)-1, LSM_SETID_FS) < 0)
-		goto error;
-
-	if (uid == old->uid  || uid == old->euid  ||
-	    uid == old->suid || uid == old->fsuid ||
-	    capable(CAP_SETUID)) {
-		if (uid != old_fsuid) {
-			new->fsuid = uid;
-			if (security_task_fix_setuid(new, old, LSM_SETID_FS) == 0)
-				goto change_okay;
-		}
-	}
 
-error:
-	abort_creds(new);
-	return old_fsuid;
+	retval = cred_setfsuid(new, uid, &old_fsuid);
+	if (retval == 0)
+		commit_creds(new);
+	else
+		abort_creds(new);
 
-change_okay:
-	commit_creds(new);
 	return old_fsuid;
 }
 
@@ -880,34 +803,20 @@ change_okay:
  */
 SYSCALL_DEFINE1(setfsgid, gid_t, gid)
 {
-	const struct cred *old;
 	struct cred *new;
 	gid_t old_fsgid;
+	int retval;
 
 	new = prepare_creds();
 	if (!new)
 		return current_fsgid();
-	old = current_cred();
-	old_fsgid = old->fsgid;
-
-	if (security_task_setgid(gid, (gid_t)-1, (gid_t)-1, LSM_SETID_FS))
-		goto error;
-
-	if (gid == old->gid  || gid == old->egid  ||
-	    gid == old->sgid || gid == old->fsgid ||
-	    capable(CAP_SETGID)) {
-		if (gid != old_fsgid) {
-			new->fsgid = gid;
-			goto change_okay;
-		}
-	}
 
-error:
-	abort_creds(new);
-	return old_fsgid;
+	retval = cred_setfsgid(new, gid, &old_fsgid);
+	if (retval == 0)
+		commit_creds(new);
+	else
+		abort_creds(new);
 
-change_okay:
-	commit_creds(new);
 	return old_fsgid;
 }
 
-- 
1.6.1

[PATCH 3/8] p9auth: use setresuid

[Serge Hallyn]

From: Serge E. Hallyn <serue@us.ibm.com>

Use setresuid to set userids through p9auth capability device, to
make sure MAC checks and setuid fixup are done.  In particular,
becoming root or dropping root should tweak capability sets.

Have the cred_setresuid() helper take a 'int force' argument,
which means the setuid has been authorized, so no need to
check CAP_SETUID.  (Analogous for cred_setresgid, which will
also eventually be used).

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
Cc: Greg KH <greg@kroah.com>
cc: rsc@swtch.com
Cc: Ashwin Ganti <ashwin.ganti@gmail.com>
Cc: ericvh@gmail.com
Cc: devel@linuxdriverproject.org
Cc: linux-kernel@vger.kernel.org
Cc: Ron Minnich <rminnich@gmail.com>
---
 drivers/staging/p9auth/p9auth.c |   12 ++++++++----
 include/linux/cred.h            |    8 ++++++--
 kernel/cred.c                   |   11 ++++++-----
 kernel/sys.c                    |    4 ++--
 4 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/p9auth/p9auth.c b/drivers/staging/p9auth/p9auth.c
index 70ef45b..8f70daa 100644
--- a/drivers/staging/p9auth/p9auth.c
+++ b/drivers/staging/p9auth/p9auth.c
@@ -289,11 +289,15 @@ static ssize_t cap_write(struct file *filp, const char __user *buf,
 					retval = -ENOMEM;
 					goto out;
 				}
-				new->uid = (uid_t) target_int;
-				new->suid = new->fsuid = new->euid = new->uid;
-				retval = commit_creds(new);
-				if (retval)
+				retval = cred_setresuid(new, target_int,
+					 target_int, target_int,
+					 CRED_SETID_FORCE);
+				if (retval == 0)
+					commit_creds(new);
+				else {
+					abort_creds(new);
 					goto out;
+				}
 
 				/*
 				 * Remove the capability from the list and
diff --git a/include/linux/cred.h b/include/linux/cred.h
index e35631e..6337e18 100644
--- a/include/linux/cred.h
+++ b/include/linux/cred.h
@@ -399,8 +399,12 @@ do {						\
 	*(_fsgid) = __cred->fsgid;		\
 } while(0)
 
-int cred_setresuid(struct cred *new, uid_t ruid, uid_t euid, uid_t suid);
-int cred_setresgid(struct cred *new, gid_t rgid, gid_t egid, gid_t sgid);
+#define CRED_SETID_NOFORCE 0
+#define CRED_SETID_FORCE 1
+int cred_setresuid(struct cred *new, uid_t ruid, uid_t euid, uid_t suid,
+		   int force);
+int cred_setresgid(struct cred *new, gid_t rgid, gid_t egid, gid_t sgid,
+		   int force);
 int cred_setfsuid(struct cred *new, uid_t uid, uid_t *old_fsuid);
 int cred_setfsgid(struct cred *new, gid_t gid, gid_t *old_fsgid);
 
diff --git a/kernel/cred.c b/kernel/cred.c
index 6eee59c..8640988 100644
--- a/kernel/cred.c
+++ b/kernel/cred.c
@@ -780,7 +780,8 @@ int set_create_files_as(struct cred *new, struct inode *inode)
 }
 EXPORT_SYMBOL(set_create_files_as);
 
-int cred_setresuid(struct cred *new, uid_t ruid, uid_t euid, uid_t suid)
+int cred_setresuid(struct cred *new, uid_t ruid, uid_t euid, uid_t suid,
+		   int force)
 {
 	int retval;
 	const struct cred *old;
@@ -790,7 +791,7 @@ int cred_setresuid(struct cred *new, uid_t ruid, uid_t euid, uid_t suid)
 		return retval;
 	old = current_cred();
 
-	if (!capable(CAP_SETUID)) {
+	if (!force && !capable(CAP_SETUID)) {
 		if (ruid != (uid_t) -1 && ruid != old->uid &&
 		    ruid != old->euid  && ruid != old->suid)
 			return -EPERM;
@@ -820,8 +821,8 @@ int cred_setresuid(struct cred *new, uid_t ruid, uid_t euid, uid_t suid)
 }
 EXPORT_SYMBOL_GPL(cred_setresuid);
 
-int cred_setresgid(struct cred *new, gid_t rgid, gid_t egid,
-			gid_t sgid)
+int cred_setresgid(struct cred *new, gid_t rgid, gid_t egid, gid_t sgid,
+		   int force)
 {
 	const struct cred *old = current_cred();
 	int retval;
@@ -830,7 +831,7 @@ int cred_setresgid(struct cred *new, gid_t rgid, gid_t egid,
 	if (retval)
 		return retval;
 
-	if (!capable(CAP_SETGID)) {
+	if (!force && !capable(CAP_SETGID)) {
 		if (rgid != (gid_t) -1 && rgid != old->gid &&
 		    rgid != old->egid  && rgid != old->sgid)
 			return -EPERM;
diff --git a/kernel/sys.c b/kernel/sys.c
index 1927edf..f6c2534 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -720,7 +720,7 @@ SYSCALL_DEFINE3(setresuid, uid_t, ruid, uid_t, euid, uid_t, suid)
 	if (!new)
 		return -ENOMEM;
 
-	retval = cred_setresuid(new, ruid, euid, suid);
+	retval = cred_setresuid(new, ruid, euid, suid, CRED_SETID_NOFORCE);
 	if (retval == 0)
 		return commit_creds(new);
 
@@ -752,7 +752,7 @@ SYSCALL_DEFINE3(setresgid, gid_t, rgid, gid_t, egid, gid_t, sgid)
 	if (!new)
 		return -ENOMEM;
 
-	retval = cred_setresgid(new, rgid, egid, sgid);
+	retval = cred_setresgid(new, rgid, egid, sgid, CRED_SETID_NOFORCE);
 	if (retval == 0)
 		return commit_creds(new);
 
-- 
1.6.1

[PATCH 4/8] p9auth: add CAP_GRANT_ID to authorize use of /dev/caphash

[Serge Hallyn]

From: Serge E. Hallyn <serue@us.ibm.com>

Granting userid capabilities to another task is a dangerous
privilege.  Don't just let file permissions authorize it.
Define CAP_GRANT_ID as a new capability needed to write to
/dev/caphash.

For one thing this lets us start a factotum server early on
in init, then have init drop CAP_GRANT_ID from its bounding
set so the rest of the system cannot regain it.

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
Cc: Greg KH <greg@kroah.com>
cc: rsc@swtch.com
Cc: Ashwin Ganti <ashwin.ganti@gmail.com>
Cc: ericvh@gmail.com
Cc: devel@linuxdriverproject.org
Cc: linux-kernel@vger.kernel.org
Cc: Ron Minnich <rminnich@gmail.com>
---
 drivers/staging/p9auth/p9auth.c |    4 ++++
 include/linux/capability.h      |    6 +++++-
 2 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/p9auth/p9auth.c b/drivers/staging/p9auth/p9auth.c
index 8f70daa..fb27459 100644
--- a/drivers/staging/p9auth/p9auth.c
+++ b/drivers/staging/p9auth/p9auth.c
@@ -201,6 +201,10 @@ static ssize_t cap_write(struct file *filp, const char __user *buf,
 			retval = -EINVAL;
 			goto out;
 		}
+		if (!capable(CAP_GRANT_ID)) {
+			retval = -EPERM;
+			goto out;
+		}
 		printk(KERN_INFO "Capability being written to /dev/caphash : \n");
 		hexdump(user_buf, count);
 		memcpy(node_ptr->data, user_buf, count);
diff --git a/include/linux/capability.h b/include/linux/capability.h
index 39e5ff5..ba2cbfe 100644
--- a/include/linux/capability.h
+++ b/include/linux/capability.h
@@ -355,7 +355,11 @@ struct cpu_vfs_cap_data {
 
 #define CAP_MAC_ADMIN        33
 
-#define CAP_LAST_CAP         CAP_MAC_ADMIN
+/* Allow granting setuid capabilities through p9auth /dev/caphash */
+
+#define CAP_GRANT_ID         34
+
+#define CAP_LAST_CAP         CAP_GRANT_ID
 
 #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
 
-- 
1.6.1

[PATCH 5/8] p9auth cleanup

[Serge Hallyn]

From: Serge E. Hallyn <serue@us.ibm.com>

Move the code doing the actual uid change into its own helper
function.  Next it will become a bit more complicated when I
add primary and auxiliary groups handling.

Split the handling of /dev/capuse and /dev/caphash writes into
their own functions.

Changelog:
	Jan 3: fix memory leak in error case

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
Cc: Greg KH <greg@kroah.com>
cc: rsc@swtch.com
Cc: Ashwin Ganti <ashwin.ganti@gmail.com>
Cc: ericvh@gmail.com
Cc: devel@linuxdriverproject.org
Cc: linux-kernel@vger.kernel.org
Cc: Ron Minnich <rminnich@gmail.com>
---
 drivers/staging/p9auth/p9auth.c |  308 +++++++++++++++++++++------------------
 1 files changed, 168 insertions(+), 140 deletions(-)

diff --git a/drivers/staging/p9auth/p9auth.c b/drivers/staging/p9auth/p9auth.c
index fb27459..50447d4 100644
--- a/drivers/staging/p9auth/p9auth.c
+++ b/drivers/staging/p9auth/p9auth.c
@@ -165,26 +165,180 @@ static int cap_release(struct inode *inode, struct file *filp)
 	return 0;
 }
 
+struct id_set {
+	char *source_user, *target_user;
+	char *randstr;
+	uid_t old_uid, new_uid;
+	char *full;  /* The full entry which must be freed */
+};
+
+/*
+ * read an entry.  For now it is
+ * source_user@target_user@rand
+ * Next it will become
+ * source_user@target_user@target_group@numgroups@grp1..@grpn@rand
+ */
+static int parse_user_capability(char *s, struct id_set *set)
+{
+	char *tmpu;
+
+	/*
+	 * break the supplied string into tokens with @ as the
+	 * delimiter If the string is "user1@user2@randomstring" we
+	 * need to split it and hash 'user1@user2' using 'randomstring'
+	 * as the key.
+	 */
+	tmpu = set->full = kstrdup(s, GFP_KERNEL);
+	if (!tmpu)
+		return -ENOMEM;
+
+	set->source_user = strsep(&tmpu, "@");
+	set->target_user = strsep(&tmpu, "@");
+	set->randstr = tmpu;
+	if (!set->source_user || !set->target_user || !set->randstr) {
+		kfree(set->full);
+		return -EINVAL;
+	}
+
+	set->new_uid = simple_strtoul(set->target_user, NULL, 0);
+	set->old_uid = simple_strtoul(set->source_user, NULL, 0);
+
+	return 0;
+}
+
+static int grant_id(struct id_set *set)
+{
+	struct cred *new;
+	int ret;
+
+	/*
+	 * Check whether the process writing to capuse
+	 * is actually owned by the source owner
+	 */
+	if (set->old_uid != current_uid()) {
+		printk(KERN_ALERT
+		       "Process is not owned by the source user of the capability.\n");
+		return -EFAULT;
+	}
+
+	/*
+	 * Change uid, euid, and fsuid.  The suid remains for
+	 * flexibility - though I'm torn as to the tradeoff of
+	 * usefulness vs. danger in that.
+	 */
+	new = prepare_creds();
+	if (!new)
+		return -ENOMEM;
+
+	ret = cred_setresuid(new, set->new_uid, set->new_uid, set->new_uid,
+			     CRED_SETID_FORCE);
+	if (ret == 0)
+		commit_creds(new);
+	else
+		abort_creds(new);
+
+	return ret;
+}
+
+static int add_caphash_entry(struct cap_dev *dev, char *user_buf, size_t count)
+{
+	struct cap_node *node_ptr;
+
+	if (count > CAP_NODE_SIZE)
+		return -EINVAL;
+	if (!capable(CAP_GRANT_ID))
+		return -EPERM;
+	node_ptr = kmalloc(sizeof(struct cap_node), GFP_KERNEL);
+	if (!node_ptr)
+		return -ENOMEM;
+
+	printk(KERN_INFO "Capability being written to /dev/caphash : \n");
+	hexdump(user_buf, count);
+	memcpy(node_ptr->data, user_buf, count);
+	list_add(&(node_ptr->list), &(dev->head->list));
+
+	return 0;
+}
+
+static int use_caphash_entry(struct cap_dev *dev, char *user_buf)
+{
+	struct cap_node *node;
+	struct id_set set;
+	int ret, len, found = 0;
+	char *tohash, *hashed;
+	struct list_head *pos;
+
+	if (!cap_devices[0].head)
+		return -EINVAL;
+	if (list_empty(&(cap_devices[0].head->list)))
+		return -EINVAL;
+
+	ret = parse_user_capability(user_buf, &set);
+	if (ret)
+		return ret;
+
+	/* hash the string user1@user2 with randstr as the key */
+	len = strlen(set.source_user) + strlen(set.target_user) + 1;
+	/* src, @, len, \0 */
+	tohash = kzalloc(len+1, GFP_KERNEL);
+	if (!tohash) {
+		kfree(set.full);
+		return -ENOMEM;
+	}
+	strcat(tohash, set.source_user);
+	strcat(tohash, "@");
+	strcat(tohash, set.target_user);
+	printk(KERN_ALERT "the source user is %s \n", set.source_user);
+	printk(KERN_ALERT "the target user is %s \n", set.target_user);
+	hashed = cap_hash(tohash, len, set.randstr, strlen(set.randstr));
+	kfree(set.full);
+	kfree(tohash);
+	if (NULL == hashed)
+		return -EFAULT;
+
+	/* Change the process's uid if the hash is present in the
+	 * list of hashes
+	 */
+	list_for_each(pos, &(cap_devices->head->list)) {
+		/*
+		 * Change the user id of the process if the hashes
+		 * match
+		 */
+		node = list_entry(pos, struct cap_node, list);
+		if (0 == memcmp(hashed, node->data, CAP_NODE_SIZE)) {
+			ret = grant_id(&set);
+			if (ret < 0)
+				goto out;
+
+			/* Capability may only be used once */
+			list_del(pos);
+			kfree(node);
+			found = 1;
+			break;
+		}
+	}
+	if (!found) {
+		printk(KERN_ALERT
+		       "Invalid capabiliy written to /dev/capuse\n");
+		ret = -EFAULT;
+	}
+out:
+	kfree(hashed);
+	return ret;
+}
+
 static ssize_t cap_write(struct file *filp, const char __user *buf,
 			 size_t count, loff_t *f_pos)
 {
-	struct cap_node *node_ptr, *tmp;
-	struct list_head *pos;
 	struct cap_dev *dev = filp->private_data;
 	ssize_t retval = -ENOMEM;
-	struct cred *new;
-	int len, target_int, source_int, flag = 0;
-	char *user_buf, *user_buf_running, *source_user, *target_user,
-	    *rand_str, *hash_str, *result;
+	char *user_buf;
 
 	if (down_interruptible(&dev->sem))
 		return -ERESTARTSYS;
 
-	user_buf_running = NULL;
-	hash_str = NULL;
-	node_ptr = kmalloc(sizeof(struct cap_node), GFP_KERNEL);
 	user_buf = kzalloc(count+1, GFP_KERNEL);
-	if (!node_ptr || !user_buf)
+	if (!user_buf)
 		goto out;
 
 	if (copy_from_user(user_buf, buf, count)) {
@@ -196,134 +350,11 @@ static ssize_t cap_write(struct file *filp, const char __user *buf,
 	 * If the minor number is 0 ( /dev/caphash ) then simply add the
 	 * hashed capability supplied by the user to the list of hashes
 	 */
-	if (0 == iminor(filp->f_dentry->d_inode)) {
-		if (count > CAP_NODE_SIZE) {
-			retval = -EINVAL;
-			goto out;
-		}
-		if (!capable(CAP_GRANT_ID)) {
-			retval = -EPERM;
-			goto out;
-		}
-		printk(KERN_INFO "Capability being written to /dev/caphash : \n");
-		hexdump(user_buf, count);
-		memcpy(node_ptr->data, user_buf, count);
-		list_add(&(node_ptr->list), &(dev->head->list));
-		node_ptr = NULL;
-	} else {
-		char *tmpu;
-		if (!cap_devices[0].head ||
-				list_empty(&(cap_devices[0].head->list))) {
-			retval = -EINVAL;
-			goto out;
-		}
-		/*
-		 * break the supplied string into tokens with @ as the
-		 * delimiter If the string is "user1@user2@randomstring" we
-		 * need to split it and hash 'user1@user2' using 'randomstring'
-		 * as the key.
-		 */
-		tmpu = user_buf_running = kstrdup(user_buf, GFP_KERNEL);
-		source_user = strsep(&tmpu, "@");
-		target_user = strsep(&tmpu, "@");
-		rand_str = tmpu;
-		if (!source_user || !target_user || !rand_str) {
-			retval = -EINVAL;
-			goto out;
-		}
+	if (0 == iminor(filp->f_dentry->d_inode))
+		retval = add_caphash_entry(dev, user_buf, count);
+	else
+		retval = use_caphash_entry(dev, user_buf);
 
-		/* hash the string user1@user2 with rand_str as the key */
-		len = strlen(source_user) + strlen(target_user) + 1;
-		/* src, @, len, \0 */
-		hash_str = kzalloc(len+1, GFP_KERNEL);
-		strcat(hash_str, source_user);
-		strcat(hash_str, "@");
-		strcat(hash_str, target_user);
-
-		printk(KERN_ALERT "the source user is %s \n", source_user);
-		printk(KERN_ALERT "the target user is %s \n", target_user);
-
-		result = cap_hash(hash_str, len, rand_str, strlen(rand_str));
-		if (NULL == result) {
-			retval = -EFAULT;
-			goto out;
-		}
-		memcpy(node_ptr->data, result, CAP_NODE_SIZE);  /* why? */
-		/* Change the process's uid if the hash is present in the
-		 * list of hashes
-		 */
-		list_for_each(pos, &(cap_devices->head->list)) {
-			/*
-			 * Change the user id of the process if the hashes
-			 * match
-			 */
-			if (0 ==
-			    memcmp(result,
-				   list_entry(pos, struct cap_node,
-					      list)->data,
-				   CAP_NODE_SIZE)) {
-				target_int = (unsigned int)
-				    simple_strtol(target_user, NULL, 0);
-				source_int = (unsigned int)
-				    simple_strtol(source_user, NULL, 0);
-				flag = 1;
-
-				/*
-				 * Check whether the process writing to capuse
-				 * is actually owned by the source owner
-				 */
-				if (source_int != current_uid()) {
-					printk(KERN_ALERT
-					       "Process is not owned by the source user of the capability.\n");
-					retval = -EFAULT;
-					goto out;
-				}
-				/*
-				 * Change all uids.  It might be useful to
-				 * keep suid unchanged, however that will
-				 * mean that changing from uid=0 to uid=!0
-				 * pP is not emptied (only pE is), and when
-				 * changing from  uid=!0 to  uid=0, sets are
-				 * not filled.  They will be correct after
-				 * the next exec, but this is IMO not
-				 * sufficient.  So change all uids.
-				 */
-				new = prepare_creds();
-				if (!new) {
-					retval = -ENOMEM;
-					goto out;
-				}
-				retval = cred_setresuid(new, target_int,
-					 target_int, target_int,
-					 CRED_SETID_FORCE);
-				if (retval == 0)
-					commit_creds(new);
-				else {
-					abort_creds(new);
-					goto out;
-				}
-
-				/*
-				 * Remove the capability from the list and
-				 * break
-				 */
-				tmp = list_entry(pos, struct cap_node, list);
-				list_del(pos);
-				kfree(tmp);
-				break;
-			}
-		}
-		if (0 == flag) {
-			/*
-			 * The capability is not present in the list of the
-			 * hashes stored, hence return failure
-			 */
-			printk(KERN_ALERT
-			       "Invalid capabiliy written to /dev/capuse \n");
-			retval = -EFAULT;
-			goto out;
-		}
-	}
 	*f_pos += count;
 	retval = count;
 	/* update the size */
@@ -331,10 +362,7 @@ static ssize_t cap_write(struct file *filp, const char __user *buf,
 		dev->size = *f_pos;
 
 out:
-	kfree(node_ptr);
 	kfree(user_buf);
-	kfree(user_buf_running);
-	kfree(hash_str);
 	up(&dev->sem);
 	return retval;
 }
-- 
1.6.1

[PATCH 6/8] p9auth: do groups

[Serge Hallyn]

From: Serge E. Hallyn <serue@us.ibm.com>

A p9auth capability used to be 'old_uid@new_uid@random_string'.  Now
it is 'old_uid@new_uid@new_gid@num_groups@g_1@...@g_n@random_string'.
After using the capability, credentials will be:
	suid = old_uid;
	sgid = old_gid;
	uid = euid = fsuid = new_uid
	gid = egid = fsgid = new_gid
	auxiliary groups = g_1, g_2, ..., g_n

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
Cc: Greg KH <greg@kroah.com>
cc: rsc@swtch.com
Cc: Ashwin Ganti <ashwin.ganti@gmail.com>
Cc: ericvh@gmail.com
Cc: devel@linuxdriverproject.org
Cc: linux-kernel@vger.kernel.org
Cc: Ron Minnich <rminnich@gmail.com>
---
 drivers/staging/p9auth/p9auth.c |   99 +++++++++++++++++++++++++--------------
 1 files changed, 64 insertions(+), 35 deletions(-)

diff --git a/drivers/staging/p9auth/p9auth.c b/drivers/staging/p9auth/p9auth.c
index 50447d4..e94c4fe 100644
--- a/drivers/staging/p9auth/p9auth.c
+++ b/drivers/staging/p9auth/p9auth.c
@@ -167,8 +167,10 @@ static int cap_release(struct inode *inode, struct file *filp)
 
 struct id_set {
 	char *source_user, *target_user;
-	char *randstr;
 	uid_t old_uid, new_uid;
+	gid_t new_gid;
+	unsigned int ngroups;
+	struct group_info *newgroups;
 	char *full;  /* The full entry which must be freed */
 };
 
@@ -180,7 +182,8 @@ struct id_set {
  */
 static int parse_user_capability(char *s, struct id_set *set)
 {
-	char *tmpu;
+	char *tmp, *tmpu;
+	int i, ret;
 
 	/*
 	 * break the supplied string into tokens with @ as the
@@ -192,18 +195,45 @@ static int parse_user_capability(char *s, struct id_set *set)
 	if (!tmpu)
 		return -ENOMEM;
 
+	ret = -EINVAL;
 	set->source_user = strsep(&tmpu, "@");
 	set->target_user = strsep(&tmpu, "@");
-	set->randstr = tmpu;
-	if (!set->source_user || !set->target_user || !set->randstr) {
-		kfree(set->full);
-		return -EINVAL;
-	}
+	tmp = strsep(&tmpu, "@");
+	if (!set->source_user || !set->target_user || !tmp)
+		goto out;
 
 	set->new_uid = simple_strtoul(set->target_user, NULL, 0);
 	set->old_uid = simple_strtoul(set->source_user, NULL, 0);
+	set->new_gid = simple_strtoul(tmp, NULL, 0);
 
-	return 0;
+	tmp = strsep(&tmpu, "@");
+	if (!tmp)
+		goto out;
+	if (sscanf(tmp, "%d", &set->ngroups) != 1 || set->ngroups < 0)
+		goto out;
+
+	ret = -ENOMEM;
+	set->newgroups = groups_alloc(set->ngroups);
+	if (!set->newgroups)
+		goto out;
+
+	ret = -EINVAL;
+	for (i = 0; i < set->ngroups; i++) {
+		gid_t g;
+
+		tmp = strsep(&tmpu, "@");
+		if (!tmp || sscanf(tmp, "%d", &g) != 1) {
+			groups_free(set->newgroups);
+			goto out;
+		}
+		GROUP_AT(set->newgroups, i) = g;
+	}
+
+	ret = 0;
+
+out:
+	kfree(set->full);
+	return ret;
 }
 
 static int grant_id(struct id_set *set)
@@ -230,8 +260,13 @@ static int grant_id(struct id_set *set)
 	if (!new)
 		return -ENOMEM;
 
-	ret = cred_setresuid(new, set->new_uid, set->new_uid, set->new_uid,
-			     CRED_SETID_FORCE);
+	ret = set_groups(new, set->newgroups);
+	if (!ret)
+		ret = cred_setresgid(new, set->new_gid, set->new_gid,
+				     set->new_gid, CRED_SETID_FORCE);
+	if (!ret)
+		ret = cred_setresuid(new, set->new_uid, set->new_uid,
+				     set->new_uid, CRED_SETID_FORCE);
 	if (ret == 0)
 		commit_creds(new);
 	else
@@ -260,12 +295,12 @@ static int add_caphash_entry(struct cap_dev *dev, char *user_buf, size_t count)
 	return 0;
 }
 
-static int use_caphash_entry(struct cap_dev *dev, char *user_buf)
+static int use_caphash_entry(struct cap_dev *dev, char *ubuf)
 {
 	struct cap_node *node;
 	struct id_set set;
-	int ret, len, found = 0;
-	char *tohash, *hashed;
+	int ret, found = 0;
+	char *hashed = NULL, *sep;
 	struct list_head *pos;
 
 	if (!cap_devices[0].head)
@@ -273,37 +308,30 @@ static int use_caphash_entry(struct cap_dev *dev, char *user_buf)
 	if (list_empty(&(cap_devices[0].head->list)))
 		return -EINVAL;
 
-	ret = parse_user_capability(user_buf, &set);
+	ret = parse_user_capability(ubuf, &set);
 	if (ret)
 		return ret;
 
-	/* hash the string user1@user2 with randstr as the key */
-	len = strlen(set.source_user) + strlen(set.target_user) + 1;
-	/* src, @, len, \0 */
-	tohash = kzalloc(len+1, GFP_KERNEL);
-	if (!tohash) {
-		kfree(set.full);
-		return -ENOMEM;
+	/*
+	 * hash the string user1@user2@ngrp@grp... with randstr as the key
+	 * XXX is there any vulnerability we're opening ourselves up to by
+	 * not rebuilding the string from its components?
+	 */
+	sep = strrchr(ubuf, '@');
+	if (sep) {
+		char *rand = sep + 1;
+		*sep = '\0';
+		hashed = cap_hash(ubuf, strlen(ubuf), rand, strlen(rand));
+	}
+	if (NULL == hashed) {
+		ret = -EINVAL;
+		goto out;
 	}
-	strcat(tohash, set.source_user);
-	strcat(tohash, "@");
-	strcat(tohash, set.target_user);
-	printk(KERN_ALERT "the source user is %s \n", set.source_user);
-	printk(KERN_ALERT "the target user is %s \n", set.target_user);
-	hashed = cap_hash(tohash, len, set.randstr, strlen(set.randstr));
-	kfree(set.full);
-	kfree(tohash);
-	if (NULL == hashed)
-		return -EFAULT;
 
 	/* Change the process's uid if the hash is present in the
 	 * list of hashes
 	 */
 	list_for_each(pos, &(cap_devices->head->list)) {
-		/*
-		 * Change the user id of the process if the hashes
-		 * match
-		 */
 		node = list_entry(pos, struct cap_node, list);
 		if (0 == memcmp(hashed, node->data, CAP_NODE_SIZE)) {
 			ret = grant_id(&set);
@@ -323,6 +351,7 @@ static int use_caphash_entry(struct cap_dev *dev, char *user_buf)
 		ret = -EFAULT;
 	}
 out:
+	put_group_info(set.newgroups);
 	kfree(hashed);
 	return ret;
 }
-- 
1.6.1

[PATCH 7/8] p9auth: add cap_node timeout

[Serge Hallyn]

From: Serge E. Hallyn <serue@us.ibm.com>

Mark each caphash entry with the current time.  When a new caphash is
added, any entries which were added more than two minutes ago are
discarded.

We may want to make two minutes configurable, or may want to also
discard entries if more than N entries are on the list (to prevent
a forced OOM by a misbehaving privileged process).  The purpose
of this patch is only to prevent gradually consuming more and more
memory due to "legitimate" unused entries.

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
Cc: Greg KH <greg@kroah.com>
cc: rsc@swtch.com
Cc: Ashwin Ganti <ashwin.ganti@gmail.com>
Cc: ericvh@gmail.com
Cc: devel@linuxdriverproject.org
Cc: linux-kernel@vger.kernel.org
Cc: Ron Minnich <rminnich@gmail.com>
---
 drivers/staging/p9auth/p9auth.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/p9auth/p9auth.c b/drivers/staging/p9auth/p9auth.c
index e94c4fe..6012bd9 100644
--- a/drivers/staging/p9auth/p9auth.c
+++ b/drivers/staging/p9auth/p9auth.c
@@ -40,6 +40,7 @@
 
 struct cap_node {
 	char data[CAP_NODE_SIZE];
+	unsigned long time_created;
 	struct list_head list;
 };
 
@@ -275,6 +276,23 @@ static int grant_id(struct id_set *set)
 	return ret;
 }
 
+/* Expose this through sysctl eventually?  2 min timeout for hashes */
+
+static int cap_timeout = 120;
+static void remove_old_entries(struct cap_dev *dev)
+{
+	struct cap_node *node, *tmp;
+
+	if (dev->head == NULL)
+		return;
+	list_for_each_entry_safe(node, tmp, &dev->head->list, list) {
+		if (node->time_created + HZ * cap_timeout < jiffies) {
+			list_del(&node->list);
+			kfree(node);
+		}
+	}
+}
+
 static int add_caphash_entry(struct cap_dev *dev, char *user_buf, size_t count)
 {
 	struct cap_node *node_ptr;
@@ -290,7 +308,9 @@ static int add_caphash_entry(struct cap_dev *dev, char *user_buf, size_t count)
 	printk(KERN_INFO "Capability being written to /dev/caphash : \n");
 	hexdump(user_buf, count);
 	memcpy(node_ptr->data, user_buf, count);
+	node_ptr->time_created = jiffies;
 	list_add(&(node_ptr->list), &(dev->head->list));
+	remove_old_entries(dev);
 
 	return 0;
 }
-- 
1.6.1

[PATCH 8/8] p9auth: don't trim entries on write-only open

[Serge Hallyn]

From: Serge E. Hallyn <serue@us.ibm.com>

If we want to support an admin clearing all entries, let's
not do it through some subtle hidden channel, but rather
implement a 'CLEAR' command to /dev/caphash, which requires
privilege to use.

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
Cc: Greg KH <greg@kroah.com>
cc: rsc@swtch.com
Cc: Ashwin Ganti <ashwin.ganti@gmail.com>
Cc: ericvh@gmail.com
Cc: devel@linuxdriverproject.org
Cc: linux-kernel@vger.kernel.org
Cc: Ron Minnich <rminnich@gmail.com>
---
 drivers/staging/p9auth/p9auth.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/p9auth/p9auth.c b/drivers/staging/p9auth/p9auth.c
index 6012bd9..06128c3 100644
--- a/drivers/staging/p9auth/p9auth.c
+++ b/drivers/staging/p9auth/p9auth.c
@@ -146,13 +146,6 @@ static int cap_open(struct inode *inode, struct file *filp)
 	dev = container_of(inode->i_cdev, struct cap_dev, cdev);
 	filp->private_data = dev;
 
-	/* trim to 0 the length of the device if open was write-only */
-	if ((filp->f_flags & O_ACCMODE) == O_WRONLY) {
-		if (down_interruptible(&dev->sem))
-			return -ERESTARTSYS;
-		cap_trim(dev);
-		up(&dev->sem);
-	}
 	/* initialise the head if it is NULL */
 	if (dev->head == NULL) {
 		dev->head = kmalloc(sizeof(struct cap_node), GFP_KERNEL);
-- 
1.6.1

Re: [PATCH 1/8] p9auth: set fsuid

[Greg KH]

On Tue, Feb 16, 2010 at 04:44:54PM -0600, Serge Hallyn wrote:
> From: Serge E. Hallyn <serue@us.ibm.com>
> 
> fsuid should always trail euid changes.  So p9auth should
> set fsuid as well when it sets ruid and euid.  Whether the
> suid should also be set is an open question - keeping the
> old uid in suid may be useful, or may just serve to trick
> lazy userspace.
> 
> Note that so long as we do not also set suid, the setuid_fixup()
> code will not (when we later switch to setresuid()) fully
> fill/clear capability sets.  So while I had previously thought
> that keeping suid unchanged would be useful, I think it is
> better to change all uids.

What is your goal for the p9auth code?  Currently it is deleted in
linux-next due to a lack of development.  I see you have some cleanup
patches, but I can't apply them unless you get the non-staging patches
accepted.

If I bring the driver back from deletion, will you work to fix it up and
get it merged into mainline?

What's the word on the non-staging patches in this series being
accepted?

thanks,

greg k-h

Re: [PATCH 1/8] p9auth: set fsuid

[Serge E. Hallyn]

Quoting Greg KH (greg@kroah.com):
> On Tue, Feb 16, 2010 at 04:44:54PM -0600, Serge Hallyn wrote:
> > From: Serge E. Hallyn <serue@us.ibm.com>
> > 
> > fsuid should always trail euid changes.  So p9auth should
> > set fsuid as well when it sets ruid and euid.  Whether the
> > suid should also be set is an open question - keeping the
> > old uid in suid may be useful, or may just serve to trick
> > lazy userspace.
> > 
> > Note that so long as we do not also set suid, the setuid_fixup()
> > code will not (when we later switch to setresuid()) fully
> > fill/clear capability sets.  So while I had previously thought
> > that keeping suid unchanged would be useful, I think it is
> > better to change all uids.

Hi Greg,

> What is your goal for the p9auth code?  Currently it is deleted in
> linux-next due to a lack of development.  I see you have some cleanup
> patches, but I can't apply them unless you get the non-staging patches
> accepted.

Sorry, what do you mean by 'the non-staging patches'?  Do you mean
the staging patches that were dropped, the cleanup patches (that
wouldn't make sense), or another set of patches?

> If I bring the driver back from deletion, will you work to fix it up and
> get it merged into mainline?

Yes.

> What's the word on the non-staging patches in this series being
> accepted?

Again, I'm not quite sure which you mean by the non-staging patches,
or what you mean by accepted - do you mean general community acceptance
of the base p9auth patches, or acceptance of my p9uath patches by
Ashwin etc?

thanks,
-serge

Re: [PATCH 1/8] p9auth: set fsuid

[Greg KH]

On Thu, Feb 25, 2010 at 10:05:53PM -0600, Serge E. Hallyn wrote:
> Quoting Greg KH (greg@kroah.com):
> > On Tue, Feb 16, 2010 at 04:44:54PM -0600, Serge Hallyn wrote:
> > > From: Serge E. Hallyn <serue@us.ibm.com>
> > > 
> > > fsuid should always trail euid changes.  So p9auth should
> > > set fsuid as well when it sets ruid and euid.  Whether the
> > > suid should also be set is an open question - keeping the
> > > old uid in suid may be useful, or may just serve to trick
> > > lazy userspace.
> > > 
> > > Note that so long as we do not also set suid, the setuid_fixup()
> > > code will not (when we later switch to setresuid()) fully
> > > fill/clear capability sets.  So while I had previously thought
> > > that keeping suid unchanged would be useful, I think it is
> > > better to change all uids.
> 
> Hi Greg,
> 
> > What is your goal for the p9auth code?  Currently it is deleted in
> > linux-next due to a lack of development.  I see you have some cleanup
> > patches, but I can't apply them unless you get the non-staging patches
> > accepted.
> 
> Sorry, what do you mean by 'the non-staging patches'?  Do you mean
> the staging patches that were dropped, the cleanup patches (that
> wouldn't make sense), or another set of patches?

I mean the ones that were not for the drivers/staging/p9auth/ directory.
I can't apply patches to the staging git tree for stuff outside of
drivers/staging/

> > If I bring the driver back from deletion, will you work to fix it up and
> > get it merged into mainline?
> 
> Yes.

Great.

> > What's the word on the non-staging patches in this series being
> > accepted?
> 
> Again, I'm not quite sure which you mean by the non-staging patches,
> or what you mean by accepted - do you mean general community acceptance
> of the base p9auth patches, or acceptance of my p9uath patches by
> Ashwin etc?

Well. I was referring to the patches outside of the drivers/staging/
directory, but also, it would be good to see if Ashwin has any
objections to them.

Once you two work it out, care to resend them?

thanks,

greg k-h

Re: [PATCH 1/8] p9auth: set fsuid

[Serge E. Hallyn]

Quoting Greg KH (greg@kroah.com):
> On Thu, Feb 25, 2010 at 10:05:53PM -0600, Serge E. Hallyn wrote:
> > Quoting Greg KH (greg@kroah.com):
> > > On Tue, Feb 16, 2010 at 04:44:54PM -0600, Serge Hallyn wrote:
> > > > From: Serge E. Hallyn <serue@us.ibm.com>
> > > > 
> > > > fsuid should always trail euid changes.  So p9auth should
> > > > set fsuid as well when it sets ruid and euid.  Whether the
> > > > suid should also be set is an open question - keeping the
> > > > old uid in suid may be useful, or may just serve to trick
> > > > lazy userspace.
> > > > 
> > > > Note that so long as we do not also set suid, the setuid_fixup()
> > > > code will not (when we later switch to setresuid()) fully
> > > > fill/clear capability sets.  So while I had previously thought
> > > > that keeping suid unchanged would be useful, I think it is
> > > > better to change all uids.
> > 
> > Hi Greg,
> > 
> > > What is your goal for the p9auth code?  Currently it is deleted in
> > > linux-next due to a lack of development.  I see you have some cleanup
> > > patches, but I can't apply them unless you get the non-staging patches
> > > accepted.
> > 
> > Sorry, what do you mean by 'the non-staging patches'?  Do you mean
> > the staging patches that were dropped, the cleanup patches (that
> > wouldn't make sense), or another set of patches?
> 
> I mean the ones that were not for the drivers/staging/p9auth/ directory.
> I can't apply patches to the staging git tree for stuff outside of
> drivers/staging/

Ah, ok, I see - I didn't realize you restricted patches to under
staging.  Makes sense obviously.

> > > If I bring the driver back from deletion, will you work to fix it up and
> > > get it merged into mainline?
> > 
> > Yes.
> 
> Great.
> 
> > > What's the word on the non-staging patches in this series being
> > > accepted?
> > 
> > Again, I'm not quite sure which you mean by the non-staging patches,
> > or what you mean by accepted - do you mean general community acceptance
> > of the base p9auth patches, or acceptance of my p9uath patches by
> > Ashwin etc?
> 
> Well. I was referring to the patches outside of the drivers/staging/
> directory, but also, it would be good to see if Ashwin has any
> objections to them.
> 
> Once you two work it out, care to resend them?

Ok, and I'll add some more cc:s to get more feedback on the
external patches.

Ashwin, Ron, Eric, (whoever else cares to take a look) I will put
up a git tree hopefully this weekend or monday... hmm, hang on, already
exists - you can take a look at
http://git.kernel.org/gitweb.cgi?p=linux/kernel/git/sergeh/linux-cr.git;a=shortlog;h=refs/heads/p9auth.feb16.3

Please let me know if you have any comments.  I'm going to add user
namespace tags and then take another step back and do general
cleanups, but the API wouldn't change any more.

thanks,
-serge

Re: [PATCH 1/8] p9auth: set fsuid

[Ashwin Ganti]

On Fri, Feb 26, 2010 at 10:19 AM, Serge E. Hallyn <serue@us.ibm.com> wrote:
> Ashwin, Ron, Eric, (whoever else cares to take a look) I will put
> up a git tree hopefully this weekend or monday... hmm, hang on, already
> exists - you can take a look at
> http://git.kernel.org/gitweb.cgi?p=linux/kernel/git/sergeh/linux-cr.git;a=shortlog;h=refs/heads/p9auth.feb16.3
>

Greg/Serge: Sorry for the delay. I am okay with the patches in
general.  Thanks for making the reaping of old capabilities change as
well along with these.

The only comment I have is that it would be nice to have a README file
in the p9auth directory describing these changes to the semantics.
Especially since we are handling groups, file system checks etc. which
is going through the capability string, it would be easier and helpful
to understand if there is some basic documentation on each of these
changes.

Also, I don't know if you want to further get them looked by someone
more knowledgeable to comment on the implications of modifying/moving
the set*id functions over but they look alright to me.

I have requested Eric too a couple of days ago to take a look at these
patches if he finds time.

Thanks,
Ashwin

Re: [PATCH 1/8] p9auth: set fsuid

[Serge E. Hallyn]

Quoting Ashwin Ganti (ashwin.ganti@gmail.com):
> On Fri, Feb 26, 2010 at 10:19 AM, Serge E. Hallyn <serue@us.ibm.com> wrote:
> > Ashwin, Ron, Eric, (whoever else cares to take a look) I will put
> > up a git tree hopefully this weekend or monday... hmm, hang on, already
> > exists - you can take a look at
> > http://git.kernel.org/gitweb.cgi?p=linux/kernel/git/sergeh/linux-cr.git;a=shortlog;h=refs/heads/p9auth.feb16.3
> >
> 
> Greg/Serge: Sorry for the delay. I am okay with the patches in
> general.  Thanks for making the reaping of old capabilities change as
> well along with these.
> 
> The only comment I have is that it would be nice to have a README file
> in the p9auth directory describing these changes to the semantics.

Will do.

> Especially since we are handling groups, file system checks etc. which
> is going through the capability string, it would be easier and helpful
> to understand if there is some basic documentation on each of these
> changes.
> 
> Also, I don't know if you want to further get them looked by someone

Yeah I'm going to have to re-send and cc: some core maintainers to get
their acks (or nacks).

> more knowledgeable to comment on the implications of modifying/moving
> the set*id functions over but they look alright to me.

If they look ok after I resend them, could you please send an ack I
can tack onto the patches to help out maintainers?

> I have requested Eric too a couple of days ago to take a look at these
> patches if he finds time.

Thanks, I'll give it a few more days to see if he sends comments before
resending.

> Thanks,
> Ashwin

thanks,
-serge