Re: [PATCH 5/6] file capabilities: remove needless inline functions
From: Serge E. Hallyn
Date: Mon Sep 29 2008 - 17:54:01 EST
Quoting Serge E. Hallyn (serue@xxxxxxxxxx):
> Quoting Andrew G. Morgan (morgan@xxxxxxxxxx):
> > -----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@xxxxxxxxxx>
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/