[request for inclusion 3.10][PATCH 0/2] fix thread cannot access stat in the same group

[request for inclusion 3.10][PATCH 0/2] fix thread cannot access stat in the same group

[Sheng Yong]

Hi, Greg,

In the case that threads in the same group try to access one of their
/proc/$PID/{stat,exe,etc.}, the thread only gets 0 at some fields, like
eip. This is because that these interfaces only allows the same task to
get these data. But one thread should not deny the access from another
thread in `the same group.

The testcase is:
=====================
#include <sys/types.h>
#include <pthread.h>
#include <stdio.h>
#include <unistd.h>
#include <asm/unistd.h>
#include <fcntl.h>
#include <string.h>
#include <sys/stat.h>

pid_t tid = 0;

void print_stat_eip(pid_t child)
{
        int fd, i;
        char buf[4096], *str, *part;

        sprintf(buf, "/proc/%d/stat", child);

        fd = open(buf, O_RDONLY);
        read(fd, buf, 4096);
        close(fd);
        buf[4095] = '\0';

        str = buf;

        part = strtok(str, " ");
        i = 0;
        while (part) {
                i++;
                if (i == 30) { // eip
                        printf("eip: %s\n", part);
                        break;  
                }
                part = strtok(NULL, " ");
        }    
}

void *child_func(void *arg)
{   
        tid = syscall(__NR_gettid);
        while(1)
                sleep(10000);
        return NULL;
}   

int main(int argc, char **argv)
{
        pthread_t child;

        setuid(1000); // 1000 is the uid of a non-root user
        pthread_create(&child, NULL, child_func, NULL);
        sleep(1);
        print_stat_eip(tid);
}
=====================

The following two patches fix this.

thanks,
Sheng

Mark Grondona (1):
  __ptrace_may_access() should not deny sub-threads

Oleg Nesterov (1):
  include/linux/sched.h: don't use task->pid/tgid in
    same_thread_group/has_group_leader_pid

 include/linux/sched.h | 8 ++++----
 kernel/ptrace.c       | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

-- 
1.8.3.4

[PATCH 1/2] include/linux/sched.h: don't use task->pid/tgid in same_thread_group/has_group_leader_pid

[Sheng Yong]

From: Oleg Nesterov <oleg@redhat.com>

commit e1403b8edf669ff49bbdf602cc97fefa2760cb15 upstream.

task_struct->pid/tgid should go away.

1. Change same_thread_group() to use task->signal for comparison.

2. Change has_group_leader_pid(task) to compare task_pid(task) with
   signal->leader_pid.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Cc: Michal Hocko <mhocko@suse.cz>
Cc: Sergey Dyasly <dserrg@gmail.com>
Reviewed-by: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 include/linux/sched.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 00c1d4f..7cf305d 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2203,15 +2203,15 @@ static inline bool thread_group_leader(struct task_struct *p)
  * all we care about is that we have a task with the appropriate
  * pid, we don't actually care if we have the right task.
  */
-static inline int has_group_leader_pid(struct task_struct *p)
+static inline bool has_group_leader_pid(struct task_struct *p)
 {
-	return p->pid == p->tgid;
+	return task_pid(p) == p->signal->leader_pid;
 }
 
 static inline
-int same_thread_group(struct task_struct *p1, struct task_struct *p2)
+bool same_thread_group(struct task_struct *p1, struct task_struct *p2)
 {
-	return p1->tgid == p2->tgid;
+	return p1->signal == p2->signal;
 }
 
 static inline struct task_struct *next_thread(const struct task_struct *p)
-- 
1.8.3.4

[PATCH 2/2] __ptrace_may_access() should not deny sub-threads

[Sheng Yong]

From: Mark Grondona <mgrondona@llnl.gov>

commit 73af963f9f3036dffed55c3a2898598186db1045 upstream.

__ptrace_may_access() checks get_dumpable/ptrace_has_cap/etc if task !=
current, this can can lead to surprising results.

For example, a sub-thread can't readlink("/proc/self/exe") if the
executable is not readable.  setup_new_exec()->would_dump() notices that
inode_permission(MAY_READ) fails and then it does
set_dumpable(suid_dumpable).  After that get_dumpable() fails.

(It is not clear why proc_pid_readlink() checks get_dumpable(), perhaps we
could add PTRACE_MODE_NODUMPABLE)

Change __ptrace_may_access() to use same_thread_group() instead of "task
== current".  Any security check is pointless when the tasks share the
same ->mm.

Signed-off-by: Mark Grondona <mgrondona@llnl.gov>
Signed-off-by: Ben Woodard <woodard@redhat.com>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 kernel/ptrace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 118323b..30ab206 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -236,7 +236,7 @@ static int __ptrace_may_access(struct task_struct *task, unsigned int mode)
 	 */
 	int dumpable = 0;
 	/* Don't let security modules deny introspection */
-	if (task == current)
+	if (same_thread_group(task, current))
 		return 0;
 	rcu_read_lock();
 	tcred = __task_cred(task);
-- 
1.8.3.4

Re: [request for inclusion 3.10][PATCH 0/2] fix thread cannot access stat in the same group

[Greg KH]

On Mon, May 18, 2015 at 02:07:22AM +0000, Sheng Yong wrote:
> Hi, Greg,
> 
> In the case that threads in the same group try to access one of their
> /proc/$PID/{stat,exe,etc.}, the thread only gets 0 at some fields, like
> eip. This is because that these interfaces only allows the same task to
> get these data. But one thread should not deny the access from another
> thread in `the same group.
> 
> The testcase is:
> =====================
> #include <sys/types.h>
> #include <pthread.h>
> #include <stdio.h>
> #include <unistd.h>
> #include <asm/unistd.h>
> #include <fcntl.h>
> #include <string.h>
> #include <sys/stat.h>
> 
> pid_t tid = 0;
> 
> void print_stat_eip(pid_t child)
> {
>         int fd, i;
>         char buf[4096], *str, *part;
> 
>         sprintf(buf, "/proc/%d/stat", child);
> 
>         fd = open(buf, O_RDONLY);
>         read(fd, buf, 4096);
>         close(fd);
>         buf[4095] = '\0';
> 
>         str = buf;
> 
>         part = strtok(str, " ");
>         i = 0;
>         while (part) {
>                 i++;
>                 if (i == 30) { // eip
>                         printf("eip: %s\n", part);
>                         break;  
>                 }
>                 part = strtok(NULL, " ");
>         }    
> }
> 
> void *child_func(void *arg)
> {   
>         tid = syscall(__NR_gettid);
>         while(1)
>                 sleep(10000);
>         return NULL;
> }   
> 
> int main(int argc, char **argv)
> {
>         pthread_t child;
> 
>         setuid(1000); // 1000 is the uid of a non-root user
>         pthread_create(&child, NULL, child_func, NULL);
>         sleep(1);
>         print_stat_eip(tid);
> }
> =====================
> 
> The following two patches fix this.
> 
> thanks,
> Sheng
> 
> Mark Grondona (1):
>   __ptrace_may_access() should not deny sub-threads
> 
> Oleg Nesterov (1):
>   include/linux/sched.h: don't use task->pid/tgid in
>     same_thread_group/has_group_leader_pid
> 
>  include/linux/sched.h | 8 ++++----
>  kernel/ptrace.c       | 2 +-
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> -- 
> 1.8.3.4

