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))
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
