LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] proc: use ns_capable instead of capable for timerslack_ns
@ 2018-10-17 22:47 bmgordon
  2018-10-25 17:09 ` Benjamin Gordon
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: bmgordon @ 2018-10-17 22:47 UTC (permalink / raw)
  To: linux-kernel
  Cc: Benjamin Gordon, 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

From: Benjamin Gordon <bmgordon@google.com>

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: 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>
---
 fs/proc/base.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 7e9f07bf260d..4b50937dff80 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2356,7 +2356,7 @@ static ssize_t timerslack_ns_write(struct file *file, const char __user *buf,
 		return -ESRCH;
 
 	if (p != current) {
-		if (!capable(CAP_SYS_NICE)) {
+		if (!ns_capable(file->f_cred->user_ns, CAP_SYS_NICE)) {
 			count = -EPERM;
 			goto out;
 		}
@@ -2393,7 +2393,7 @@ static int timerslack_ns_show(struct seq_file *m, void *v)
 
 	if (p != current) {
 
-		if (!capable(CAP_SYS_NICE)) {
+		if (!ns_capable(seq_user_ns(m), CAP_SYS_NICE)) {
 			err = -EPERM;
 			goto out;
 		}
-- 
2.19.1.331.ge82ca0e54c-goog


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

* Re: [PATCH] proc: use ns_capable instead of capable for timerslack_ns
  2018-10-17 22:47 [PATCH] proc: use ns_capable instead of capable for timerslack_ns bmgordon
@ 2018-10-25 17:09 ` Benjamin Gordon
  2018-10-25 17:20 ` John Stultz
  2018-10-25 18:56 ` Eric W. Biederman
  2 siblings, 0 replies; 5+ messages in thread
From: Benjamin Gordon @ 2018-10-25 17:09 UTC (permalink / raw)
  To: linux-kernel
  Cc: 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

