Quantcast

[PATCH] [Xenial] procfs: fix pthread cross-thread naming if !PR_DUMPABLE

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH] [Xenial] procfs: fix pthread cross-thread naming if !PR_DUMPABLE

Breno Leitao
From: Janis Danisevskis <[hidden email]>

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.

[[hidden email]: fix spello in comment]
Signed-off-by: Janis Danisevskis <[hidden email]>
Acked-by: Kees Cook <[hidden email]>
Cc: Al Viro <[hidden email]>
Cc: Cyrill Gorcunov <[hidden email]>
Cc: Alexey Dobriyan <[hidden email]>
Cc: Colin Ian King <[hidden email]>
Cc: David Rientjes <[hidden email]>
Cc: Minfei Huang <[hidden email]>
Cc: John Stultz <[hidden email]>
Cc: Calvin Owens <[hidden email]>
Cc: Jann Horn <[hidden email]>
Signed-off-by: Andrew Morton <[hidden email]>
Signed-off-by: Linus Torvalds <[hidden email]>
(cherry picked from commit 1b3044e39a89cb1d4d5313da477e8dfea2b5232d)
Signed-off-by: Breno Leitao <[hidden email]>
---
 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
--
2.11.0


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

ACK: [PATCH] [Xenial] procfs: fix pthread cross-thread naming if !PR_DUMPABLE

Seth Forshee
On Thu, May 11, 2017 at 06:10:19PM -0300, Breno Leitao wrote:

> From: Janis Danisevskis <[hidden email]>
>
> 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.
>
> [[hidden email]: fix spello in comment]
> Signed-off-by: Janis Danisevskis <[hidden email]>
> Acked-by: Kees Cook <[hidden email]>
> Cc: Al Viro <[hidden email]>
> Cc: Cyrill Gorcunov <[hidden email]>
> Cc: Alexey Dobriyan <[hidden email]>
> Cc: Colin Ian King <[hidden email]>
> Cc: David Rientjes <[hidden email]>
> Cc: Minfei Huang <[hidden email]>
> Cc: John Stultz <[hidden email]>
> Cc: Calvin Owens <[hidden email]>
> Cc: Jann Horn <[hidden email]>
> Signed-off-by: Andrew Morton <[hidden email]>
> Signed-off-by: Linus Torvalds <[hidden email]>
> (cherry picked from commit 1b3044e39a89cb1d4d5313da477e8dfea2b5232d)
> Signed-off-by: Breno Leitao <[hidden email]>

Clean cherry pick, has been upstream since 4.7.

Acked-by: Seth Forshee <[hidden email]>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] [Xenial] procfs: fix pthread cross-thread naming if !PR_DUMPABLE

Colin Ian King-2
In reply to this post by Breno Leitao
On 11/05/17 22:10, Breno Leitao wrote:

> From: Janis Danisevskis <[hidden email]>
>
> 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.
>
> [[hidden email]: fix spello in comment]
> Signed-off-by: Janis Danisevskis <[hidden email]>
> Acked-by: Kees Cook <[hidden email]>
> Cc: Al Viro <[hidden email]>
> Cc: Cyrill Gorcunov <[hidden email]>
> Cc: Alexey Dobriyan <[hidden email]>
> Cc: Colin Ian King <[hidden email]>
> Cc: David Rientjes <[hidden email]>
> Cc: Minfei Huang <[hidden email]>
> Cc: John Stultz <[hidden email]>
> Cc: Calvin Owens <[hidden email]>
> Cc: Jann Horn <[hidden email]>
> Signed-off-by: Andrew Morton <[hidden email]>
> Signed-off-by: Linus Torvalds <[hidden email]>
> (cherry picked from commit 1b3044e39a89cb1d4d5313da477e8dfea2b5232d)
> Signed-off-by: Breno Leitao <[hidden email]>
> ---
>  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 <[hidden email]>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Loading...