Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1335976 > unrolled thread

[PATCH 0/2] Extend timer_slack_ns to u64 on 32bit systems & add /proc/<pid>/timerslack_ns

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/2] Extend timer_slack_ns to u64 on 32bit systems & add /proc/<pid>/timerslack_ns John Stultz <john.stultz@linaro.org> - 2016-02-17 02:10 +0100
    [PATCH 2/2] proc: Add /proc/<pid>/timerslack_ns interface John Stultz <john.stultz@linaro.org> - 2016-02-17 02:10 +0100
      Re: [PATCH 2/2] proc: Add /proc/<pid>/timerslack_ns interface Andrew Morton <akpm@linux-foundation.org> - 2016-02-17 20:40 +0100
        Re: [PATCH 2/2] proc: Add /proc/<pid>/timerslack_ns interface Kees Cook <keescook@chromium.org> - 2016-02-17 21:10 +0100
          Re: [PATCH 2/2] proc: Add /proc/<pid>/timerslack_ns interface Andrew Morton <akpm@linux-foundation.org> - 2016-02-17 21:20 +0100
            Re: [PATCH 2/2] proc: Add /proc/<pid>/timerslack_ns interface John Stultz <john.stultz@linaro.org> - 2016-02-17 22:00 +0100
              Re: [PATCH 2/2] proc: Add /proc/<pid>/timerslack_ns interface Andrew Morton <akpm@linux-foundation.org> - 2016-02-17 22:10 +0100
            Re: [PATCH 2/2] proc: Add /proc/<pid>/timerslack_ns interface John Stultz <john.stultz@linaro.org> - 2016-02-17 23:30 +0100
              Re: [PATCH 2/2] proc: Add /proc/<pid>/timerslack_ns interface Andrew Morton <akpm@linux-foundation.org> - 2016-02-17 23:50 +0100
              Re: [PATCH 2/2] proc: Add /proc/<pid>/timerslack_ns interface Kees Cook <keescook@chromium.org> - 2016-02-17 23:50 +0100
                Re: [PATCH 2/2] proc: Add /proc/<pid>/timerslack_ns interface John Stultz <john.stultz@linaro.org> - 2016-02-18 00:00 +0100
        Re: [PATCH 2/2] proc: Add /proc/<pid>/timerslack_ns interface John Stultz <john.stultz@linaro.org> - 2016-02-17 21:50 +0100
      [PATCH] proc: /proc/<pid>/timerslack_ns permissions fixes John Stultz <john.stultz@linaro.org> - 2016-02-18 07:00 +0100
        Re: [PATCH] proc: /proc/<pid>/timerslack_ns permissions fixes Kees Cook <keescook@chromium.org> - 2016-02-18 19:00 +0100

#1335976 — [PATCH 0/2] Extend timer_slack_ns to u64 on 32bit systems & add /proc/<pid>/timerslack_ns

I didn't get any negative feedback on the last round of this,
so I figured I'd send it along w/o the RFC header this time.

This patchset introduces a /proc/<pid>/timerslack_ns interface
which would allow controlling processes to be able to set the
timerslack value on other processes in order to save power by
avoiding wakeups (Something Android currently does via
out-of-tree patches).

The first patch tries to fix the internal timer_slack_ns usage
which was defined as a long, which limits the slack range to
~4 seconds on 32bit systems. It converts it to a u64, which
provides the same basically unlimited slack (500 years) on both
32bit and 64bit machines.

The second patch introduces the /proc/<pid>/timerslack_ns
interface which allows the full 64bit slack range for a task
to be read or set on both 32bit and 64bit machines.

With these two patches, on a 32bit machine, after setting the
slack on bash to 10 seconds:
$ time sleep 1

real    0m10.747s
user    0m0.001s
sys     0m0.005s


The first patch is a little ugly, since I had to chase the slack
delta arguments through a number of functions converting them to
u64s. Let me know if it makes sense to break that up more or not.

Other then that things are fairly straight forward.

Feedback and thoughts would be greatly appreciated!

thanks
-john

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>

John Stultz (2):
  timer: Convert timer_slack_ns from unsigned long to u64
  proc: Add /proc/<pid>/timerslack_ns interface

 Documentation/filesystems/proc.txt | 18 ++++++++++
 fs/eventpoll.c                     |  2 +-
 fs/proc/base.c                     | 69 ++++++++++++++++++++++++++++++++++++++
 fs/select.c                        |  8 ++---
 include/linux/freezer.h            |  2 +-
 include/linux/hrtimer.h            | 12 ++++---
 include/linux/poll.h               |  2 +-
 include/linux/sched.h              |  4 +--
 kernel/sys.c                       |  5 ++-
 kernel/time/hrtimer.c              |  8 ++---
 kernel/time/timer.c                |  4 +--
 11 files changed, 113 insertions(+), 21 deletions(-)