On Wed, Oct 17, 2018 at 04:47:38PM -0600, bmgordon@google.com wrote:
> 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: 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>
> ---
>  fs/proc/base.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 7e9f07bf260d..4b50937dff80 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2356,7 +2356,7 @@ static ssize_t timerslack_ns_write(struct file *file, const char __user *buf,
>  		return -ESRCH;
>  
>  	if (p != current) {
> -		if (!capable(CAP_SYS_NICE)) {
> +		if (!ns_capable(file->f_cred->user_ns, CAP_SYS_NICE)) {
>  			count = -EPERM;
>  			goto out;
>  		}
> @@ -2393,7 +2393,7 @@ static int timerslack_ns_show(struct seq_file *m, void *v)
>  
>  	if (p != current) {
>  
> -		if (!capable(CAP_SYS_NICE)) {
> +		if (!ns_capable(seq_user_ns(m), CAP_SYS_NICE)) {
>  			err = -EPERM;
>  			goto out;
>  		}
> -- 
> 2.19.1.331.ge82ca0e54c-goog
> 

Friendly ping.  Does anybody have time to take a look at this?

Thanks,
Benjamin

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

* Re: [PATCH] proc: use ns_capable instead of capable for timerslack_ns
  2018-10-17 22:47 [PATCH] proc: use ns_capable instead of capable for timerslack_ns bmgordon
  2018-10-25 17:09 ` Benjamin Gordon
@ 2018-10-25 17:20 ` John Stultz
  2018-10-25 18:56 ` Eric W. Biederman
  2 siblings, 0 replies; 5+ messages in thread
From: John Stultz @ 2018-10-25 17:20 UTC (permalink / raw)
  To: bmgordon
  Cc: lkml, 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

On Wed, Oct 17, 2018 at 3:47 PM,  <bmgordon@google.com> wrote:
> From: Benjamin Gordon <bmgordon@google.com>
>
> 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: 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>
> ---
>  fs/proc/base.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 7e9f07bf260d..4b50937dff80 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2356,7 +2356,7 @@ static ssize_t timerslack_ns_write(struct file *file, const char __user *buf,
>                 return -ESRCH;
>
>         if (p != current) {
> -               if (!capable(CAP_SYS_NICE)) {
> +               if (!ns_capable(file->f_cred->user_ns, CAP_SYS_NICE)) {
>                         count = -EPERM;
>                         goto out;
>                 }
> @@ -2393,7 +2393,7 @@ static int timerslack_ns_show(struct seq_file *m, void *v)
>
>         if (p != current) {
>
> -               if (!capable(CAP_SYS_NICE)) {
> +               if (!ns_capable(seq_user_ns(m), CAP_SYS_NICE)) {
>                         err = -EPERM;
>                         goto out;
>                 }

Looks reasonable to me.
Acked-by: John Stultz <john.stultz@linaro.org>

thanks
-john

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

* Re: [PATCH] proc: use ns_capable instead of capable for timerslack_ns
  2018-10-17 22:47 [PATCH] proc: use ns_capable instead of capable for timerslack_ns bmgordon
  2018-10-25 17:09 ` Benjamin Gordon
  2018-10-25 17:20 ` John Stultz
@ 2018-10-25 18:56 ` Eric W. Biederman
  2018-10-30 16:40   ` Benjamin Gordon
  2 siblings, 1 reply; 5+ messages in thread
From: Eric W. Biederman @ 2018-10-25 18:56 UTC (permalink / raw)
  To: bmgordon
  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

bmgordon@google.com writes:

> From: Benjamin Gordon <bmgordon@google.com>
>
> 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.

The goal seems legitimate.  However the permission checks look wrong.

In particular the choice of user namespace should be
"p->cred->user_ns".  This will limit this to tasks that have
CAP_SYS_NICE in the same namespace as the task that is being modified.

Testing file->f_cred->user_ns it is testing whoever opened the file and
that could be anyone.

Eric


> Signed-off-by: Benjamin Gordon <bmgordon@google.com>
> Cc: John Stultz <john.stultz@linaro.org>
> 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>
> ---
>  fs/proc/base.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 7e9f07bf260d..4b50937dff80 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2356,7 +2356,7 @@ static ssize_t timerslack_ns_write(struct file *file, const char __user *buf,
>  		return -ESRCH;
>  
>  	if (p != current) {
> -		if (!capable(CAP_SYS_NICE)) {
> +		if (!ns_capable(file->f_cred->user_ns, CAP_SYS_NICE)) {
>  			count = -EPERM;
>  			goto out;
>  		}
> @@ -2393,7 +2393,7 @@ static int timerslack_ns_show(struct seq_file *m, void *v)
>  
>  	if (p != current) {
>  
> -		if (!capable(CAP_SYS_NICE)) {
> +		if (!ns_capable(seq_user_ns(m), CAP_SYS_NICE)) {
>  			err = -EPERM;
>  			goto out;
>  		}

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

* Re: [PATCH] proc: use ns_capable instead of capable for timerslack_ns
  2018-10-25 18:56 ` Eric W. Biederman
@ 2018-10-30 16:40   ` Benjamin Gordon
  0 siblings, 0 replies; 5+ messages in thread
From: Benjamin Gordon @ 2018-10-30 16:40 UTC (permalink / raw)
  To: Eric W. Biederman
  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

On Thu, Oct 25, 2018 at 01:56:27PM -0500, Eric W. Biederman wrote:
> > 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.
> 
> The goal seems legitimate.  However the permission checks look wrong.
> 
> In particular the choice of user namespace should be
> "p->cred->user_ns".  This will limit this to tasks that have
> CAP_SYS_NICE in the same namespace as the task that is being modified.
> 
> Testing file->f_cred->user_ns it is testing whoever opened the file and
> that could be anyone.

Thanks, that seems like the right answer here.  I'll send a v2 to fix
it.

Thanks,
Benjamin

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

end of thread, other threads:[~2018-10-30 16:40 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-17 22:47 [PATCH] proc: use ns_capable instead of capable for timerslack_ns bmgordon
2018-10-25 17:09 ` Benjamin Gordon
2018-10-25 17:20 ` John Stultz
2018-10-25 18:56 ` Eric W. Biederman
2018-10-30 16:40   ` Benjamin Gordon

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