Re: [PATCH v2 4/8] futex: don't leak robust_list pointer

From: Oleg Nesterov <oleg@redhat.com>
To: Jann Horn <jann@thejh.net>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>,
	Roland McGrath <roland@hack.frob.com>,
	John Johansen <john.johansen@canonical.com>,
	James Morris <james.l.morris@oracle.com>,
	"Serge E. Hallyn" <serge@hallyn.com>,
	Paul Moore <aul@paul-moore.com>,
	Stephen Smalley <sds@tycho.nsa.gov>,
	Eric Paris <eparis@parisplace.org>,
	Casey Schaufler <casey@schaufler-ca.com>,
	Kees Cook <keescook@chromium.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Janis Danisevskis <jdanis@google.com>,
	Seth Forshee <seth.forshee@canonical.com>,
	"Eric . Biederman" <ebiederm@xmission.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Benjamin LaHaise <bcrl@kvack.org>,
	Ben Hutchings <ben@decadent.org.uk>,
	Andy Lutomirski <luto@amacapital.net>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	linux-fsdevel@vger.kernel.org,
	linux-security-module@vger.kernel.org, security@kernel.org
Subject: Re: [PATCH v2 4/8] futex: don't leak robust_list pointer
Date: Fri, 30 Sep 2016 16:52:57 +0200	[thread overview]
Message-ID: <20160930145256.GB12862@redhat.com> (raw)
In-Reply-To: <1474663238-22134-5-git-send-email-jann@thejh.net>

On 09/23, Jann Horn wrote:
>
> This prevents an attacker from determining the robust_list or
> compat_robust_list userspace pointer of a process created by executing
> a setuid binary. Such an attack could be performed by racing
> get_robust_list() with a setuid execution. The impact of this issue is that
> an attacker could theoretically bypass ASLR when attacking setuid binaries.

Well. I am not sure this actually needs a fix, but I won't argue.

I can't really understand what this patch actually fixes,

> @@ -3007,31 +3007,43 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
>  	if (!futex_cmpxchg_enabled)
>  		return -ENOSYS;
>
> -	rcu_read_lock();
> -
> -	ret = -ESRCH;
> -	if (!pid)
> +	if (!pid) {
>  		p = current;
> -	else {
> +		get_task_struct(p);
> +	} else {
> +		rcu_read_lock();
>  		p = find_task_by_vpid(pid);
> +		/* pin the task to permit dropping the RCU read lock before
> +		 * acquiring the mutex
> +		 */
> +		if (p)
> +			get_task_struct(p);
> +		rcu_read_unlock();
>  		if (!p)
> -			goto err_unlock;
> +			return -ESRCH;
>  	}
>
> +	ret = mutex_lock_killable(&p->signal->cred_guard_light);
> +	if (ret)
> +		goto err_put;
> +
>  	ret = -EPERM;
>  	if (!ptrace_may_access(p, PTRACE_MODE_READ_REALCREDS))
>  		goto err_unlock;
>
>  	head = p->robust_list;
> -	rcu_read_unlock();

OK, suppose it races with setuid exec, and mutex_lock_killable() +
ptrace_may_access() comes after flush_old_exec() but before
install_exec_creds(), in this case ptrace_may_access() can wrongly
succeed.

In theory, it is possible that the execing thread can complete exec,
return to user-mode and call sys_set_robust_list() before we read
head = p->robust_list. Yes, this is unlikely, but unless I am totally
confused the race you are trying to fix is equally unlikely?

perhaps we can make a much simpler change to prevent this, see below.
We can rely on fact that both ptrace_may_access() and exec_mmap()
takes the same task_lock(). Sure, this can "leak" robust_list too,
a set-uid binary can exec and/or lower its credentials after we
read p->robust_list, but personally I think we do not care.

Or I missed something else?

Oleg.

--- x/kernel/futex.c
+++ x/kernel/futex.c
@@ -3019,10 +3019,10 @@ SYSCALL_DEFINE3(get_robust_list, int, pi
 	}
 
 	ret = -EPERM;
+	head = p->robust_list;
 	if (!ptrace_may_access(p, PTRACE_MODE_READ_REALCREDS))
 		goto err_unlock;
 
-	head = p->robust_list;
 	rcu_read_unlock();
 
 	if (put_user(sizeof(*head), len_ptr))