Thanks, both now applied.

greg k-h

Re: [PATCH 1/2] include/linux/sched.h: don't use task->pid/tgid in same_thread_group/has_group_leader_pid

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 523 bytes --]

On Mon, 2015-05-18 at 02:07 +0000, Sheng Yong wrote:
> From: Oleg Nesterov <oleg@redhat.com>
> 
> commit e1403b8edf669ff49bbdf602cc97fefa2760cb15 upstream.
> 
> task_struct->pid/tgid should go away.
> 
> 1. Change same_thread_group() to use task->signal for comparison.
> 
> 2. Change has_group_leader_pid(task) to compare task_pid(task) with
>    signal->leader_pid.
[...]

Queued up for 3.2, thanks.

Ben.

-- 
Ben Hutchings
One of the nice things about standards is that there are so many of them.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH 2/2] __ptrace_may_access() should not deny sub-threads

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 992 bytes --]

On Mon, 2015-05-18 at 02:07 +0000, Sheng Yong wrote:
> From: Mark Grondona <mgrondona@llnl.gov>
> 
> commit 73af963f9f3036dffed55c3a2898598186db1045 upstream.
> 
> __ptrace_may_access() checks get_dumpable/ptrace_has_cap/etc if task !=
> current, this can can lead to surprising results.
> 
> For example, a sub-thread can't readlink("/proc/self/exe") if the
> executable is not readable.  setup_new_exec()->would_dump() notices that
> inode_permission(MAY_READ) fails and then it does
> set_dumpable(suid_dumpable).  After that get_dumpable() fails.
> 
> (It is not clear why proc_pid_readlink() checks get_dumpable(), perhaps we
> could add PTRACE_MODE_NODUMPABLE)
> 
> Change __ptrace_may_access() to use same_thread_group() instead of "task
> == current".  Any security check is pointless when the tasks share the
> same ->mm.
[...]

Queued up for 3.2, thanks.

Ben.

-- 
Ben Hutchings
One of the nice things about standards is that there are so many of them.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]