All of lore.kernel.org
 help / color / mirror / Atom feed
* [RESEND][PATCH] proc: Fix timerslack_ns CAP_SYS_NICE check when adjusting self
@ 2016-08-22 23:01 John Stultz
  2016-08-29 18:28 ` John Stultz
  2016-08-30 22:46 ` Kees Cook
  0 siblings, 2 replies; 7+ messages in thread
From: John Stultz @ 2016-08-22 23:01 UTC (permalink / raw)
  To: lkml
  Cc: John Stultz, Kees Cook, Serge E. Hallyn, Andrew Morton,
	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

In changing from checking ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS)
to capable(CAP_SYS_NICE), I missed that ptrace_my_access succeeds
when p == current, but the CAP_SYS_NICE doesn't.

Thus while the previous commit was intended to loosen the needed
privledges to modify a processes timerslack, it needlessly restricted
a task modifying its own timerslack via the proc/<tid>/timerslack_ns
(which is permitted also via the PR_SET_TIMERSLACK method).

This patch corrects this by checking if p == current before checking
the CAP_SYS_NICE value.

This patch applies on top of my two previous patches currently in -mm

Cc: Kees Cook <keescook@chromium.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
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>
Acked-by: Kees Cook <keescook@chromium.org>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 fs/proc/base.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 02f8389..01c3c2d 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2281,15 +2281,17 @@ static ssize_t timerslack_ns_write(struct file *file, const char __user *buf,
 	if (!p)
 		return -ESRCH;
 
-	if (!capable(CAP_SYS_NICE)) {
-		count = -EPERM;
-		goto out;
-	}
+	if (p != current) {
+		if (!capable(CAP_SYS_NICE)) {
+			count = -EPERM;
+			goto out;
+		}
 
-	err = security_task_setscheduler(p);
-	if (err) {
-		count = err;
-		goto out;
+		err = security_task_setscheduler(p);
+		if (err) {
+			count = err;
+			goto out;
+		}
 	}
 
 	task_lock(p);
@@ -2315,14 +2317,16 @@ static int timerslack_ns_show(struct seq_file *m, void *v)
 	if (!p)
 		return -ESRCH;
 
-	if (!capable(CAP_SYS_NICE)) {
-		err = -EPERM;
-		goto out;
-	}
+	if (p != current) {
 
-	err = security_task_getscheduler(p);
-	if (err)
-		goto out;
+		if (!capable(CAP_SYS_NICE)) {
+			err = -EPERM;
+			goto out;
+		}
+		err = security_task_getscheduler(p);
+		if (err)
+			goto out;
+	}
 
 	task_lock(p);
 	seq_printf(m, "%llu\n", p->timer_slack_ns);
-- 
1.9.1

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

* Re: [RESEND][PATCH] proc: Fix timerslack_ns CAP_SYS_NICE check when adjusting self
  2016-08-22 23:01 [RESEND][PATCH] proc: Fix timerslack_ns CAP_SYS_NICE check when adjusting self John Stultz
@ 2016-08-29 18:28 ` John Stultz
  2016-08-31  2:37   ` Serge E. Hallyn
  2016-08-30 22:46 ` Kees Cook
  1 sibling, 1 reply; 7+ messages in thread
From: John Stultz @ 2016-08-29 18:28 UTC (permalink / raw)
  To: lkml
  Cc: John Stultz, Kees Cook, Serge E. Hallyn, Andrew Morton,
	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

On Mon, Aug 22, 2016 at 4:01 PM, John Stultz <john.stultz@linaro.org> wrote:
> In changing from checking ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS)
> to capable(CAP_SYS_NICE), I missed that ptrace_my_access succeeds
> when p == current, but the CAP_SYS_NICE doesn't.
>
> Thus while the previous commit was intended to loosen the needed
> privledges to modify a processes timerslack, it needlessly restricted
> a task modifying its own timerslack via the proc/<tid>/timerslack_ns
> (which is permitted also via the PR_SET_TIMERSLACK method).
>
> This patch corrects this by checking if p == current before checking
> the CAP_SYS_NICE value.
>
> This patch applies on top of my two previous patches currently in -mm

Ping? Any feedback or comments on this one?

thanks
-john

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

* Re: [RESEND][PATCH] proc: Fix timerslack_ns CAP_SYS_NICE check when adjusting self
  2016-08-22 23:01 [RESEND][PATCH] proc: Fix timerslack_ns CAP_SYS_NICE check when adjusting self John Stultz
  2016-08-29 18:28 ` John Stultz
@ 2016-08-30 22:46 ` Kees Cook
  2016-08-30 23:06   ` Andrew Morton
  1 sibling, 1 reply; 7+ messages in thread
From: Kees Cook @ 2016-08-30 22:46 UTC (permalink / raw)
  To: John Stultz
  Cc: lkml, Serge E. Hallyn, Andrew Morton, 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

On Mon, Aug 22, 2016 at 7:01 PM, John Stultz <john.stultz@linaro.org> wrote:
> In changing from checking ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS)
> to capable(CAP_SYS_NICE), I missed that ptrace_my_access succeeds
> when p == current, but the CAP_SYS_NICE doesn't.
>
> Thus while the previous commit was intended to loosen the needed
> privledges to modify a processes timerslack, it needlessly restricted
> a task modifying its own timerslack via the proc/<tid>/timerslack_ns
> (which is permitted also via the PR_SET_TIMERSLACK method).
>
> This patch corrects this by checking if p == current before checking
> the CAP_SYS_NICE value.
>
> This patch applies on top of my two previous patches currently in -mm
>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> 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>
> Acked-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: John Stultz <john.stultz@linaro.org>

Andrew, can you take this for v4.8?

-Kees

> ---
>  fs/proc/base.c | 34 +++++++++++++++++++---------------
>  1 file changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 02f8389..01c3c2d 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2281,15 +2281,17 @@ static ssize_t timerslack_ns_write(struct file *file, const char __user *buf,
>         if (!p)
>                 return -ESRCH;
>
> -       if (!capable(CAP_SYS_NICE)) {
> -               count = -EPERM;
> -               goto out;
> -       }
> +       if (p != current) {
> +               if (!capable(CAP_SYS_NICE)) {
> +                       count = -EPERM;
> +                       goto out;
> +               }
>
> -       err = security_task_setscheduler(p);
> -       if (err) {
> -               count = err;
> -               goto out;
> +               err = security_task_setscheduler(p);
> +               if (err) {
> +                       count = err;
> +                       goto out;
> +               }
>         }
>
>         task_lock(p);
> @@ -2315,14 +2317,16 @@ static int timerslack_ns_show(struct seq_file *m, void *v)
>         if (!p)
>                 return -ESRCH;
>
> -       if (!capable(CAP_SYS_NICE)) {
> -               err = -EPERM;
> -               goto out;
> -       }
> +       if (p != current) {
>
> -       err = security_task_getscheduler(p);
> -       if (err)
> -               goto out;
> +               if (!capable(CAP_SYS_NICE)) {
> +                       err = -EPERM;
> +                       goto out;
> +               }
> +               err = security_task_getscheduler(p);
> +               if (err)
> +                       goto out;
> +       }
>
>         task_lock(p);
>         seq_printf(m, "%llu\n", p->timer_slack_ns);
> --
> 1.9.1
>



-- 
Kees Cook
Nexus Security

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

* Re: [RESEND][PATCH] proc: Fix timerslack_ns CAP_SYS_NICE check when adjusting self
  2016-08-30 22:46 ` Kees Cook
@ 2016-08-30 23:06   ` Andrew Morton
  2016-08-30 23:12     ` Kees Cook
  2016-08-30 23:36     ` John Stultz
  0 siblings, 2 replies; 7+ messages in thread
From: Andrew Morton @ 2016-08-30 23:06 UTC (permalink / raw)
  To: Kees Cook
  Cc: John Stultz, lkml, 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

On Tue, 30 Aug 2016 18:46:23 -0400 Kees Cook <keescook@chromium.org> wrote:

> On Mon, Aug 22, 2016 at 7:01 PM, John Stultz <john.stultz@linaro.org> wrote:
> > In changing from checking ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS)
> > to capable(CAP_SYS_NICE), I missed that ptrace_my_access succeeds
> > when p == current, but the CAP_SYS_NICE doesn't.
> >
> > Thus while the previous commit was intended to loosen the needed
> > privledges to modify a processes timerslack, it needlessly restricted
> > a task modifying its own timerslack via the proc/<tid>/timerslack_ns
> > (which is permitted also via the PR_SET_TIMERSLACK method).
> >
> > This patch corrects this by checking if p == current before checking
> > the CAP_SYS_NICE value.
> >
> > This patch applies on top of my two previous patches currently in -mm
> >
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: "Serge E. Hallyn" <serge@hallyn.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > 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>
> > Acked-by: Kees Cook <keescook@chromium.org>
> > Signed-off-by: John Stultz <john.stultz@linaro.org>
> 
> Andrew, can you take this for v4.8?

Well, it fixes
proc-relax-proc-tid-timerslack_ns-capability-requirements.patch,
somewhat.  And it textually depends on that.

Do we want all of

proc-relax-proc-tid-timerslack_ns-capability-requirements.patch
proc-add-lsm-hook-checks-to-proc-tid-timerslack_ns.patch
proc-fix-timerslack_ns-cap_sys_nice-check-when-adjusting-self.patch

in 4.8?  If so, why?

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

* Re: [RESEND][PATCH] proc: Fix timerslack_ns CAP_SYS_NICE check when adjusting self
  2016-08-30 23:06   ` Andrew Morton
@ 2016-08-30 23:12     ` Kees Cook
  2016-08-30 23:36     ` John Stultz
  1 sibling, 0 replies; 7+ messages in thread
From: Kees Cook @ 2016-08-30 23:12 UTC (permalink / raw)
  To: Andrew Morton
  Cc: John Stultz, lkml, 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

On Tue, Aug 30, 2016 at 7:06 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Tue, 30 Aug 2016 18:46:23 -0400 Kees Cook <keescook@chromium.org> wrote:
>
>> On Mon, Aug 22, 2016 at 7:01 PM, John Stultz <john.stultz@linaro.org> wrote:
>> > In changing from checking ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS)
>> > to capable(CAP_SYS_NICE), I missed that ptrace_my_access succeeds
>> > when p == current, but the CAP_SYS_NICE doesn't.
>> >
>> > Thus while the previous commit was intended to loosen the needed
>> > privledges to modify a processes timerslack, it needlessly restricted
>> > a task modifying its own timerslack via the proc/<tid>/timerslack_ns
>> > (which is permitted also via the PR_SET_TIMERSLACK method).
>> >
>> > This patch corrects this by checking if p == current before checking
>> > the CAP_SYS_NICE value.
>> >
>> > This patch applies on top of my two previous patches currently in -mm
>> >
>> > Cc: Kees Cook <keescook@chromium.org>
>> > Cc: "Serge E. Hallyn" <serge@hallyn.com>
>> > Cc: Andrew Morton <akpm@linux-foundation.org>
>> > 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>
>> > Acked-by: Kees Cook <keescook@chromium.org>
>> > Signed-off-by: John Stultz <john.stultz@linaro.org>
>>
>> Andrew, can you take this for v4.8?
>
> Well, it fixes
> proc-relax-proc-tid-timerslack_ns-capability-requirements.patch,
> somewhat.  And it textually depends on that.
>
> Do we want all of
>
> proc-relax-proc-tid-timerslack_ns-capability-requirements.patch
> proc-add-lsm-hook-checks-to-proc-tid-timerslack_ns.patch
> proc-fix-timerslack_ns-cap_sys_nice-check-when-adjusting-self.patch
>
> in 4.8?  If so, why?

Oh, my misunderstanding. I thought those already landed in 4.8. If
not, nevermind on the "rush". :) Thanks for picking it up!

-Kees

-- 
Kees Cook
Nexus Security

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

* Re: [RESEND][PATCH] proc: Fix timerslack_ns CAP_SYS_NICE check when adjusting self
  2016-08-30 23:06   ` Andrew Morton
  2016-08-30 23:12     ` Kees Cook
@ 2016-08-30 23:36     ` John Stultz
  1 sibling, 0 replies; 7+ messages in thread
From: John Stultz @ 2016-08-30 23:36 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Kees Cook, lkml, 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

On Tue, Aug 30, 2016 at 4:06 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Tue, 30 Aug 2016 18:46:23 -0400 Kees Cook <keescook@chromium.org> wrote:
>
>> On Mon, Aug 22, 2016 at 7:01 PM, John Stultz <john.stultz@linaro.org> wrote:
>> > In changing from checking ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS)
>> > to capable(CAP_SYS_NICE), I missed that ptrace_my_access succeeds
>> > when p == current, but the CAP_SYS_NICE doesn't.
>> >
>> > Thus while the previous commit was intended to loosen the needed
>> > privledges to modify a processes timerslack, it needlessly restricted
>> > a task modifying its own timerslack via the proc/<tid>/timerslack_ns
>> > (which is permitted also via the PR_SET_TIMERSLACK method).
>> >
>> > This patch corrects this by checking if p == current before checking
>> > the CAP_SYS_NICE value.
>> >
>> > This patch applies on top of my two previous patches currently in -mm
>> >
>> > Cc: Kees Cook <keescook@chromium.org>
>> > Cc: "Serge E. Hallyn" <serge@hallyn.com>
>> > Cc: Andrew Morton <akpm@linux-foundation.org>
>> > 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>
>> > Acked-by: Kees Cook <keescook@chromium.org>
>> > Signed-off-by: John Stultz <john.stultz@linaro.org>
>>
>> Andrew, can you take this for v4.8?
>
> Well, it fixes
> proc-relax-proc-tid-timerslack_ns-capability-requirements.patch,
> somewhat.  And it textually depends on that.
>
> Do we want all of
>
> proc-relax-proc-tid-timerslack_ns-capability-requirements.patch
> proc-add-lsm-hook-checks-to-proc-tid-timerslack_ns.patch
> proc-fix-timerslack_ns-cap_sys_nice-check-when-adjusting-self.patch
>
> in 4.8?  If so, why?

No.. they're fine to wait for the 4.9 merge window. But you picking up
this last fix is appreciated!

thanks
-john

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

* Re: [RESEND][PATCH] proc: Fix timerslack_ns CAP_SYS_NICE check when adjusting self
  2016-08-29 18:28 ` John Stultz
@ 2016-08-31  2:37   ` Serge E. Hallyn
  0 siblings, 0 replies; 7+ messages in thread
From: Serge E. Hallyn @ 2016-08-31  2:37 UTC (permalink / raw)
  To: John Stultz
  Cc: lkml, Kees Cook, Serge E. Hallyn, Andrew Morton, 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

On Mon, Aug 29, 2016 at 11:28:47AM -0700, John Stultz wrote:
> On Mon, Aug 22, 2016 at 4:01 PM, John Stultz <john.stultz@linaro.org> wrote:
> > In changing from checking ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS)
> > to capable(CAP_SYS_NICE), I missed that ptrace_my_access succeeds
> > when p == current, but the CAP_SYS_NICE doesn't.
> >
> > Thus while the previous commit was intended to loosen the needed
> > privledges to modify a processes timerslack, it needlessly restricted
> > a task modifying its own timerslack via the proc/<tid>/timerslack_ns
> > (which is permitted also via the PR_SET_TIMERSLACK method).
> >
> > This patch corrects this by checking if p == current before checking
> > the CAP_SYS_NICE value.
> >
> > This patch applies on top of my two previous patches currently in -mm
> 
> Ping? Any feedback or comments on this one?

(sorry - no objection from me on this patch, thanks)

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

end of thread, other threads:[~2016-08-31  2:38 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-22 23:01 [RESEND][PATCH] proc: Fix timerslack_ns CAP_SYS_NICE check when adjusting self John Stultz
2016-08-29 18:28 ` John Stultz
2016-08-31  2:37   ` Serge E. Hallyn
2016-08-30 22:46 ` Kees Cook
2016-08-30 23:06   ` Andrew Morton
2016-08-30 23:12     ` Kees Cook
2016-08-30 23:36     ` John Stultz

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.