[patch V2 0/6] posix-cpu-timers: Fix bogus permission checks

[patch V2 0/6] posix-cpu-timers: Fix bogus permission checks

[Thomas Gleixner]

When cleaning up posix-cpu-timers 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.

Changes vs. V1:

  - Address the review comments from Frederic

  - Actually return -EPERM when the permission check fails.
    See patch 6/6 for rationale

V1 can be found here:

  https://lore.kernel.org/r/20190905120339.561100423@linutronix.de

I still did not come around to write self tests and won't do so in the next
weeks as I'm traveling as of tomorrow and then going on vacation (finally) :)

Thanks,

	tglx

---
 posix-cpu-timers.c |   71 ++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 49 insertions(+), 22 deletions(-)

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

[Thomas Gleixner]

From: Thomas Gleixner <tglx@linutronix.de>

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>
Reviewed-by: Frederic Weisbecker <frederic@kernel.org>

---
 kernel/time/posix-cpu-timers.c |   16 +++++++++++++++-
 1 file changed, 15 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>
@@ -82,7 +83,20 @@ 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:
+	 */
+	return ptrace_may_access(p, PTRACE_MODE_ATTACH_REALCREDS) ? p : NULL;
 }
 
 static struct task_struct *__get_task_for_clock(const clockid_t clock,

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

[Thomas Gleixner]

From: Thomas Gleixner <tglx@linutronix.de>

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>
---
 kernel/time/posix-cpu-timers.c |   13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -76,8 +76,19 @@ static struct task_struct *lookup_task(c
 		 * Timers need the group leader because they take a
 		 * reference on it and store the task pointer until the
 		 * timer is destroyed.
+		 *
+		 * current can obviously access it's own process, 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;
 	}
 
 	/*

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

[Thomas Gleixner]

From: Thomas Gleixner <tglx@linutronix.de>

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>
Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
---
 kernel/time/posix-cpu-timers.c |   66 +++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 34 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,50 +65,47 @@ 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(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.
+		 * 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 it's own process, so spare
-		 * the ptrace check below.
+		 * 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 {
+		/*
+		 * Process wide timers need the group leader because they
+		 * take a reference on it and store the task pointer until
+		 * the timer is destroyed.
+		 */
+		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 V2 4/6] posix-cpu-timers: Make PID=0 and PID=self handling consistent

[Thomas Gleixner]

From: Thomas Gleixner <tglx@linutronix.de>

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>
Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
---
V2: Remove the extra same_thread_group() check which is pointless.
---
 kernel/time/posix-cpu-timers.c |   14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -90,18 +90,20 @@ static struct task_struct *lookup_task(c
 
 	} else {
 		/*
+		 * Timer is going to be attached to a process. If p is
+		 * current then treat it like the PID=0 case above.
+		 * This also avoids the ptrace overhead.
+		 */
+		if (p == current)
+			return current->group_leader;
+
+		/*
 		 * Process wide timers need the group leader because they
 		 * take a reference on it and store the task pointer until
 		 * the timer is destroyed.
 		 */
 		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;
 	}
 
 	/* Decide based on the ptrace permissions. */

[patch V2 5/6] posix-cpu-timers: Return PTR_ERR() from lookup_task()

[Thomas Gleixner]

To prepare for changing the return code to -EPERM when the ptrace
permission check fails, use PTR_ERR() to return the error information from
lookup_task() and fixup all call sites.

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

