[patch 0/6] posix-cpu-timers: Fallout fixes and permission tightening

[patch 0/6] posix-cpu-timers: Fallout fixes and permission tightening

[Thomas Gleixner]

Sysbot triggered an issue in the posix timer rework which was trivial to
fix, but after running another test case I discovered that the rework broke
the permission checks subtly. That's also a straightforward fix.

Though when staring at it I discovered that the permission checks for
process clocks and process timers are completely bonkers. The only
requirement is that the target PID is a group leader. Which means that any
process can read the clocks and attach timers to any other process without
priviledge restrictions.

That's just wrong because the clocks and timers can be used to observe
behaviour and both reading the clocks and arming timers adds overhead and
influences runtime performance of the target process.

The last 4 patches deal with that and introduce ptrace based permission
checks and also make the behaviour consistent between thread and process
timers/clocks.

Thanks,

	tglx
---
 include/linux/posix-timers.h   |    9 +---
 kernel/time/posix-cpu-timers.c |   78 ++++++++++++++++++++++++++++++++++-------
 2 files changed, 69 insertions(+), 18 deletions(-)

[patch 1/6] posix-cpu-timers: Always clear head pointer on dequeue

[Thomas Gleixner]

The head pointer in struct cpu_timer is checked to be NULL in
posix_cpu_timer_del() when the delete raced with the exit cleanup. The
works correctly as long as the timer is actually dequeued via
posix_cpu_timers_exit*().

But if the timer was dequeued due to expiry the head pointer is still set
and triggers the warning.

In fact keeping the head pointer around after any dequeue is pointless as
is has no meaning at all after that.

Clear the head pointer always on dequeue and remove the unused requeue
function while at it.

Fixes: 60bda037f1dd ("posix-cpu-timers: Utilize timerqueue for storage")
Reported-by: syzbot+55acd54b57bb4b3840a4@syzkaller.appspotmail.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/posix-timers.h |    9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -74,11 +74,6 @@ struct cpu_timer {
 	int			firing;
 };
 