-- 
1.9.1

[toc] | [next] | [standalone]

#1335978 — [PATCH 2/2] proc: Add /proc/<pid>/timerslack_ns interface

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.

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>
---
 Documentation/filesystems/proc.txt | 18 ++++++++++
 fs/proc/base.c                     | 69 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 87 insertions(+)

diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
index 843b045b..7f5607a 100644
--- a/Documentation/filesystems/proc.txt
+++ b/Documentation/filesystems/proc.txt
@@ -43,6 +43,7 @@ Table of Contents
   3.7   /proc/<pid>/task/<tid>/children - Information about task children
   3.8   /proc/<pid>/fdinfo/<fd> - Information about opened file
   3.9   /proc/<pid>/map_files - Information about memory mapped files
+  3.10  /proc/<pid>/timerslack_ns - Task timerslack value
 
   4	Configuring procfs
   4.1	Mount options
@@ -1862,6 +1863,23 @@ time one can open(2) mappings from the listings of two processes and
 comparing their inode numbers to figure out which anonymous memory areas
 are actually shared.
 
+3.10	/proc/<pid>/timerslack_ns - Task timerslack value
+---------------------------------------------------------
+This file provides the value of the task's timerslack value in nanoseconds.
+This value specifies a amount of time that normal timers may be deferred
+in order to coalesce timers and avoid unnecessary wakeups.
+
+This allows a task's interactivity vs power consumption trade off to be
+adjusted.
+
+Writing 0 to the file will set the tasks timerslack to the default value.
+
+Valid values are from 0 - ULLONG_MAX
+
+An application setting the value must have PTRACE_MODE_ATTACH_FSCREDS level
+permissions on the task specified to change its timerslack_ns value.
+
+
 ------------------------------------------------------------------------------
 Configuring procfs
 ------------------------------------------------------------------------------
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 4f764c2..d7c51ca 100644
--- 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;
+
+	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;
+}
+
+static int timerslack_ns_show(struct seq_file *m, void *v)
+{
+	struct inode *inode = m->private;
+	struct task_struct *p;
+
+	p = get_proc_task(inode);
+	if (!p)
+		return -ESRCH;
+
+	task_lock(p);
+	seq_printf(m, "%llu\n", p->timer_slack_ns);
+	task_unlock(p);
+
+	put_task_struct(p);
+
+	return 0;
+}
+
+static int timerslack_ns_open(struct inode *inode, struct file *filp)
+{
+	return single_open(filp, timerslack_ns_show, inode);
+}
+
+static const struct file_operations proc_pid_set_timerslack_ns_operations = {
+	.open		= timerslack_ns_open,
+	.read		= seq_read,
+	.write		= timerslack_ns_write,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
 static int proc_pident_instantiate(struct inode *dir,
 	struct dentry *dentry, struct task_struct *task, const void *ptr)
 {
@@ -2831,6 +2899,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),
 };
 
 static int proc_tgid_base_readdir(struct file *file, struct dir_context *ctx)
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

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

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?

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

> --- 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;
> +}
> +

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

On Wed, 17 Feb 2016 12:09:08 -0800 Kees Cook <keescook@chromium.org> wrote:

> 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 only place where PTRACE_MODE_ATTACH_FSCREDS is used in all of Linux
is /prc/pid/stack.  Makes me curious!

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

Sounds safer.

[toc] | [prev] | [next] | [standalone]

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

On Wed, Feb 17, 2016 at 12:18 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Wed, 17 Feb 2016 12:09:08 -0800 Kees Cook <keescook@chromium.org> wrote:
>
>> 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 only place where PTRACE_MODE_ATTACH_FSCREDS is used in all of Linux
> is /prc/pid/stack.  Makes me curious!

Other uses may be using a combination PTRACE_MODE_ATTACH|PTRACE_MODE_FSCREDS.

>> > The procfs file's permissions are 0644, yes?  So a process's
>> > timer_slack is world-readable?  hm.
>>
>> This should be 600, IMO.
>
> Sounds safer.

Ok. Reworking the patch to use that as well.


Andrew: I saw you added these to -mm already. Would you prefer a fixup
patch ontop, or should I just send out a folded down v3 of the
patchset?

thanks
-john

[toc] | [prev] | [next] | [standalone]

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

On Wed, 17 Feb 2016 12:51:17 -0800 John Stultz <john.stultz@linaro.org> wrote:

> Andrew: I saw you added these to -mm already. Would you prefer a fixup
> patch ontop, or should I just send out a folded down v3 of the
> patchset?

I'm OK with either.  I usually turn replacements into deltas so that
I (and others) can see what changed.  But it can become a bit unwieldy.

