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))
>
Attachment:
signature.asc
Description: Digital signature
