Messages in this thread Patch in this message | | | Date | Fri, 26 Sep 2008 21:58:22 -0700 | From | "Andrew G. Morgan" <> | Subject | Re: [PATCH 4/6] file capabilities: clean up setcap code |
| |
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Serge,
I have to say I'm a bit confused by this one. Specifically, the cap_get_target_pid() change. In your 5/6 patch, you say this change ("the previous patch") makes the kernel bigger? Is this because of the cap_get_target_pid() changes? Since you are fighting to reduce space, if it bloats the code does the cap_get_target_pid() part of the change make sense?
Cheers
Andrew
Serge E. Hallyn wrote: > Clean up the sys_capset codepath a bit to account for the fact > that you can now not ever, never, capset on another task. > > Signed-off-by: Serge E. Hallyn <serue@us.ibm.com> > --- > kernel/capability.c | 83 +++++++++++++++++++------------------------------- > 1 files changed, 32 insertions(+), 51 deletions(-) > > diff --git a/kernel/capability.c b/kernel/capability.c > index d39c989..92dd85b 100644 > --- a/kernel/capability.c > +++ b/kernel/capability.c > @@ -132,46 +132,31 @@ static int cap_validate_magic(cap_user_header_t header, unsigned *tocopy) > * process. The net result is that we can limit our use of locks to > * when we are reading the caps of another process. > */ > -static inline int cap_get_target_pid(pid_t pid, kernel_cap_t *pEp, > +static int cap_get_target_pid(pid_t pid, kernel_cap_t *pEp, > kernel_cap_t *pIp, kernel_cap_t *pPp) > { > int ret; > + struct task_struct *target; > > - if (pid && (pid != task_pid_vnr(current))) { > - struct task_struct *target; > + if (!pid || pid == task_pid_vnr(current)) > + return security_capget(current, pEp, pIp, pPp); > > - spin_lock(&task_capability_lock); > - read_lock(&tasklist_lock); > + spin_lock(&task_capability_lock); > + read_lock(&tasklist_lock); > > - target = find_task_by_vpid(pid); > - if (!target) > - ret = -ESRCH; > - else > - ret = security_capget(target, pEp, pIp, pPp); > + target = find_task_by_vpid(pid); > + if (!target) > + ret = -ESRCH; > + else > + ret = security_capget(target, pEp, pIp, pPp); > > - read_unlock(&tasklist_lock); > - spin_unlock(&task_capability_lock); > - } else > - ret = security_capget(current, pEp, pIp, pPp); > + read_unlock(&tasklist_lock); > + spin_unlock(&task_capability_lock); > > return ret; > } > > /* > - * With filesystem capability support configured, the kernel does not > - * permit the changing of capabilities in one process by another > - * process. (CAP_SETPCAP has much less broad semantics when configured > - * this way.) > - */ > -static inline int do_sys_capset_other_tasks(pid_t pid, > - kernel_cap_t *effective, > - kernel_cap_t *inheritable, > - kernel_cap_t *permitted) > -{ > - return -EPERM; > -} > - > -/* > * Atomically modify the effective capabilities returning the original > * value. No permission check is performed here - it is assumed that the > * caller is permitted to set the desired effective capabilities. > @@ -293,6 +278,9 @@ asmlinkage long sys_capset(cap_user_header_t header, const cap_user_data_t data) > if (get_user(pid, &header->pid)) > return -EFAULT; > > + if (pid && (pid != task_pid_vnr(current))) > + return -EPERM; > + > if (copy_from_user(&kdata, data, tocopy > * sizeof(struct __user_cap_data_struct))) { > return -EFAULT; > @@ -310,30 +298,23 @@ asmlinkage long sys_capset(cap_user_header_t header, const cap_user_data_t data) > i++; > } > > - if (pid && (pid != task_pid_vnr(current))) > - ret = do_sys_capset_other_tasks(pid, &effective, &inheritable, > - &permitted); > - else { > - /* > - * This lock is required even when filesystem > - * capability support is configured - it protects the > - * sys_capget() call from returning incorrect data in > - * the case that the targeted process is not the > - * current one. > - */ > - spin_lock(&task_capability_lock); > + /* > + * This lock protects the sys_capget() call from > + * returning incorrect data in the case that the targeted > + * process is not the current one. > + */ > + spin_lock(&task_capability_lock); > > - ret = security_capset_check(current, &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); > - spin_unlock(&task_capability_lock); > - } > + ret = security_capset_check(current, &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); > + spin_unlock(&task_capability_lock); > > > return ret; -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.7 (Darwin) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFI3b1s+bHCR3gb8jsRAkWCAJ4j5Q5NQc2TD8B+WOYJ1JIqV1GdqQCg1qQU +qzZPOvwo/W/73BuA+HvuxQ= =fRje -----END PGP SIGNATURE-----
| |