--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -63,7 +63,7 @@ static struct task_struct *lookup_task(c
 
 	p = find_task_by_vpid(pid);
 	if (!p)
-		return p;
+		return ERR_PTR(-EINVAL);
 
 	if (gettime) {
 		/*
@@ -103,11 +103,11 @@ static struct task_struct *lookup_task(c
 		 * the timer is destroyed.
 		 */
 		if (!has_group_leader_pid(p))
-			return NULL;
+			return ERR_PTR(-EINVAL);
 	}
 
 	/* Decide based on the ptrace permissions. */
-	return ptrace_may_access(p, mode) ? p : NULL;
+	return ptrace_may_access(p, mode) ? p : ERR_PTR(-EINVAL);
 }
 
 static struct task_struct *__get_task_for_clock(const clockid_t clock,
@@ -118,11 +118,11 @@ static struct task_struct *__get_task_fo
 	struct task_struct *p;
 
 	if (CPUCLOCK_WHICH(clock) >= CPUCLOCK_MAX)
-		return NULL;
+		return ERR_PTR(-EINVAL);
 
 	rcu_read_lock();
 	p = lookup_task(pid, thread, gettime);
-	if (p && getref)
+	if (!IS_ERR(p) && getref)
 		get_task_struct(p);
 	rcu_read_unlock();
 	return p;
@@ -140,7 +140,9 @@ static inline struct task_struct *get_ta
 
 static inline int validate_clock_permissions(const clockid_t clock)
 {
-	return __get_task_for_clock(clock, false, false) ? 0 : -EINVAL;
+	struct task_struct *res =  __get_task_for_clock(clock, false, false);
+
+	return IS_ERR(res) ? PTR_ERR(res) : 0;
 }
 
 /*
@@ -391,8 +393,8 @@ static int posix_cpu_clock_get(const clo
 	u64 t;
 
 	tsk = get_task_for_clock_get(clock);
-	if (!tsk)
-		return -EINVAL;
+	if (IS_ERR(tsk))
+		return PTR_ERR(tsk);
 
 	if (CPUCLOCK_PERTHREAD(clock))
 		t = cpu_clock_sample(clkid, tsk);
@@ -413,8 +415,8 @@ static int posix_cpu_timer_create(struct
 {
 	struct task_struct *p = get_task_for_clock(new_timer->it_clock);
 
-	if (!p)
-		return -EINVAL;
+	if (IS_ERR(p))
+		return PTR_ERR(p);
 
 	new_timer->kclock = &clock_posix_cpu;
 	timerqueue_init(&new_timer->it.cpu.node);

Re: [patch V2 5/6] posix-cpu-timers: Return PTR_ERR() from lookup_task()

[Frederic Weisbecker]

On Mon, Sep 23, 2019 at 04:54:40PM +0200, Thomas Gleixner wrote:
> To prepare for changing the return code to -EPERM when the ptrace
> permission check fails, use PTR_ERR() to return the error information from
> lookup_task() and fixup all call sites.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

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

[patch V2 6/6] posix-cpu-timers: Return -EPERM if ptrace permission check fails

[Thomas Gleixner]

Returning -EINVAL when a permission check fails is not really intuitive and
can cause hard to diagnose problems.

The POSIX specification for clock_gettime() and timer_create() requires to
obtain the clock id first by invoking clock_getcpuclockid().

clock_getcpuclockid() can return -EPERM if the caller does not have
permissions. That does not make sense in two aspects:

 - Nothing prevents the caller to make up a clockid and feed it into the
   syscalls

 - clock_getcpuclockid() is a helper function in glibc which just mangles
   the PID/TID bits to the proper place and glibc cannot do any permission
   checks at all for this function.

In order to prevent abuse the kernel has to do the permission checking in
timer_create() and clock_gettime(). Those functions have only -EINVAL as
documented return values, but returning -EINVAL for a valid clockid when
the permission check fails is not understandable for programmers.

So ignore the POSIX specification and return -EPERM when the ptrace
permission check fails.

Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
V2: New patch.

TODO: Update timer_create.2 and clock_gettime.2 manpages
---
 kernel/time/posix-cpu-timers.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -107,7 +107,7 @@ static struct task_struct *lookup_task(c
 	}
 
 	/* Decide based on the ptrace permissions. */
-	return ptrace_may_access(p, mode) ? p : ERR_PTR(-EINVAL);
+	return ptrace_may_access(p, mode) ? p : ERR_PTR(-EPERM);
 }
 
 static struct task_struct *__get_task_for_clock(const clockid_t clock,

Re: [patch V2 6/6] posix-cpu-timers: Return -EPERM if ptrace permission check fails

[Frederic Weisbecker]

On Mon, Sep 23, 2019 at 04:54:41PM +0200, Thomas Gleixner wrote:
> Returning -EINVAL when a permission check fails is not really intuitive and
> can cause hard to diagnose problems.
> 
> The POSIX specification for clock_gettime() and timer_create() requires to
> obtain the clock id first by invoking clock_getcpuclockid().
> 
> clock_getcpuclockid() can return -EPERM if the caller does not have
> permissions. That does not make sense in two aspects:
> 
>  - Nothing prevents the caller to make up a clockid and feed it into the
>    syscalls
> 
>  - clock_getcpuclockid() is a helper function in glibc which just mangles
>    the PID/TID bits to the proper place and glibc cannot do any permission
>    checks at all for this function.
> 
> In order to prevent abuse the kernel has to do the permission checking in
> timer_create() and clock_gettime(). Those functions have only -EINVAL as
> documented return values, but returning -EINVAL for a valid clockid when
> the permission check fails is not understandable for programmers.
> 
> So ignore the POSIX specification and return -EPERM when the ptrace
> permission check fails.
> 
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

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