On Fri, Sep 30, 2016 at 04:52:57PM +0200, Oleg Nesterov wrote:
> 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.

I take cred_guard_light in flush_old_exec() and release it in
install_exec_creds(), so that shouldn't work, I think.


> 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?

No - I think your patch would work, too, apart from the potential
leak you mentioned.

But unless this breaks something, I would prefer to do it properly.

> 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))
>