LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v2] proc: use ns_capable instead of capable for timerslack_ns
@ 2018-10-30 18:00 Benjamin Gordon
  2018-10-31  4:31 ` Eric W. Biederman
  0 siblings, 1 reply; 2+ messages in thread
From: Benjamin Gordon @ 2018-10-30 18:00 UTC (permalink / raw)
  To: linux-kernel
  Cc: Benjamin Gordon, John Stultz, Eric W. Biederman, Kees Cook,
	Serge E. Hallyn, Thomas Gleixner, Arjan van de Ven, Oren Laadan,
	Ruchi Kandoi, Rom Lemarchand, Todd Kjos, Colin Cross,
	Nick Kralevich, Dmitry Shmidt, Elliott Hughes,
	Android Kernel Team, Andrew Morton

Access to timerslack_ns is controlled by a process having CAP_SYS_NICE
in its effective capability set, but the current check looks in the root
namespace instead of the process' user namespace.  Since a process is
allowed to do other activities controlled by CAP_SYS_NICE inside a
namespace, it should also be able to adjust timerslack_ns.

Signed-off-by: Benjamin Gordon <bmgordon@google.com>
Cc: John Stultz <john.stultz@linaro.org>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Arjan van de Ven <arjan@linux.intel.com>
Cc: Oren Laadan <orenl@cellrox.com>
Cc: Ruchi Kandoi <kandoiruchi@google.com>
Cc: Rom Lemarchand <romlem@android.com>
Cc: Todd Kjos <tkjos@google.com>
Cc: Colin Cross <ccross@android.com>
Cc: Nick Kralevich <nnk@google.com>
Cc: Dmitry Shmidt <dimitrysh@google.com>
Cc: Elliott Hughes <enh@google.com>
Cc: Android Kernel Team <kernel-team@android.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---

Changes from v1:
  - Use the namespace of the target process instead of the file opener.
    Didn't carry over John Stultz' Acked-by since the changes aren't
    cosmetic.

 fs/proc/base.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index c78d8da09b52c..bdc093ba81dd3 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2385,10 +2385,13 @@ static ssize_t timerslack_ns_write(struct file *file, const char __user *buf,
 		return -ESRCH;
 
 	if (p != current) {
-		if (!capable(CAP_SYS_NICE)) {
+		rcu_read_lock();
+		if (!ns_capable(__task_cred(p)->user_ns, CAP_SYS_NICE)) {
+			rcu_read_unlock();
 			count = -EPERM;
 			goto out;
 		}
+		rcu_read_unlock();
 
 		err = security_task_setscheduler(p);
 		if (err) {
@@ -2421,11 +2424,14 @@ static int timerslack_ns_show(struct seq_file *m, void *v)
 		return -ESRCH;
 
 	if (p != current) {
-
-		if (!capable(CAP_SYS_NICE)) {
+		rcu_read_lock();
+		if (!ns_capable(__task_cred(p)->user_ns, CAP_SYS_NICE)) {
+			rcu_read_unlock();
 			err = -EPERM;
 			goto out;
 		}
+		rcu_read_unlock();
+
 		err = security_task_getscheduler(p);
 		if (err)
 			goto out;
-- 
2.19.1.568.g152ad8e336-goog


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH v2] proc: use ns_capable instead of capable for timerslack_ns
  2018-10-30 18:00 [PATCH v2] proc: use ns_capable instead of capable for timerslack_ns Benjamin Gordon
@ 2018-10-31  4:31 ` Eric W. Biederman
  0 siblings, 0 replies; 2+ messages in thread
From: Eric W. Biederman @ 2018-10-31  4:31 UTC (permalink / raw)
  To: Benjamin Gordon
  Cc: linux-kernel, John Stultz, Kees Cook, Serge E. Hallyn,
	Thomas Gleixner, Arjan van de Ven, Oren Laadan, Ruchi Kandoi,
	Rom Lemarchand, Todd Kjos, Colin Cross, Nick Kralevich,
	Dmitry Shmidt, Elliott Hughes, Android Kernel Team,
	Andrew Morton

Benjamin Gordon <bmgordon@google.com> writes:

> Access to timerslack_ns is controlled by a process having CAP_SYS_NICE
> in its effective capability set, but the current check looks in the root
> namespace instead of the process' user namespace.  Since a process is
> allowed to do other activities controlled by CAP_SYS_NICE inside a
> namespace, it should also be able to adjust timerslack_ns.

Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>

I don't see any fundamental probess with how the processes user
namespace is being accessed.   You can race with setns
and that may result in a descendent user namespace of the current
user namespace being set.  But if you have permissions in the parent
user namespace you will have permissions over a child user namespace.
So the race can't effect the outcome of the ns_capable test.

That and while __task_cred(p) may change it is guaranteed there is a
valid one until __put_task_struct which only happens when a process has
a zero refcount.  Which the success of get_proc_task in before these
checks already ensures is not true.

So from my perspective this looks like a reasonable change.

I don't know how this looks from people who understand the timer bits
and what timerslack does. I suspect it is reasonable as there is no
permission check for changing yourself.

Eric

> Signed-off-by: Benjamin Gordon <bmgordon@google.com>
> Cc: John Stultz <john.stultz@linaro.org>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Arjan van de Ven <arjan@linux.intel.com>
> Cc: Oren Laadan <orenl@cellrox.com>
> Cc: Ruchi Kandoi <kandoiruchi@google.com>
> Cc: Rom Lemarchand <romlem@android.com>
> Cc: Todd Kjos <tkjos@google.com>
> Cc: Colin Cross <ccross@android.com>
> Cc: Nick Kralevich <nnk@google.com>
> Cc: Dmitry Shmidt <dimitrysh@google.com>
> Cc: Elliott Hughes <enh@google.com>
> Cc: Android Kernel Team <kernel-team@android.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> ---
>
> Changes from v1:
>   - Use the namespace of the target process instead of the file opener.
>     Didn't carry over John Stultz' Acked-by since the changes aren't
>     cosmetic.
>
>  fs/proc/base.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index c78d8da09b52c..bdc093ba81dd3 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2385,10 +2385,13 @@ static ssize_t timerslack_ns_write(struct file *file, const char __user *buf,
>  		return -ESRCH;
>  
>  	if (p != current) {
> -		if (!capable(CAP_SYS_NICE)) {
> +		rcu_read_lock();
> +		if (!ns_capable(__task_cred(p)->user_ns, CAP_SYS_NICE)) {
> +			rcu_read_unlock();
>  			count = -EPERM;
>  			goto out;
>  		}
> +		rcu_read_unlock();
>  
>  		err = security_task_setscheduler(p);
>  		if (err) {
> @@ -2421,11 +2424,14 @@ static int timerslack_ns_show(struct seq_file *m, void *v)
>  		return -ESRCH;
>  
>  	if (p != current) {
> -
> -		if (!capable(CAP_SYS_NICE)) {
> +		rcu_read_lock();
> +		if (!ns_capable(__task_cred(p)->user_ns, CAP_SYS_NICE)) {
> +			rcu_read_unlock();
>  			err = -EPERM;
>  			goto out;
>  		}
> +		rcu_read_unlock();
> +
>  		err = security_task_getscheduler(p);
>  		if (err)
>  			goto out;

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2018-10-31  4:32 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-30 18:00 [PATCH v2] proc: use ns_capable instead of capable for timerslack_ns Benjamin Gordon
2018-10-31  4:31 ` Eric W. Biederman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).