-static inline bool cpu_timer_requeue(struct cpu_timer *ctmr)
-{
-	return timerqueue_add(ctmr->head, &ctmr->node);
-}
-
 static inline bool cpu_timer_enqueue(struct timerqueue_head *head,
 				     struct cpu_timer *ctmr)
 {
@@ -88,8 +83,10 @@ static inline bool cpu_timer_enqueue(str
 
 static inline void cpu_timer_dequeue(struct cpu_timer *ctmr)
 {
-	if (!RB_EMPTY_NODE(&ctmr->node.node))
+	if (ctmr->head) {
 		timerqueue_del(ctmr->head, &ctmr->node);
+		ctmr->head = NULL;
+	}
 }
 
 static inline u64 cpu_timer_getexpires(struct cpu_timer *ctmr)

[patch 2/6] posix-cpu-timers: Fix permission check regression

[Thomas Gleixner]

The recent consolidation of the three permission checks introduced a subtle
regression. For timer_create() with a process wide timer it returns the
current task if the lookup through the PID which is encoded into the
clockid results in returning current.

That's broken because it does not validate whether the current task is the
group leader.

That was caused by the two different variants of permission checks:

  - posix_cpu_timer_get() allowed access to the process wide clock when the
    looked up task is current. That's not an issue because the process wide
    clock is in the shared sighand.

  - posix_cpu_timer_create() made sure that the looked up task is the group
    leader.

Restore the previous state.

Note, that these permission checks are more than questionable, but that's
subject to follow up changes.

Fixes: 6ae40e3fdcd3 ("posix-cpu-timers: Provide task validation functions")
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/time/posix-cpu-timers.c |   40 +++++++++++++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 9 deletions(-)

--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -47,25 +47,42 @@ void update_rlimit_cpu(struct task_struc
 /*
  * Functions for validating access to tasks.
  */
-static struct task_struct *lookup_task(const pid_t pid, bool thread)
+static struct task_struct *lookup_task(const pid_t pid, bool thread,
+				       bool gettime)
 {
 	struct task_struct *p;
 
+	/*
+	 * If the encoded PID is 0, then the timer is targeted at current
+	 * or the process to which current belongs.
+	 */
 	if (!pid)
 		return thread ? current : current->group_leader;
 
 	p = find_task_by_vpid(pid);
-	if (!p || p == current)
+	if (!p)
 		return p;
+
 	if (thread)
 		return same_thread_group(p, current) ? p : NULL;
-	if (p == current)
-		return p;
+
+	if (gettime) {
+		/*
+		 * For clock_gettime() the task does not need to be the
+		 * actual group leader. tsk->sighand gives access to the
+		 * group's clock.
+		 */
+		return (p == current || thread_group_leader(p)) ? p : NULL;
+	}
+
+	/*
+	 * For processes require that p is group leader.
+	 */
 	return has_group_leader_pid(p) ? p : NULL;
 }
 
 static struct task_struct *__get_task_for_clock(const clockid_t clock,
-						bool getref)
+						bool getref, bool gettime)
 {
 	const bool thread = !!CPUCLOCK_PERTHREAD(clock);
 	const pid_t pid = CPUCLOCK_PID(clock);
@@ -75,7 +92,7 @@ static struct task_struct *__get_task_fo
 		return NULL;
 
 	rcu_read_lock();
-	p = lookup_task(pid, thread);
+	p = lookup_task(pid, thread, gettime);
 	if (p && getref)
 		get_task_struct(p);
 	rcu_read_unlock();
@@ -84,12 +101,17 @@ static struct task_struct *__get_task_fo
 
 static inline struct task_struct *get_task_for_clock(const clockid_t clock)
 {
-	return __get_task_for_clock(clock, true);
+	return __get_task_for_clock(clock, true, false);
+}
+
+static inline struct task_struct *get_task_for_clock_get(const clockid_t clock)
+{
+	return __get_task_for_clock(clock, true, true);
 }
 
 static inline int validate_clock_permissions(const clockid_t clock)
 {
-	return __get_task_for_clock(clock, false) ? 0 : -EINVAL;
+	return __get_task_for_clock(clock, false, false) ? 0 : -EINVAL;
 }
 
 /*
@@ -339,7 +361,7 @@ static int posix_cpu_clock_get(const clo
 	struct task_struct *tsk;
 	u64 t;
 
-	tsk = get_task_for_clock(clock);
+	tsk = get_task_for_clock_get(clock);
 	if (!tsk)
 		return -EINVAL;
 

[patch 3/6] posix-cpu-timers: Restrict timer_create() permissions

[Thomas Gleixner]

Right now there is no restriction at all to attach a Posix CPU timer to any
process in the system. Per thread CPU timers are limited to be created by
threads in the same thread group.

Timers can be used to observe activity of tasks and also impose overhead on
the process to which they are attached because that process needs to do the
fine grained CPU time accounting.

Limit the ability to attach timers to a process by checking whether the
task which is creating the timer has permissions to attach ptrace on the
target process.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/time/posix-cpu-timers.c |   18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -6,6 +6,7 @@
 #include <linux/sched/signal.h>
 #include <linux/sched/cputime.h>
 #include <linux/posix-timers.h>
+#include <linux/ptrace.h>
 #include <linux/errno.h>
 #include <linux/math64.h>
 #include <linux/uaccess.h>
@@ -78,7 +79,22 @@ static struct task_struct *lookup_task(c
 	/*
 	 * For processes require that p is group leader.
 	 */
-	return has_group_leader_pid(p) ? p : NULL;
+	if (!has_group_leader_pid(p))
+		return NULL;
+
+	/*
+	 * Avoid the ptrace overhead when this is current's process
+	 */
+	if (same_thread_group(p, current))
+		return p;
+
+	/*
+	 * Creating timers on processes which cannot be ptraced is not
+	 * permitted:
+	 */
+	if (!ptrace_may_access(p, PTRACE_MODE_ATTACH_REALCREDS))
+		return NULL;
+	return p;
 }
 
 static struct task_struct *__get_task_for_clock(const clockid_t clock,

[patch 4/6] posix-cpu-timers: Restrict clock_gettime() permissions

[Thomas Gleixner]

Similar to creating timers on a process there is no restriction at all to
read the Posix CPU clocks of any process in the system. Per thread CPU
clock access is limited to threads in the same thread group.

The per process CPU clocks can be used to observe activity of tasks and
reading them can affect the execution of the process to which they are
attached as reading can require to lock sighand lock and sum up the fine
grained accounting for all threads in the process.

Restrict it by checking ptrace MODE_READ permissions of the reader on the
target process.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/time/posix-cpu-timers.c |   17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -71,9 +71,18 @@ static struct task_struct *lookup_task(c
 		/*
 		 * For clock_gettime() the task does not need to be the
 		 * actual group leader. tsk->sighand gives access to the
-		 * group's clock.
+		 * group's clock. current can obviously access itself, so
+		 * spare the ptrace check below.
 		 */
-		return (p == current || thread_group_leader(p)) ? p : NULL;
+		if (p == current)
+			return p;
+
+		if (!thread_group_leader(p))
+			return NULL;
+
+		if (!ptrace_may_access(p, PTRACE_MODE_READ_REALCREDS))
+			return NULL;
+		return p;
 	}
 
 	/*
@@ -92,9 +101,7 @@ static struct task_struct *lookup_task(c
 	 * Creating timers on processes which cannot be ptraced is not
 	 * permitted:
 	 */
-	if (!ptrace_may_access(p, PTRACE_MODE_ATTACH_REALCREDS))
-		return NULL;
-	return p;
+	return ptrace_may_access(p, PTRACE_MODE_ATTACH_REALCREDS) ? p : NULL;
 }
 
 static struct task_struct *__get_task_for_clock(const clockid_t clock,

[patch 5/6] posix-cpu-timers: Sanitize thread clock permissions

[Thomas Gleixner]

The thread clock permissions are restricted to tasks of the same thread
group, but that also prevents a ptracer from reading them. This is
inconsistent vs. the process restrictions and unnecessary strict.

Relax it to ptrace permissions in the same way as process permissions are
handled.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/time/posix-cpu-timers.c |   56 +++++++++++++++++++++--------------------
 1 file changed, 29 insertions(+), 27 deletions(-)

--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -51,6 +51,7 @@ void update_rlimit_cpu(struct task_struc
 static struct task_struct *lookup_task(const pid_t pid, bool thread,
 				       bool gettime)
 {
+	unsigned int mode = PTRACE_MODE_ATTACH_REALCREDS;
 	struct task_struct *p;
 
 	/*
@@ -64,44 +65,45 @@ static struct task_struct *lookup_task(c
 	if (!p)
 		return p;
 
-	if (thread)
-		return same_thread_group(p, current) ? p : NULL;
-
 	if (gettime) {
 		/*
 		 * For clock_gettime() the task does not need to be the
 		 * actual group leader. tsk->sighand gives access to the
-		 * group's clock. current can obviously access itself, so
-		 * spare the ptrace check below.
+		 * group's clock.
+		 *
+		 * The trivial case is that p is current or in the same
+		 * thread group, i.e. sharing p->signal. Spare the ptrace
+		 * check in that case.
 		 */
-		if (p == current)
+		if (same_thread_group(p, current))
 			return p;
 
-		if (!thread_group_leader(p))
-			return NULL;
+		mode = PTRACE_MODE_READ_REALCREDS;
 
-		if (!ptrace_may_access(p, PTRACE_MODE_READ_REALCREDS))
-			return NULL;
-		return p;
-	}
+	} else if (thread) {
+		/*
+		 * Timer is going to be attached to a thread. If p is
+		 * current or in the same thread group, granted.
+		 */
+		if (same_thread_group(p, current))
+			return p;
 
-	/*
-	 * For processes require that p is group leader.
-	 */
-	if (!has_group_leader_pid(p))
-		return NULL;
+	} else {
+		/*
+		 * For processes require that p is group leader.
+		 */
+		if (!has_group_leader_pid(p))
+			return NULL;
 
-	/*
-	 * Avoid the ptrace overhead when this is current's process
-	 */
-	if (same_thread_group(p, current))
-		return p;
+		/*
+		 * Avoid the ptrace overhead when this is current's process
+		 */
+		if (same_thread_group(p, current))
+			return p;
+	}
 
-	/*
-	 * Creating timers on processes which cannot be ptraced is not
-	 * permitted:
-	 */
-	return ptrace_may_access(p, PTRACE_MODE_ATTACH_REALCREDS) ? p : NULL;
+	/* Decide based on the ptrace permissions. */
+	return ptrace_may_access(p, mode) ? p : NULL;
 }
 
 static struct task_struct *__get_task_for_clock(const clockid_t clock,

[patch 6/6] posix-cpu-timers: Make PID=0 and PID=self handling consistent

[Thomas Gleixner]

If the PID encoded into the clock id is 0 then the target is either the
calling thread itself or the process to which it belongs.

If the current thread encodes its own PID on a process wide clock then
there is no reason not to treat it in the same way as the PID=0 case.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/time/posix-cpu-timers.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -90,7 +90,14 @@ static struct task_struct *lookup_task(c
 
 	} else {
 		/*
-		 * For processes require that p is group leader.
+		 * Timer is going to be attached to a process. If p is
+		 * current then treat it like the PID=0 case above.
+		 */
+		if (p == current)
+			return current->group_leader;
+
+		/*
+		 * For foreign processes require that p is group leader.
 		 */
 		if (!has_group_leader_pid(p))
 			return NULL;

Re: [patch 5/6] posix-cpu-timers: Sanitize thread clock permissions

[Peter Zijlstra]

On Thu, Sep 05, 2019 at 02:03:44PM +0200, Thomas Gleixner wrote:
> The thread clock permissions are restricted to tasks of the same thread
> group, but that also prevents a ptracer from reading them. This is
> inconsistent vs. the process restrictions and unnecessary strict.
> 
> Relax it to ptrace permissions in the same way as process permissions are
> handled.

More of a meta comment on the added permission checking; so where
clock_getcpuclockid() is allowed to return -EPERM, it doesn't because
that's in glibc and it has no clue.

And these patches implement the ptrace checks and result in -EINVAL for
timer_create() and clock_gettime(), even though it should arguably be
-EPERM, but we're not allowed to return that here.

Re: [patch 5/6] posix-cpu-timers: Sanitize thread clock permissions

[Thomas Gleixner]

On Thu, 5 Sep 2019, Peter Zijlstra wrote:

> On Thu, Sep 05, 2019 at 02:03:44PM +0200, Thomas Gleixner wrote:
> > The thread clock permissions are restricted to tasks of the same thread
> > group, but that also prevents a ptracer from reading them. This is
> > inconsistent vs. the process restrictions and unnecessary strict.
> > 
> > Relax it to ptrace permissions in the same way as process permissions are
> > handled.
> 
> More of a meta comment on the added permission checking; so where
> clock_getcpuclockid() is allowed to return -EPERM, it doesn't because
> that's in glibc and it has no clue.
> 
> And these patches implement the ptrace checks and result in -EINVAL for
> timer_create() and clock_gettime(), even though it should arguably be
> -EPERM, but we're not allowed to return that here.

Yeah. Maybe we should nevertheless.

Thanks,

	tglx

Re: [patch 0/6] posix-cpu-timers: Fallout fixes and permission tightening

[Frederic Weisbecker]

On Thu, Sep 05, 2019 at 02:03:39PM +0200, Thomas Gleixner wrote:
> Sysbot triggered an issue in the posix timer rework which was trivial to
> fix, but after running another test case I discovered that the rework broke
> the permission checks subtly. That's also a straightforward fix.
> 
> Though when staring at it I discovered that the permission checks for
> process clocks and process timers are completely bonkers. The only
> requirement is that the target PID is a group leader. Which means that any
> process can read the clocks and attach timers to any other process without
> priviledge restrictions.
> 
> That's just wrong because the clocks and timers can be used to observe
> behaviour and both reading the clocks and arming timers adds overhead and
> influences runtime performance of the target process.

Yeah I stumbled upon that by the past and found out the explanation behind
in old history: https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/kernel/posix-cpu-timers.c?id=a78331f2168ef1e67b53a0f8218c70a19f0b2a4c

"This makes no constraint on who can see whose per-process clocks.  This
information is already available for the VIRT and PROF (i.e.  utime and stime)
information via /proc.  I am open to suggestions on if/how security
constraints on who can see whose clocks should be imposed."

I'm all for mitigating that, let's just hope that won't break some ABIs.

Re: [patch 0/6] posix-cpu-timers: Fallout fixes and permission tightening

[Thomas Gleixner]

On Thu, 5 Sep 2019, Frederic Weisbecker wrote:
> On Thu, Sep 05, 2019 at 02:03:39PM +0200, Thomas Gleixner wrote:
> > Sysbot triggered an issue in the posix timer rework which was trivial to
> > fix, but after running another test case I discovered that the rework broke
> > the permission checks subtly. That's also a straightforward fix.
> > 
> > Though when staring at it I discovered that the permission checks for
> > process clocks and process timers are completely bonkers. The only
> > requirement is that the target PID is a group leader. Which means that any
> > process can read the clocks and attach timers to any other process without
> > priviledge restrictions.
> > 
> > That's just wrong because the clocks and timers can be used to observe
> > behaviour and both reading the clocks and arming timers adds overhead and
> > influences runtime performance of the target process.
> 
> Yeah I stumbled upon that by the past and found out the explanation behind
> in old history: https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/kernel/posix-cpu-timers.c?id=a78331f2168ef1e67b53a0f8218c70a19f0b2a4c
> 
> "This makes no constraint on who can see whose per-process clocks.  This
> information is already available for the VIRT and PROF (i.e.  utime and stime)
> information via /proc.  I am open to suggestions on if/how security
> constraints on who can see whose clocks should be imposed."
> 
> I'm all for mitigating that, let's just hope that won't break some ABIs.

Well, reading clocks is one part of the issue. Arming timers on any process
is a different story.

Also /proc/$PID access can be restricted nowadays. So that posic clock
stuff should at least have exactly the same restrictions.

Thanks,

	tglx

Re: [patch 0/6] posix-cpu-timers: Fallout fixes and permission tightening

[Frederic Weisbecker]

On Thu, Sep 05, 2019 at 04:57:10PM +0200, Thomas Gleixner wrote:
> On Thu, 5 Sep 2019, Frederic Weisbecker wrote:
> > On Thu, Sep 05, 2019 at 02:03:39PM +0200, Thomas Gleixner wrote:
> > > Sysbot triggered an issue in the posix timer rework which was trivial to
> > > fix, but after running another test case I discovered that the rework broke
> > > the permission checks subtly. That's also a straightforward fix.
> > > 
> > > Though when staring at it I discovered that the permission checks for
> > > process clocks and process timers are completely bonkers. The only
> > > requirement is that the target PID is a group leader. Which means that any
> > > process can read the clocks and attach timers to any other process without
> > > priviledge restrictions.
> > > 
> > > That's just wrong because the clocks and timers can be used to observe
> > > behaviour and both reading the clocks and arming timers adds overhead and
> > > influences runtime performance of the target process.
> > 
> > Yeah I stumbled upon that by the past and found out the explanation behind
> > in old history: https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/kernel/posix-cpu-timers.c?id=a78331f2168ef1e67b53a0f8218c70a19f0b2a4c
> > 
> > "This makes no constraint on who can see whose per-process clocks.  This
> > information is already available for the VIRT and PROF (i.e.  utime and stime)
> > information via /proc.  I am open to suggestions on if/how security
> > constraints on who can see whose clocks should be imposed."
> > 
> > I'm all for mitigating that, let's just hope that won't break some ABIs.
> 
> Well, reading clocks is one part of the issue. Arming timers on any process
> is a different story.

Exactly!

> 
> Also /proc/$PID access can be restricted nowadays. So that posic clock
> stuff should at least have exactly the same restrictions.

Yeah definetly.

> 
> Thanks,
> 
> 	tglx
> 

Re: [patch 1/6] posix-cpu-timers: Always clear head pointer on dequeue

[Frederic Weisbecker]

On Thu, Sep 05, 2019 at 02:03:40PM +0200, Thomas Gleixner wrote:
> The head pointer in struct cpu_timer is checked to be NULL in
> posix_cpu_timer_del() when the delete raced with the exit cleanup. The
> works correctly as long as the timer is actually dequeued via
> posix_cpu_timers_exit*().
> 
> But if the timer was dequeued due to expiry the head pointer is still set
> and triggers the warning.
> 
> In fact keeping the head pointer around after any dequeue is pointless as
> is has no meaning at all after that.
> 
> Clear the head pointer always on dequeue and remove the unused requeue
> function while at it.
> 
> Fixes: 60bda037f1dd ("posix-cpu-timers: Utilize timerqueue for storage")
> Reported-by: syzbot+55acd54b57bb4b3840a4@syzkaller.appspotmail.com
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Reviewed-by: Frederic Weisbecker <frederic@kernel.org>

Re: [patch 2/6] posix-cpu-timers: Fix permission check regression

[Frederic Weisbecker]

On Thu, Sep 05, 2019 at 02:03:41PM +0200, Thomas Gleixner wrote:
> The recent consolidation of the three permission checks introduced a subtle
> regression. For timer_create() with a process wide timer it returns the
> current task if the lookup through the PID which is encoded into the
> clockid results in returning current.
> 
> That's broken because it does not validate whether the current task is the
> group leader.
> 
> That was caused by the two different variants of permission checks:
> 
>   - posix_cpu_timer_get() allowed access to the process wide clock when the
>     looked up task is current. That's not an issue because the process wide
>     clock is in the shared sighand.
> 
>   - posix_cpu_timer_create() made sure that the looked up task is the group
>     leader.
> 
> Restore the previous state.
> 
> Note, that these permission checks are more than questionable, but that's
> subject to follow up changes.
> 
> Fixes: 6ae40e3fdcd3 ("posix-cpu-timers: Provide task validation functions")
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  kernel/time/posix-cpu-timers.c |   40 +++++++++++++++++++++++++++++++---------
>  1 file changed, 31 insertions(+), 9 deletions(-)
> 
> --- a/kernel/time/posix-cpu-timers.c
> +++ b/kernel/time/posix-cpu-timers.c
> @@ -47,25 +47,42 @@ void update_rlimit_cpu(struct task_struc
>  /*
>   * Functions for validating access to tasks.
>   */
> -static struct task_struct *lookup_task(const pid_t pid, bool thread)
> +static struct task_struct *lookup_task(const pid_t pid, bool thread,
> +				       bool gettime)
>  {
>  	struct task_struct *p;
>  
> +	/*
> +	 * If the encoded PID is 0, then the timer is targeted at current
> +	 * or the process to which current belongs.
> +	 */
>  	if (!pid)
>  		return thread ? current : current->group_leader;
>  
>  	p = find_task_by_vpid(pid);
> -	if (!p || p == current)
> +	if (!p)
>  		return p;
> +
>  	if (thread)
>  		return same_thread_group(p, current) ? p : NULL;
> -	if (p == current)
> -		return p;
> +
> +	if (gettime) {
> +		/*
> +		 * For clock_gettime() the task does not need to be the
> +		 * actual group leader. tsk->sighand gives access to the
> +		 * group's clock.
> +		 */

I'm a bit confused with the explanation. Why is it fine to do so with clock
and not with timer? tsk->sighand gives access to the group's timer as
well.

> +		return (p == current || thread_group_leader(p)) ? p : NULL;
> +	}
> +
> +	/*
> +	 * For processes require that p is group leader.
> +	 */
>  	return has_group_leader_pid(p) ? p : NULL;
>  }

Re: [patch 2/6] posix-cpu-timers: Fix permission check regression

[Thomas Gleixner]

On Thu, 5 Sep 2019, Frederic Weisbecker wrote:
> On Thu, Sep 05, 2019 at 02:03:41PM +0200, Thomas Gleixner wrote:
> > +	if (gettime) {
> > +		/*
> > +		 * For clock_gettime() the task does not need to be the
> > +		 * actual group leader. tsk->sighand gives access to the
> > +		 * group's clock.
> > +		 */
> 
> I'm a bit confused with the explanation. Why is it fine to do so with clock
> and not with timer? tsk->sighand gives access to the group's timer as
> well.

Timer stores the target task and that's the group leader for process wide
while clock read is just momentary. Lemme rephrase that.

Thanks,

	tglx

Re: [patch 0/6] posix-cpu-timers: Fallout fixes and permission tightening

[Kees Cook]

On Thu, Sep 05, 2019 at 02:03:39PM +0200, Thomas Gleixner wrote:
> Sysbot triggered an issue in the posix timer rework which was trivial to
> fix, but after running another test case I discovered that the rework broke
> the permission checks subtly. That's also a straightforward fix.
> 
> Though when staring at it I discovered that the permission checks for
> process clocks and process timers are completely bonkers. The only
> requirement is that the target PID is a group leader. Which means that any
> process can read the clocks and attach timers to any other process without
> priviledge restrictions.
> 
> That's just wrong because the clocks and timers can be used to observe
> behaviour and both reading the clocks and arming timers adds overhead and
> influences runtime performance of the target process.
> 
> The last 4 patches deal with that and introduce ptrace based permission
> checks and also make the behaviour consistent between thread and process
> timers/clocks.

I like these changes! Thanks for working on it. :)

Since this is a subtle bit of checking and there are concerns about ABI
breaks, can you also write some selftests for this area just to nail
down what should work and what should be blocked, etc?

-- 
Kees Cook

Re: [patch 0/6] posix-cpu-timers: Fallout fixes and permission tightening

[Thomas Gleixner]

On Thu, 5 Sep 2019, Kees Cook wrote:
> On Thu, Sep 05, 2019 at 02:03:39PM +0200, Thomas Gleixner wrote:
> > Sysbot triggered an issue in the posix timer rework which was trivial to
> > fix, but after running another test case I discovered that the rework broke
> > the permission checks subtly. That's also a straightforward fix.
> > 
> > Though when staring at it I discovered that the permission checks for
> > process clocks and process timers are completely bonkers. The only
> > requirement is that the target PID is a group leader. Which means that any
> > process can read the clocks and attach timers to any other process without
> > priviledge restrictions.
> > 
> > That's just wrong because the clocks and timers can be used to observe
> > behaviour and both reading the clocks and arming timers adds overhead and
> > influences runtime performance of the target process.
> > 
> > The last 4 patches deal with that and introduce ptrace based permission
> > checks and also make the behaviour consistent between thread and process
> > timers/clocks.
> 
> I like these changes! Thanks for working on it. :)
> 
> Since this is a subtle bit of checking and there are concerns about ABI
> breaks, can you also write some selftests for this area just to nail
> down what should work and what should be blocked, etc?

Sigh, yes. /me adds it to that append only todo list thingy :)

[patch V2 2/6] posix-cpu-timers: Fix permission check regression

[Thomas Gleixner]

The recent consolidation of the three permission checks introduced a subtle
regression. For timer_create() with a process wide timer it returns the
current task if the lookup through the PID which is encoded into the
clockid results in returning current.

That's broken because it does not validate whether the current task is the
group leader.

That was caused by the two different variants of permission checks:

  - posix_cpu_timer_get() allowed access to the process wide clock when the
    looked up task is current. That's not an issue because the process wide
    clock is in the shared sighand.

  - posix_cpu_timer_create() made sure that the looked up task is the group
    leader.

Restore the previous state.

Note, that these permission checks are more than questionable, but that's
subject to follow up changes.

Fixes: 6ae40e3fdcd3 ("posix-cpu-timers: Provide task validation functions")
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
V2: Updated comment. Note the following patches need updates as well
    to fixup the trivial conflicts...
---
 kernel/time/posix-cpu-timers.c |   44 ++++++++++++++++++++++++++++++++---------
 1 file changed, 35 insertions(+), 9 deletions(-)

--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -47,25 +47,46 @@ void update_rlimit_cpu(struct task_struc
 /*
  * Functions for validating access to tasks.
  */
-static struct task_struct *lookup_task(const pid_t pid, bool thread)
+static struct task_struct *lookup_task(const pid_t pid, bool thread,
+				       bool gettime)
 {
 	struct task_struct *p;
 
+	/*
+	 * If the encoded PID is 0, then the timer is targeted at current
+	 * or the process to which current belongs.
+	 */
 	if (!pid)
 		return thread ? current : current->group_leader;
 
 	p = find_task_by_vpid(pid);
-	if (!p || p == current)
+	if (!p)
 		return p;
+
 	if (thread)
 		return same_thread_group(p, current) ? p : NULL;
-	if (p == current)
-		return p;
+
+	if (gettime) {
+		/*
+		 * For clock_gettime(PROCESS) the task does not need to be
+		 * the actual group leader. tsk->sighand gives
+		 * access to the group's clock.
+		 *
+		 * Timers need the group leader because they take a
+		 * reference on it and store the task pointer until the
+		 * timer is destroyed.
+		 */
+		return (p == current || thread_group_leader(p)) ? p : NULL;
+	}
+
+	/*
+	 * For processes require that p is group leader.
+	 */
 	return has_group_leader_pid(p) ? p : NULL;
 }
 
 static struct task_struct *__get_task_for_clock(const clockid_t clock,
-						bool getref)
+						bool getref, bool gettime)
 {
 	const bool thread = !!CPUCLOCK_PERTHREAD(clock);
 	const pid_t pid = CPUCLOCK_PID(clock);
@@ -75,7 +96,7 @@ static struct task_struct *__get_task_fo
 		return NULL;
 
 	rcu_read_lock();
-	p = lookup_task(pid, thread);
+	p = lookup_task(pid, thread, gettime);
 	if (p && getref)
 		get_task_struct(p);
 	rcu_read_unlock();
@@ -84,12 +105,17 @@ static struct task_struct *__get_task_fo
 
 static inline struct task_struct *get_task_for_clock(const clockid_t clock)
 {
-	return __get_task_for_clock(clock, true);
+	return __get_task_for_clock(clock, true, false);
+}
+
+static inline struct task_struct *get_task_for_clock_get(const clockid_t clock)
+{
+	return __get_task_for_clock(clock, true, true);
 }
 
 static inline int validate_clock_permissions(const clockid_t clock)
 {
-	return __get_task_for_clock(clock, false) ? 0 : -EINVAL;
+	return __get_task_for_clock(clock, false, false) ? 0 : -EINVAL;
 }
 
 /*
@@ -339,7 +365,7 @@ static int posix_cpu_clock_get(const clo
 	struct task_struct *tsk;
 	u64 t;
 
-	tsk = get_task_for_clock(clock);
+	tsk = get_task_for_clock_get(clock);
 	if (!tsk)
 		return -EINVAL;
 

Re: [patch V2 2/6] posix-cpu-timers: Fix permission check regression

[Frederic Weisbecker]

On Thu, Sep 05, 2019 at 11:15:08PM +0200, Thomas Gleixner wrote:
> +	if (gettime) {
> +		/*
> +		 * For clock_gettime(PROCESS) the task does not need to be
> +		 * the actual group leader. tsk->sighand gives
> +		 * access to the group's clock.
> +		 *
> +		 * Timers need the group leader because they take a
> +		 * reference on it and store the task pointer until the
> +		 * timer is destroyed.

Well, that would work with a non group leader as well but anyway that wouldn't
be pretty.

Reviewed-by: Frederic Weisbecker <frederic@kernel.org>

[tip: timers/core] posix-cpu-timers: Fix permission check regression

[tip-bot2 for Thomas Gleixner]

The following commit has been merged into the timers/core branch of tip:

Commit-ID:     77b4b5420422fc037d00b8f3f0e89b2262e4ae29
Gitweb:        https://git.kernel.org/tip/77b4b5420422fc037d00b8f3f0e89b2262e4ae29
Author:        Thomas Gleixner <tglx@linutronix.de>
AuthorDate:    Thu, 05 Sep 2019 23:15:08 +02:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 10 Sep 2019 12:13:07 +01:00

posix-cpu-timers: Fix permission check regression

The recent consolidation of the three permission checks introduced a subtle
regression. For timer_create() with a process wide timer it returns the
current task if the lookup through the PID which is encoded into the
clockid results in returning current.

That's broken because it does not validate whether the current task is the
group leader.

That was caused by the two different variants of permission checks:

  - posix_cpu_timer_get() allowed access to the process wide clock when the
    looked up task is current. That's not an issue because the process wide
    clock is in the shared sighand.

  - posix_cpu_timer_create() made sure that the looked up task is the group
    leader.

Restore the previous state.

Note, that these permission checks are more than questionable, but that's
subject to follow up changes.

Fixes: 6ae40e3fdcd3 ("posix-cpu-timers: Provide task validation functions")
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
Link: https://lkml.kernel.org/r/alpine.DEB.2.21.1909052314110.1902@nanos.tec.linutronix.de

---
 kernel/time/posix-cpu-timers.c | 44 ++++++++++++++++++++++++++-------
 1 file changed, 35 insertions(+), 9 deletions(-)

diff --git a/kernel/time/posix-cpu-timers.c b/kernel/time/posix-cpu-timers.c
index c3a95b1..92a4319 100644
--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -47,25 +47,46 @@ void update_rlimit_cpu(struct task_struct *task, unsigned long rlim_new)
 /*
  * Functions for validating access to tasks.
  */
-static struct task_struct *lookup_task(const pid_t pid, bool thread)
+static struct task_struct *lookup_task(const pid_t pid, bool thread,
+				       bool gettime)
 {
 	struct task_struct *p;
 
+	/*
+	 * If the encoded PID is 0, then the timer is targeted at current
+	 * or the process to which current belongs.
+	 */
 	if (!pid)
 		return thread ? current : current->group_leader;
 
 	p = find_task_by_vpid(pid);
-	if (!p || p == current)
+	if (!p)
 		return p;
+
 	if (thread)
 		return same_thread_group(p, current) ? p : NULL;
-	if (p == current)
-		return p;
+
+	if (gettime) {
+		/*
+		 * For clock_gettime(PROCESS) the task does not need to be
+		 * the actual group leader. tsk->sighand gives
+		 * access to the group's clock.
+		 *
+		 * Timers need the group leader because they take a
+		 * reference on it and store the task pointer until the
+		 * timer is destroyed.
+		 */
+		return (p == current || thread_group_leader(p)) ? p : NULL;
+	}
+
+	/*
+	 * For processes require that p is group leader.
+	 */
 	return has_group_leader_pid(p) ? p : NULL;
 }
 
 static struct task_struct *__get_task_for_clock(const clockid_t clock,
-						bool getref)
+						bool getref, bool gettime)
 {
 	const bool thread = !!CPUCLOCK_PERTHREAD(clock);
 	const pid_t pid = CPUCLOCK_PID(clock);
@@ -75,7 +96,7 @@ static struct task_struct *__get_task_for_clock(const clockid_t clock,
 		return NULL;
 
 	rcu_read_lock();
-	p = lookup_task(pid, thread);
+	p = lookup_task(pid, thread, gettime);
 	if (p && getref)
 		get_task_struct(p);
 	rcu_read_unlock();
@@ -84,12 +105,17 @@ static struct task_struct *__get_task_for_clock(const clockid_t clock,
 
 static inline struct task_struct *get_task_for_clock(const clockid_t clock)
 {
-	return __get_task_for_clock(clock, true);
+	return __get_task_for_clock(clock, true, false);
+}
+
+static inline struct task_struct *get_task_for_clock_get(const clockid_t clock)
+{
+	return __get_task_for_clock(clock, true, true);
 }
 
 static inline int validate_clock_permissions(const clockid_t clock)
 {
-	return __get_task_for_clock(clock, false) ? 0 : -EINVAL;
+	return __get_task_for_clock(clock, false, false) ? 0 : -EINVAL;
 }
 
 /*
@@ -339,7 +365,7 @@ static int posix_cpu_clock_get(const clockid_t clock, struct timespec64 *tp)
 	struct task_struct *tsk;
 	u64 t;
 
-	tsk = get_task_for_clock(clock);
+	tsk = get_task_for_clock_get(clock);
 	if (!tsk)
 		return -EINVAL;
 

Re: [patch 3/6] posix-cpu-timers: Restrict timer_create() permissions

[Frederic Weisbecker]

On Thu, Sep 05, 2019 at 02:03:42PM +0200, Thomas Gleixner wrote:
> Right now there is no restriction at all to attach a Posix CPU timer to any
> process in the system. Per thread CPU timers are limited to be created by
> threads in the same thread group.
> 
> Timers can be used to observe activity of tasks and also impose overhead on
> the process to which they are attached because that process needs to do the
> fine grained CPU time accounting.
> 
> Limit the ability to attach timers to a process by checking whether the
> task which is creating the timer has permissions to attach ptrace on the
> target process.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Makes sense. I hope no serious user currently rely on that lack of
restriction. Let's just apply and wait for complains if any.

Reviewed-by: Frederic Weisbecker <frederic@kernel.org>

Re: [patch 4/6] posix-cpu-timers: Restrict clock_gettime() permissions

[Frederic Weisbecker]

On Thu, Sep 05, 2019 at 02:03:43PM +0200, Thomas Gleixner wrote:
> Similar to creating timers on a process there is no restriction at all to
> read the Posix CPU clocks of any process in the system. Per thread CPU
> clock access is limited to threads in the same thread group.
> 
> The per process CPU clocks can be used to observe activity of tasks and
> reading them can affect the execution of the process to which they are
> attached as reading can require to lock sighand lock and sum up the fine
> grained accounting for all threads in the process.
> 
> Restrict it by checking ptrace MODE_READ permissions of the reader on the
> target process.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Reviewed-by: Frederic Weisbecker <frederic@kernel.org>

Re: [patch 5/6] posix-cpu-timers: Sanitize thread clock permissions

[Frederic Weisbecker]

On Thu, Sep 05, 2019 at 02:03:44PM +0200, Thomas Gleixner wrote:
> The thread clock permissions are restricted to tasks of the same thread
> group, but that also prevents a ptracer from reading them. This is
> inconsistent vs. the process restrictions and unnecessary strict.
> 
> Relax it to ptrace permissions in the same way as process permissions are
> handled.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  kernel/time/posix-cpu-timers.c |   56 +++++++++++++++++++++--------------------
>  1 file changed, 29 insertions(+), 27 deletions(-)
> 
> --- a/kernel/time/posix-cpu-timers.c
> +++ b/kernel/time/posix-cpu-timers.c
> @@ -51,6 +51,7 @@ void update_rlimit_cpu(struct task_struc
>  static struct task_struct *lookup_task(const pid_t pid, bool thread,
>  				       bool gettime)
>  {
> +	unsigned int mode = PTRACE_MODE_ATTACH_REALCREDS;
>  	struct task_struct *p;
>  
>  	/*
> @@ -64,44 +65,45 @@ static struct task_struct *lookup_task(c
>  	if (!p)
>  		return p;
>  
> -	if (thread)
> -		return same_thread_group(p, current) ? p : NULL;
> -
>  	if (gettime) {
>  		/*
>  		 * For clock_gettime() the task does not need to be the
>  		 * actual group leader. tsk->sighand gives access to the
> -		 * group's clock. current can obviously access itself, so
> -		 * spare the ptrace check below.
> +		 * group's clock.
> +		 *
> +		 * The trivial case is that p is current or in the same
> +		 * thread group, i.e. sharing p->signal. Spare the ptrace
> +		 * check in that case.
>  		 */
> -		if (p == current)
> +		if (same_thread_group(p, current))
>  			return p;
>  
> -		if (!thread_group_leader(p))
> -			return NULL;
> +		mode = PTRACE_MODE_READ_REALCREDS;
>  
> -		if (!ptrace_may_access(p, PTRACE_MODE_READ_REALCREDS))
> -			return NULL;
> -		return p;
> -	}
> +	} else if (thread) {
> +		/*
> +		 * Timer is going to be attached to a thread. If p is
> +		 * current or in the same thread group, granted.
> +		 */
> +		if (same_thread_group(p, current))
> +			return p;
>  
> -	/*
> -	 * For processes require that p is group leader.
> -	 */
> -	if (!has_group_leader_pid(p))
> -		return NULL;
> +	} else {
> +		/*
> +		 * For processes require that p is group leader.
> +		 */
> +		if (!has_group_leader_pid(p))
> +			return NULL;
>  
> -	/*
> -	 * Avoid the ptrace overhead when this is current's process
> -	 */
> -	if (same_thread_group(p, current))
> -		return p;
> +		/*
> +		 * Avoid the ptrace overhead when this is current's process
> +		 */
> +		if (same_thread_group(p, current))

Should it be "if (p == current)" ?

Other than that:

Reviewed-by: Frederic Weisbecker <frederic@kernel.org>


> +			return p;
> +	}
>  
> -	/*
> -	 * Creating timers on processes which cannot be ptraced is not
> -	 * permitted:
> -	 */
> -	return ptrace_may_access(p, PTRACE_MODE_ATTACH_REALCREDS) ? p : NULL;
> +	/* Decide based on the ptrace permissions. */
> +	return ptrace_may_access(p, mode) ? p : NULL;
>  }

Re: [patch 6/6] posix-cpu-timers: Make PID=0 and PID=self handling consistent

[Frederic Weisbecker]

On Thu, Sep 05, 2019 at 02:03:45PM +0200, Thomas Gleixner wrote:
> If the PID encoded into the clock id is 0 then the target is either the
> calling thread itself or the process to which it belongs.
> 
> If the current thread encodes its own PID on a process wide clock then
> there is no reason not to treat it in the same way as the PID=0 case.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  kernel/time/posix-cpu-timers.c |    9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> --- a/kernel/time/posix-cpu-timers.c
> +++ b/kernel/time/posix-cpu-timers.c
> @@ -90,7 +90,14 @@ static struct task_struct *lookup_task(c
>  
>  	} else {
>  		/*
> -		 * For processes require that p is group leader.
> +		 * Timer is going to be attached to a process. If p is
> +		 * current then treat it like the PID=0 case above.
> +		 */
> +		if (p == current)
> +			return current->group_leader;
> +
> +		/*
> +		 * For foreign processes require that p is group leader.
>  		 */
>  		if (!has_group_leader_pid(p))
>  			return NULL;

So, right after you should have that:

                if (same_thread_group(p, current))
                        return p;

Which I suggested to convert as:

                if (p == current)
                        return p;

Either way, you can now remove those lines.

And then:

Reviewed-by: Frederic Weisbecker <frederic@kernel.org>

Re: [patch 6/6] posix-cpu-timers: Make PID=0 and PID=self handling consistent

[Thomas Gleixner]

On Mon, 23 Sep 2019, Frederic Weisbecker wrote:

> On Thu, Sep 05, 2019 at 02:03:45PM +0200, Thomas Gleixner wrote:
> > If the PID encoded into the clock id is 0 then the target is either the
> > calling thread itself or the process to which it belongs.
> > 
> > If the current thread encodes its own PID on a process wide clock then
> > there is no reason not to treat it in the same way as the PID=0 case.
> > 
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > ---
> >  kernel/time/posix-cpu-timers.c |    9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > --- a/kernel/time/posix-cpu-timers.c
> > +++ b/kernel/time/posix-cpu-timers.c
> > @@ -90,7 +90,14 @@ static struct task_struct *lookup_task(c
> >  
> >  	} else {
> >  		/*
> > -		 * For processes require that p is group leader.
> > +		 * Timer is going to be attached to a process. If p is
> > +		 * current then treat it like the PID=0 case above.
> > +		 */
> > +		if (p == current)
> > +			return current->group_leader;
> > +
> > +		/*
> > +		 * For foreign processes require that p is group leader.
> >  		 */
> >  		if (!has_group_leader_pid(p))
> >  			return NULL;
> 
> So, right after you should have that:
> 
>                 if (same_thread_group(p, current))
>                         return p;
> 
> Which I suggested to convert as:
> 
>                 if (p == current)
>                         return p;

Indeed :)