| Message ID | 1494537019-4465-1-git-send-email-leitao@debian.org |
|---|---|
| State | New |
| Headers | show |
On Thu, May 11, 2017 at 06:10:19PM -0300, Breno Leitao wrote: > From: Janis Danisevskis <jdanis@google.com> > > BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1690225 > > The PR_DUMPABLE flag causes the pid related paths of the proc file > system to be owned by ROOT. > > The implementation of pthread_set/getname_np however needs access to > /proc/<pid>/task/<tid>/comm. If PR_DUMPABLE is false this > implementation is locked out. > > This patch installs a special permission function for the file "comm" > that grants read and write access to all threads of the same group > regardless of the ownership of the inode. For all other threads the > function falls back to the generic inode permission check. > > [akpm@linux-foundation.org: fix spello in comment] > Signed-off-by: Janis Danisevskis <jdanis@google.com> > Acked-by: Kees Cook <keescook@chromium.org> > Cc: Al Viro <viro@zeniv.linux.org.uk> > Cc: Cyrill Gorcunov <gorcunov@openvz.org> > Cc: Alexey Dobriyan <adobriyan@gmail.com> > Cc: Colin Ian King <colin.king@canonical.com> > Cc: David Rientjes <rientjes@google.com> > Cc: Minfei Huang <mnfhuang@gmail.com> > Cc: John Stultz <john.stultz@linaro.org> > Cc: Calvin Owens <calvinowens@fb.com> > Cc: Jann Horn <jann@thejh.net> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org> > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> > (cherry picked from commit 1b3044e39a89cb1d4d5313da477e8dfea2b5232d) > Signed-off-by: Breno Leitao <breno.leitao@gmail.com> Clean cherry pick, has been upstream since 4.7. Acked-by: Seth Forshee <seth.forshee@canonical.com>
On 11/05/17 22:10, Breno Leitao wrote: > From: Janis Danisevskis <jdanis@google.com> > > BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1690225 > > The PR_DUMPABLE flag causes the pid related paths of the proc file > system to be owned by ROOT. > > The implementation of pthread_set/getname_np however needs access to > /proc/<pid>/task/<tid>/comm. If PR_DUMPABLE is false this > implementation is locked out. > > This patch installs a special permission function for the file "comm" > that grants read and write access to all threads of the same group > regardless of the ownership of the inode. For all other threads the > function falls back to the generic inode permission check. > > [akpm@linux-foundation.org: fix spello in comment] > Signed-off-by: Janis Danisevskis <jdanis@google.com> > Acked-by: Kees Cook <keescook@chromium.org> > Cc: Al Viro <viro@zeniv.linux.org.uk> > Cc: Cyrill Gorcunov <gorcunov@openvz.org> > Cc: Alexey Dobriyan <adobriyan@gmail.com> > Cc: Colin Ian King <colin.king@canonical.com> > Cc: David Rientjes <rientjes@google.com> > Cc: Minfei Huang <mnfhuang@gmail.com> > Cc: John Stultz <john.stultz@linaro.org> > Cc: Calvin Owens <calvinowens@fb.com> > Cc: Jann Horn <jann@thejh.net> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org> > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> > (cherry picked from commit 1b3044e39a89cb1d4d5313da477e8dfea2b5232d) > Signed-off-by: Breno Leitao <breno.leitao@gmail.com> > --- > fs/proc/base.c | 42 +++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 41 insertions(+), 1 deletion(-) > > diff --git a/fs/proc/base.c b/fs/proc/base.c > index b4acb8837c79..cfc87edd7adf 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -3081,6 +3081,44 @@ int proc_pid_readdir(struct file *file, struct dir_context *ctx) > } > > /* > + * proc_tid_comm_permission is a special permission function exclusively > + * used for the node /proc/<pid>/task/<tid>/comm. > + * It bypasses generic permission checks in the case where a task of the same > + * task group attempts to access the node. > + * The rationale behind this is that glibc and bionic access this node for > + * cross thread naming (pthread_set/getname_np(!self)). However, if > + * PR_SET_DUMPABLE gets set to 0 this node among others becomes uid=0 gid=0, > + * which locks out the cross thread naming implementation. > + * This function makes sure that the node is always accessible for members of > + * same thread group. > + */ > +static int proc_tid_comm_permission(struct inode *inode, int mask) > +{ > + bool is_same_tgroup; > + struct task_struct *task; > + > + task = get_proc_task(inode); > + if (!task) > + return -ESRCH; > + is_same_tgroup = same_thread_group(current, task); > + put_task_struct(task); > + > + if (likely(is_same_tgroup && !(mask & MAY_EXEC))) { > + /* This file (/proc/<pid>/task/<tid>/comm) can always be > + * read or written by the members of the corresponding > + * thread group. > + */ > + return 0; > + } > + > + return generic_permission(inode, mask); > +} > + > +static const struct inode_operations proc_tid_comm_inode_operations = { > + .permission = proc_tid_comm_permission, > +}; > + > +/* > * Tasks > */ > static const struct pid_entry tid_base_stuff[] = { > @@ -3098,7 +3136,9 @@ static const struct pid_entry tid_base_stuff[] = { > #ifdef CONFIG_SCHED_DEBUG > REG("sched", S_IRUGO|S_IWUSR, proc_pid_sched_operations), > #endif > - REG("comm", S_IRUGO|S_IWUSR, proc_pid_set_comm_operations), > + NOD("comm", S_IFREG|S_IRUGO|S_IWUSR, > + &proc_tid_comm_inode_operations, > + &proc_pid_set_comm_operations, {}), > #ifdef CONFIG_HAVE_ARCH_TRACEHOOK > ONE("syscall", S_IRUSR, proc_pid_syscall), > #endif > The bug report is lacking the normal SRU bullet points such as "regression potential" and a "test case". However, this is an upstream fix to a known issue and as Seth pointed out has been around since 4.7, so I won't quibble on this. Perhaps the bug report can be updated in accordance with the SRU Policy, e.g. https://wiki.ubuntu.com/StableReleaseUpdates Fix looks sane, so.. Acked-by: Colin Ian King <colin.king@canonical.com>
Applied to xenial master-next branch. Thanks. Cascardo.
diff --git a/fs/proc/base.c b/fs/proc/base.c index b4acb8837c79..cfc87edd7adf 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -3081,6 +3081,44 @@ int proc_pid_readdir(struct file *file, struct dir_context *ctx) } /* + * proc_tid_comm_permission is a special permission function exclusively + * used for the node /proc/<pid>/task/<tid>/comm. + * It bypasses generic permission checks in the case where a task of the same + * task group attempts to access the node. + * The rationale behind this is that glibc and bionic access this node for + * cross thread naming (pthread_set/getname_np(!self)). However, if + * PR_SET_DUMPABLE gets set to 0 this node among others becomes uid=0 gid=0, + * which locks out the cross thread naming implementation. + * This function makes sure that the node is always accessible for members of + * same thread group. + */ +static int proc_tid_comm_permission(struct inode *inode, int mask) +{ + bool is_same_tgroup; + struct task_struct *task; + + task = get_proc_task(inode); + if (!task) + return -ESRCH; + is_same_tgroup = same_thread_group(current, task); + put_task_struct(task); + + if (likely(is_same_tgroup && !(mask & MAY_EXEC))) { + /* This file (/proc/<pid>/task/<tid>/comm) can always be + * read or written by the members of the corresponding + * thread group. + */ + return 0; + } + + return generic_permission(inode, mask); +} + +static const struct inode_operations proc_tid_comm_inode_operations = { + .permission = proc_tid_comm_permission, +}; + +/* * Tasks */ static const struct pid_entry tid_base_stuff[] = { @@ -3098,7 +3136,9 @@ static const struct pid_entry tid_base_stuff[] = { #ifdef CONFIG_SCHED_DEBUG REG("sched", S_IRUGO|S_IWUSR, proc_pid_sched_operations), #endif - REG("comm", S_IRUGO|S_IWUSR, proc_pid_set_comm_operations), + NOD("comm", S_IFREG|S_IRUGO|S_IWUSR, + &proc_tid_comm_inode_operations, + &proc_pid_set_comm_operations, {}), #ifdef CONFIG_HAVE_ARCH_TRACEHOOK ONE("syscall", S_IRUSR, proc_pid_syscall), #endif