Re: [PATCH 5/6] file capabilities: remove needless inline functions

["Serge E. Hallyn" <serue@us.ibm.com>]

Quoting Serge E. Hallyn (serue@us.ibm.com):
> Quoting Andrew G. Morgan (morgan@kernel.org):
> > -----BEGIN PGP SIGNED MESSAGE-----
> > Hash: SHA1
> > 
> > Serge,
> > 
> > I'd much rather simply remove the target argument from the
> > security_capset_check() call. Relying on the caller to not do something
> 
> Yes, I did do that in a patch on top of all of these.  It increased the
> kernel size by 8 (or was it 16) bytes :)  I assume that was because
> now there were more references to 'current' which is inlined?  So I
> should be able to fix that by saving current() away at the top of
> the fn.
> 
> Will try that.
> 
> thanks,
> -serge
> 
> > bad seems fragile... If the code internally operates on current only,
> > then it doesn't need a target argument... No? (Evidently, such a change
> > is also needed to selinux_capset_check() too, but this doesn't look like
> > it will pose a problem for the selinux code.)

Here is a patch (on top of the others) doing that.

thanks,
-serge

Subject: [PATCH] capabilities: remove target argument from capset_check and set

Since it is no longer possible to set capabilities on another task,
remove the 'target' option from capset_set and capset_check.

Since the userspace API shouldn't change, we maintain 'pid' as a
valid option to the actual sys_capset(), continuing to return
-EPERM when pid is not current's pid.

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>

---

 include/linux/security.h |   49 ++++++++++++++++------------------------------
 kernel/capability.c      |    6 ++----
 security/commoncap.c     |   29 +++++++++++++++------------
 security/security.c      |   18 ++++++++---------
 security/selinux/hooks.c |   15 ++++++++------
 5 files changed, 51 insertions(+), 66 deletions(-)