[toc] | [prev] | [next] | [standalone]

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

On Wed, Feb 17, 2016 at 12:18 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Wed, 17 Feb 2016 12:09:08 -0800 Kees Cook <keescook@chromium.org> wrote:
>> On Wed, Feb 17, 2016 at 11:35 AM, Andrew Morton
>> > The procfs file's permissions are 0644, yes?  So a process's
>> > timer_slack is world-readable?  hm.
>>
>> This should be 600, IMO.
>
> Sounds safer.

So I've gone ahead and addressed this and the other feedback you had.
But this bit made me realize that I may have missed a key aspect to
the interface that Android needs.

In particular, the whole point here is to allow a controlling task to
modify other tasks' timerslack to limit background tasks' power usage
(and to modify them back to normal when the background tasks become
foreground tasks). Note that on android different tasks run as
different users.

Currently, the controlling process has minimally elevated privileges
(CAP_SYS_NICE). The initial review suggested those privileges should
be higher (PTRACE_MODE_ATTACH), which I've implemented.  However, I'm
realizing that by moving to the proc interface, the filesystem
permissions here put yet another barrier in the way.

While the 600 permissions makes initial sense, it does limit these
controlling tasks with extra privileges (though not root) from
modifying the timerslack, since they cannot open the file to begin
with.

So.... Does world writable (plus the PTRACE_MODE_ATTACH_FSCREDS check)
make more sense here?  Or is there a better way for a system to tweak
the default permissions for procfs entries? (And if so, does that
render the PTRACE_MODE_ATTACH... check unnecessary?).

Apologies. I'm fighting a head-cold, so I'm not feeling particularly sharp here.

thanks
-john

[toc] | [prev] | [next] | [standalone]

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

On Wed, 17 Feb 2016 14:29:29 -0800 John Stultz <john.stultz@linaro.org> wrote:

> On Wed, Feb 17, 2016 at 12:18 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
> > On Wed, 17 Feb 2016 12:09:08 -0800 Kees Cook <keescook@chromium.org> wrote:
> >> On Wed, Feb 17, 2016 at 11:35 AM, Andrew Morton
> >> > The procfs file's permissions are 0644, yes?  So a process's
> >> > timer_slack is world-readable?  hm.
> >>
> >> This should be 600, IMO.
> >
> > Sounds safer.
> 
> So I've gone ahead and addressed this and the other feedback you had.
> But this bit made me realize that I may have missed a key aspect to
> the interface that Android needs.
> 
> In particular, the whole point here is to allow a controlling task to
> modify other tasks' timerslack to limit background tasks' power usage
> (and to modify them back to normal when the background tasks become
> foreground tasks). Note that on android different tasks run as
> different users.
> 
> Currently, the controlling process has minimally elevated privileges
> (CAP_SYS_NICE). The initial review suggested those privileges should
> be higher (PTRACE_MODE_ATTACH), which I've implemented.  However, I'm
> realizing that by moving to the proc interface, the filesystem
> permissions here put yet another barrier in the way.
> 
> While the 600 permissions makes initial sense, it does limit these
> controlling tasks with extra privileges (though not root) from
> modifying the timerslack, since they cannot open the file to begin
> with.
> 
> So.... Does world writable (plus the PTRACE_MODE_ATTACH_FSCREDS check)
> make more sense here?  Or is there a better way for a system to tweak
> the default permissions for procfs entries? (And if so, does that
> render the PTRACE_MODE_ATTACH... check unnecessary?).

I can't immediately think of a problem with it.  Could we check
PTRACE_MODE_ATTACH_FSCREDS in open() to prevent bad guys from reading
our timerslack?

[toc] | [prev] | [next] | [standalone]

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

On Wed, Feb 17, 2016 at 2:29 PM, John Stultz <john.stultz@linaro.org> wrote:
> On Wed, Feb 17, 2016 at 12:18 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
>> On Wed, 17 Feb 2016 12:09:08 -0800 Kees Cook <keescook@chromium.org> wrote:
>>> On Wed, Feb 17, 2016 at 11:35 AM, Andrew Morton
>>> > The procfs file's permissions are 0644, yes?  So a process's
>>> > timer_slack is world-readable?  hm.
>>>
>>> This should be 600, IMO.
>>
>> Sounds safer.
>
> So I've gone ahead and addressed this and the other feedback you had.
> But this bit made me realize that I may have missed a key aspect to
> the interface that Android needs.
>
> In particular, the whole point here is to allow a controlling task to
> modify other tasks' timerslack to limit background tasks' power usage
> (and to modify them back to normal when the background tasks become
> foreground tasks). Note that on android different tasks run as
> different users.
>
> Currently, the controlling process has minimally elevated privileges
> (CAP_SYS_NICE). The initial review suggested those privileges should
> be higher (PTRACE_MODE_ATTACH), which I've implemented.  However, I'm
> realizing that by moving to the proc interface, the filesystem
> permissions here put yet another barrier in the way.
>
> While the 600 permissions makes initial sense, it does limit these
> controlling tasks with extra privileges (though not root) from
> modifying the timerslack, since they cannot open the file to begin
> with.
>
> So.... Does world writable (plus the PTRACE_MODE_ATTACH_FSCREDS check)
> make more sense here?  Or is there a better way for a system to tweak
> the default permissions for procfs entries? (And if so, does that
> render the PTRACE_MODE_ATTACH... check unnecessary?).
>
> Apologies. I'm fighting a head-cold, so I'm not feeling particularly sharp here.

