linux-kernel - Re: [PATCH 2/2] proc: Add /proc/<pid>/timerslack

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 17 Feb 2016 12:09:08 -0800
From:	Kees Cook <keescook@...omium.org>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	John Stultz <john.stultz@...aro.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Arjan van de Ven <arjan@...ux.intel.com>,
	lkml <linux-kernel@...r.kernel.org>,
	Oren Laadan <orenl@...lrox.com>,
	Ruchi Kandoi <kandoiruchi@...gle.com>,
	Rom Lemarchand <romlem@...roid.com>,
	Android Kernel Team <kernel-team@...roid.com>
Subject: Re: [PATCH 2/2] proc: Add /proc/<pid>/timerslack_ns interface

On Wed, Feb 17, 2016 at 11:35 AM, Andrew Morton
<akpm@...ux-foundation.org> wrote:
> On Tue, 16 Feb 2016 17:06:31 -0800 John Stultz <john.stultz@...aro.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