Re: [PATCH 2/2] proc: Add /proc/<pid>/timerslack_ns interface

[Kees Cook <keescook@chromium.org>]

On Wed, Feb 17, 2016 at 11:35 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Tue, 16 Feb 2016 17:06:31 -0800 John Stultz <john.stultz@linaro.org> wrote:
>
>> This patch provides a proc/PID/timerslack_ns interface which
>> exposes a task's timerslack value in nanoseconds and allows it
>> to be changed.
>>
>> This allows power/performance management software to set timer
>> slack for other threads according to its policy for the thread
>> (such as when the thread is designated foreground vs. background
>> activity)
>>
>> If the value written is non-zero, slack is set to that value.
>> Otherwise sets it to the default for the thread.
>>
>> This interface checks that the calling task has permissions to
>> to use PTRACE_MODE_ATTACH_FSCREDS on the target task, so that we
>> can ensure arbitrary apps do not change the timer slack for other
>> apps.
>
> hm.  What the heck is PTRACE_MODE_ATTACH_FSCREDS and why was it chosen?

This says the writer needs to have ptrace "attach" level of access,
and that it should be checked with fscreds, as is the standard for
most /proc things like that.

> The procfs file's permissions are 0644, yes?  So a process's
> timer_slack is world-readable?  hm.

This should be 600, IMO.

-Kees

>
>> --- a/fs/proc/base.c
>> +++ b/fs/proc/base.c
>> @@ -2257,6 +2257,74 @@ static const struct file_operations proc_timers_operations = {
>>       .release        = seq_release_private,
>>  };
>>
>> +static ssize_t timerslack_ns_write(struct file *file, const char __user *buf,
>> +                                     size_t count, loff_t *offset)
>> +{
>> +     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);
>> +     if (err < 0)
>> +             return err;
>
> Use kstrtoull_from_user()?
>
>> +     p = get_proc_task(inode);
>> +     if (!p)
>> +             return -ESRCH;
>> +
>> +     if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS)) {
>> +             if (slack_ns == 0)
>> +                     p->timer_slack_ns = p->default_timer_slack_ns;
>> +             else
>> +                     p->timer_slack_ns = slack_ns;
>> +     } else
>> +             count = -EINVAL;
>> +
>> +     put_task_struct(p);
>> +
>> +     return count;
>> +}
>> +
>



-- 
Kees Cook
Chrome OS & Brillo Security