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

[Colin Ian King]

On 11/05/17 22:10, Breno Leitao wrote:
> From: Janis Danisevskis <jdanis at 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 at linux-foundation.org: fix spello in comment]
> Signed-off-by: Janis Danisevskis <jdanis at google.com>
> Acked-by: Kees Cook <keescook at chromium.org>
> Cc: Al Viro <viro at zeniv.linux.org.uk>
> Cc: Cyrill Gorcunov <gorcunov at openvz.org>
> Cc: Alexey Dobriyan <adobriyan at gmail.com>
> Cc: Colin Ian King <colin.king at canonical.com>
> Cc: David Rientjes <rientjes at google.com>
> Cc: Minfei Huang <mnfhuang at gmail.com>
> Cc: John Stultz <john.stultz at linaro.org>
> Cc: Calvin Owens <calvinowens at fb.com>
> Cc: Jann Horn <jann at thejh.net>
> Signed-off-by: Andrew Morton <akpm at linux-foundation.org>
> Signed-off-by: Linus Torvalds <torvalds at linux-foundation.org>
> (cherry picked from commit 1b3044e39a89cb1d4d5313da477e8dfea2b5232d)
> Signed-off-by: Breno Leitao <breno.leitao at 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 at canonical.com>