35b1a7906d26fd10da34e5cd350470abb812a91c
diff --git a/include/linux/security.h b/include/linux/security.h
index 80c4d00..7b9f7a2 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -49,8 +49,8 @@ extern int cap_settime(struct timespec *
 extern int cap_ptrace_may_access(struct task_struct *child, unsigned int mode);
 extern int cap_ptrace_traceme(struct task_struct *parent);
 extern int cap_capget(struct task_struct *target, kernel_cap_t *effective, kernel_cap_t *inheritable, kernel_cap_t *permitted);
-extern int cap_capset_check(struct task_struct *target, kernel_cap_t *effective, kernel_cap_t *inheritable, kernel_cap_t *permitted);
-extern void cap_capset_set(struct task_struct *target, kernel_cap_t *effective, kernel_cap_t *inheritable, kernel_cap_t *permitted);
+extern int cap_capset_check(kernel_cap_t *effective, kernel_cap_t *inheritable, kernel_cap_t *permitted);
+extern void cap_capset_set(kernel_cap_t *effective, kernel_cap_t *inheritable, kernel_cap_t *permitted);
 extern int cap_bprm_set_security(struct linux_binprm *bprm);
 extern void cap_bprm_apply_creds(struct linux_binprm *bprm, int unsafe);
 extern int cap_bprm_secureexec(struct linux_binprm *bprm);
@@ -1187,24 +1187,15 @@ static inline void security_free_mnt_opt
  *	Return 0 if the capability sets were successfully obtained.
  * @capset_check:
  *	Check permission before setting the @effective, @inheritable, and
- *	@permitted capability sets for the @target process.
- *	Caveat:  @target is also set to current if a set of processes is
- *	specified (i.e. all processes other than current and init or a
- *	particular process group).  Hence, the capset_set hook may need to
- *	revalidate permission to the actual target process.
- *	@target contains the task_struct structure for target process.
+ *	@permitted capability sets for the current process.
  *	@effective contains the effective capability set.
  *	@inheritable contains the inheritable capability set.
  *	@permitted contains the permitted capability set.
  *	Return 0 if permission is granted.
  * @capset_set:
  *	Set the @effective, @inheritable, and @permitted capability sets for
- *	the @target process.  Since capset_check cannot always check permission
- *	to the real @target process, this hook may also perform permission
- *	checking to determine if the current process is allowed to set the
- *	capability sets of the @target process.  However, this hook has no way
- *	of returning an error due to the structure of the sys_capset code.
- *	@target contains the task_struct structure for target process.
+ *	the current process.  This hook has no way of returning an error due to
+ *	the structure of the sys_capset code.
  *	@effective contains the effective capability set.
  *	@inheritable contains the inheritable capability set.
  *	@permitted contains the permitted capability set.
@@ -1299,12 +1290,10 @@ struct security_operations {
 	int (*capget) (struct task_struct *target,
 		       kernel_cap_t *effective,
 		       kernel_cap_t *inheritable, kernel_cap_t *permitted);
-	int (*capset_check) (struct task_struct *target,
-			     kernel_cap_t *effective,
+	int (*capset_check) (kernel_cap_t *effective,
 			     kernel_cap_t *inheritable,
 			     kernel_cap_t *permitted);
-	void (*capset_set) (struct task_struct *target,
-			    kernel_cap_t *effective,
+	void (*capset_set) (kernel_cap_t *effective,
 			    kernel_cap_t *inheritable,
 			    kernel_cap_t *permitted);
 	int (*capable) (struct task_struct *tsk, int cap);
@@ -1573,12 +1562,10 @@ int security_capget(struct task_struct *
 		    kernel_cap_t *effective,
 		    kernel_cap_t *inheritable,
 		    kernel_cap_t *permitted);
-int security_capset_check(struct task_struct *target,
-			  kernel_cap_t *effective,
+int security_capset_check(kernel_cap_t *effective,
 			  kernel_cap_t *inheritable,
 			  kernel_cap_t *permitted);
-void security_capset_set(struct task_struct *target,
-			 kernel_cap_t *effective,
+void security_capset_set(kernel_cap_t *effective,
 			 kernel_cap_t *inheritable,
 			 kernel_cap_t *permitted);
 int security_capable(struct task_struct *tsk, int cap);
@@ -1768,20 +1755,18 @@ static inline int security_capget(struct
 	return cap_capget(target, effective, inheritable, permitted);
 }
 
-static inline int security_capset_check(struct task_struct *target,
-					 kernel_cap_t *effective,
-					 kernel_cap_t *inheritable,
-					 kernel_cap_t *permitted)
+static inline int security_capset_check(kernel_cap_t *effective,
+					kernel_cap_t *inheritable,
+					kernel_cap_t *permitted)
 {
-	return cap_capset_check(target, effective, inheritable, permitted);
+	return cap_capset_check(effective, inheritable, permitted);
 }
 
-static inline void security_capset_set(struct task_struct *target,
-					kernel_cap_t *effective,
-					kernel_cap_t *inheritable,
-					kernel_cap_t *permitted)
+static inline void security_capset_set(kernel_cap_t *effective,
+				       kernel_cap_t *inheritable,
+				       kernel_cap_t *permitted)
 {
-	cap_capset_set(target, effective, inheritable, permitted);
+	cap_capset_set(effective, inheritable, permitted);
 }
 
 static inline int security_capable(struct task_struct *tsk, int cap)
diff --git a/kernel/capability.c b/kernel/capability.c
index 92dd85b..f119288 100644
--- a/kernel/capability.c
+++ b/kernel/capability.c
@@ -305,15 +305,13 @@ asmlinkage long sys_capset(cap_user_head
 	 */
 	spin_lock(&task_capability_lock);
 
-	ret = security_capset_check(current, &effective, &inheritable,
-				    &permitted);
+	ret = security_capset_check(&effective, &inheritable, &permitted);
 	/*
 	 * Having verified that the proposed changes are
 	 * legal, we now put them into effect.
 	 */
 	if (!ret)
-		security_capset_set(current, &effective, &inheritable,
-				    &permitted);
+		security_capset_set(&effective, &inheritable, &permitted);
 	spin_unlock(&task_capability_lock);
 
 
diff --git a/security/commoncap.c b/security/commoncap.c
index 43bba7a..031bb90 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -103,27 +103,29 @@ static inline int cap_inh_is_capped(void
 	return (cap_capable(current, CAP_SETPCAP) != 0);
 }
 
-int cap_capset_check (struct task_struct *target, kernel_cap_t *effective,
-		      kernel_cap_t *inheritable, kernel_cap_t *permitted)
+int cap_capset_check (kernel_cap_t *effective, kernel_cap_t *inheritable,
+		      kernel_cap_t *permitted)
 {
+	struct task_struct *t = current;
+
 	if (cap_inh_is_capped()
 	    && !cap_issubset(*inheritable,
-			     cap_combine(target->cap_inheritable,
-					 current->cap_permitted))) {
+			     cap_combine(t->cap_inheritable,
+					 t->cap_permitted))) {
 		/* incapable of using this inheritable set */
 		return -EPERM;
 	}
 	if (!cap_issubset(*inheritable,
-			   cap_combine(target->cap_inheritable,
-				       current->cap_bset))) {
+			   cap_combine(t->cap_inheritable,
+				       t->cap_bset))) {
 		/* no new pI capabilities outside bounding set */
 		return -EPERM;
 	}
 
 	/* verify restrictions on target's new Permitted set */
 	if (!cap_issubset (*permitted,
-			   cap_combine (target->cap_permitted,
-					current->cap_permitted))) {
+			   cap_combine (t->cap_permitted,
+					t->cap_permitted))) {
 		return -EPERM;
 	}
 
@@ -135,12 +137,13 @@ int cap_capset_check (struct task_struct
 	return 0;
 }
 
-void cap_capset_set (struct task_struct *target, kernel_cap_t *effective,
-		     kernel_cap_t *inheritable, kernel_cap_t *permitted)
+void cap_capset_set (kernel_cap_t *effective, kernel_cap_t *inheritable,
+		     kernel_cap_t *permitted)
 {
-	target->cap_effective = *effective;
-	target->cap_inheritable = *inheritable;
-	target->cap_permitted = *permitted;
+	struct task_struct *t = current;
+	t->cap_effective = *effective;
+	t->cap_inheritable = *inheritable;
+	t->cap_permitted = *permitted;
 }
 
 static inline void bprm_clear_caps(struct linux_binprm *bprm)
diff --git a/security/security.c b/security/security.c
index 3a4b4f5..78502ac 100644
--- a/security/security.c
+++ b/security/security.c
@@ -145,20 +145,18 @@ int security_capget(struct task_struct *
 	return security_ops->capget(target, effective, inheritable, permitted);
 }
 
-int security_capset_check(struct task_struct *target,
-			   kernel_cap_t *effective,
-			   kernel_cap_t *inheritable,
-			   kernel_cap_t *permitted)
+int security_capset_check(kernel_cap_t *effective,
+			  kernel_cap_t *inheritable,
+			  kernel_cap_t *permitted)
 {
-	return security_ops->capset_check(target, effective, inheritable, permitted);
+	return security_ops->capset_check(effective, inheritable, permitted);
 }
 
-void security_capset_set(struct task_struct *target,
-			  kernel_cap_t *effective,
-			  kernel_cap_t *inheritable,
-			  kernel_cap_t *permitted)
+void security_capset_set(kernel_cap_t *effective,
+			 kernel_cap_t *inheritable,
+			 kernel_cap_t *permitted)
 {
-	security_ops->capset_set(target, effective, inheritable, permitted);
+	security_ops->capset_set(effective, inheritable, permitted);
 }
 
 int security_capable(struct task_struct *tsk, int cap)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 03fc6a8..d06e943 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1780,22 +1780,23 @@ static int selinux_capget(struct task_st
 	return secondary_ops->capget(target, effective, inheritable, permitted);
 }
 
-static int selinux_capset_check(struct task_struct *target, kernel_cap_t *effective,
-				kernel_cap_t *inheritable, kernel_cap_t *permitted)
+static int selinux_capset_check(kernel_cap_t *effective, kernel_cap_t *inheritable,
+				kernel_cap_t *permitted)
 {
 	int error;
+	struct task_struct *t = current;
 
-	error = secondary_ops->capset_check(target, effective, inheritable, permitted);
+	error = secondary_ops->capset_check(effective, inheritable, permitted);
 	if (error)
 		return error;
 
-	return task_has_perm(current, target, PROCESS__SETCAP);
+	return task_has_perm(t, t, PROCESS__SETCAP);
 }
 
-static void selinux_capset_set(struct task_struct *target, kernel_cap_t *effective,
-			       kernel_cap_t *inheritable, kernel_cap_t *permitted)
+static void selinux_capset_set(kernel_cap_t *effective, kernel_cap_t *inheritable,
+			       kernel_cap_t *permitted)
 {
-	secondary_ops->capset_set(target, effective, inheritable, permitted);
+	secondary_ops->capset_set(effective, inheritable, permitted);
 }
 
 static int selinux_capable(struct task_struct *tsk, int cap)
-- 
1.1.6