Messages in this thread | | | Date | Thu, 18 Feb 2016 09:52:20 -0800 | Subject | Re: [PATCH] proc: /proc/<pid>/timerslack_ns permissions fixes | From | Kees Cook <> |
| |
On Wed, Feb 17, 2016 at 9:59 PM, John Stultz <john.stultz@linaro.org> wrote: > This patch adjusts the timerslack_ns file permissions to be > 0666 but requires PTRACE_MODE_ATTACH_FSCREDS to read or write > the value. > > This allows tasks with sufficient privledges (CAP_SYS_PTRACE) > to be able to modify a the timerslack for proccesses owned by > a different user. > > This patch also fixes a return value from EINVAL to EPERM, > and does task locking consistently, given we're handling u64s > on 32bit systems. It also makes use of kstrtoull_from_user > which simplifies some code. > > Cc: Arjan van de Ven <arjan@linux.intel.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Oren Laadan <orenl@cellrox.com> > Cc: Ruchi Kandoi <kandoiruchi@google.com> > Cc: Rom Lemarchand <romlem@android.com> > Cc: Kees Cook <keescook@chromium.org> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Android Kernel Team <kernel-team@android.com> > Signed-off-by: John Stultz <john.stultz@linaro.org>
Acked-by: Kees Cook <keescook@chromium.org>
> --- > This patch applies on top of the previous two patches > which Andrew already added to -mm. It can be folded > down or kept separate as desired.
Probably best to fold them together.
-Kees
> > I've also wired up the Android userspace side to use > this interface, and tested it there, and things seem > to be working properly ( - with some selinux noise, I > still need to figure out the selinux policy changes, > but its working with permissive mode). > > fs/proc/base.c | 28 +++++++++++++--------------- > 1 file changed, 13 insertions(+), 15 deletions(-) > > diff --git a/fs/proc/base.c b/fs/proc/base.c > index d7c51ca..35f583a 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -2262,18 +2262,10 @@ static ssize_t timerslack_ns_write(struct file *file, const char __user *buf, > { > struct inode *inode = file_inode(file); > struct task_struct *p; > - char buffer[PROC_NUMBUF]; > u64 slack_ns; > int err; > > - memset(buffer, 0, sizeof(buffer)); > - if (count > sizeof(buffer) - 1) > - count = sizeof(buffer) - 1; > - > - if (copy_from_user(buffer, buf, count)) > - return -EFAULT; > - > - err = kstrtoull(strstrip(buffer), 10, &slack_ns); > + err = kstrtoull_from_user(buf, count, 10, &slack_ns); > if (err < 0) > return err; > > @@ -2282,12 +2274,14 @@ static ssize_t timerslack_ns_write(struct file *file, const char __user *buf, > return -ESRCH; > > if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS)) { > + task_lock(p); > if (slack_ns == 0) > p->timer_slack_ns = p->default_timer_slack_ns; > else > p->timer_slack_ns = slack_ns; > + task_unlock(p); > } else > - count = -EINVAL; > + count = -EPERM; > > put_task_struct(p); > > @@ -2298,18 +2292,22 @@ static int timerslack_ns_show(struct seq_file *m, void *v) > { > struct inode *inode = m->private; > struct task_struct *p; > + int err = 0; > > p = get_proc_task(inode); > if (!p) > return -ESRCH; > > - task_lock(p); > - seq_printf(m, "%llu\n", p->timer_slack_ns); > - task_unlock(p); > + if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS)) { > + task_lock(p); > + seq_printf(m, "%llu\n", p->timer_slack_ns); > + task_unlock(p); > + } else > + err = -EPERM; > > put_task_struct(p); > > - return 0; > + return err; > } > > static int timerslack_ns_open(struct inode *inode, struct file *filp) > @@ -2899,7 +2897,7 @@ static const struct pid_entry tgid_base_stuff[] = { > #ifdef CONFIG_CHECKPOINT_RESTORE > REG("timers", S_IRUGO, proc_timers_operations), > #endif > - REG("timerslack_ns", S_IRUGO|S_IWUSR, proc_pid_set_timerslack_ns_operations), > + REG("timerslack_ns", S_IRUGO|S_IWUGO, proc_pid_set_timerslack_ns_operations), > }; > > static int proc_tgid_base_readdir(struct file *file, struct dir_context *ctx) > -- > 1.9.1 >
-- Kees Cook Chrome OS & Brillo Security
| |