Is timerslack sensitive at all? You could add the ptrace test to the
_show function too, maybe. Then 0666 would solve the open issue
without leaking the timerslack.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

[toc] | [prev] | [next] | [standalone]

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

On Wed, Feb 17, 2016 at 2:45 PM, Kees Cook <keescook@chromium.org> wrote:
> On Wed, Feb 17, 2016 at 2:29 PM, John Stultz <john.stultz@linaro.org> wrote:
>> On Wed, Feb 17, 2016 at 12:18 PM, Andrew Morton
>> <akpm@linux-foundation.org> wrote:
>>> On Wed, 17 Feb 2016 12:09:08 -0800 Kees Cook <keescook@chromium.org> wrote:
>>>> On Wed, Feb 17, 2016 at 11:35 AM, Andrew Morton
>>>> > The procfs file's permissions are 0644, yes?  So a process's
>>>> > timer_slack is world-readable?  hm.
>>>>
>>>> This should be 600, IMO.
>>>
>>> Sounds safer.
>>
>> So I've gone ahead and addressed this and the other feedback you had.
>> But this bit made me realize that I may have missed a key aspect to
>> the interface that Android needs.
>>
>> In particular, the whole point here is to allow a controlling task to
>> modify other tasks' timerslack to limit background tasks' power usage
>> (and to modify them back to normal when the background tasks become
>> foreground tasks). Note that on android different tasks run as
>> different users.
>>
>> Currently, the controlling process has minimally elevated privileges
>> (CAP_SYS_NICE). The initial review suggested those privileges should
>> be higher (PTRACE_MODE_ATTACH), which I've implemented.  However, I'm
>> realizing that by moving to the proc interface, the filesystem
>> permissions here put yet another barrier in the way.
>>
>> While the 600 permissions makes initial sense, it does limit these
>> controlling tasks with extra privileges (though not root) from
>> modifying the timerslack, since they cannot open the file to begin
>> with.
>>
>> So.... Does world writable (plus the PTRACE_MODE_ATTACH_FSCREDS check)
>> make more sense here?  Or is there a better way for a system to tweak
>> the default permissions for procfs entries? (And if so, does that
>> render the PTRACE_MODE_ATTACH... check unnecessary?).
>>
>> Apologies. I'm fighting a head-cold, so I'm not feeling particularly sharp here.
>
> Is timerslack sensitive at all? You could add the ptrace test to the
> _show function too, maybe. Then 0666 would solve the open issue
> without leaking the timerslack.

I don't see how timerslack would be sensitive, but probably many
mistakes start out that way, so not being cavalier about it seems
wise.  :)

Ok. Sounds like you and Andrew are on the same page wrt 666 +
PTRACE_MODE_ATTACH, and that seems like it would be workable.

I'll get that implemented here shortly.

Thanks so much again for the feedback! Really appreciate it!
-john

[toc] | [prev] | [next] | [standalone]

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

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?

Somewhat out of necessity. I found due to recent changes (sha1:
caaee6234d0)  to __ptrace_may_access() one must use FSCREDS or
REALCREDS. So PTRACE_MODE_ATTACH on its own won't work. ("What the
heck" was what I thought too when I noticed my test wasn't working :)

Since we're accessing this via a filesystem interface, I picked
FSCREDS.  But if I chose wrong here, please let me know.

>> +     err = kstrtoull(strstrip(buffer), 10, &slack_ns);
>> +     if (err < 0)
>> +             return err;
>
> Use kstrtoull_from_user()?

Ok. I'll rework it to use this.

thanks!
-john

[toc] | [prev] | [next] | [standalone]

#1337062 — [PATCH] proc: /proc/<pid>/timerslack_ns permissions fixes

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>
---
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.

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

[toc] | [prev] | [next] | [standalone]

#1337591 — Re: [PATCH] proc: /proc/<pid>/timerslack_ns permissions fixes

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web