| Message ID | 1474211117-16674-5-git-send-email-jann@thejh.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Sep 18, 2016 at 05:05:12PM +0200, 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. > > Signed-off-by: Jann Horn <jann@thejh.net> > --- > kernel/futex.c | 31 +++++++++++++++++++++---------- > kernel/futex_compat.c | 31 +++++++++++++++++++++---------- > 2 files changed, 42 insertions(+), 20 deletions(-) > > diff --git a/kernel/futex.c b/kernel/futex.c > index 46cb3a3..002f056 100644 > --- a/kernel/futex.c > +++ b/kernel/futex.c > @@ -3007,31 +3007,42 @@ 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); > - if (!p) > - goto err_unlock; > + /* pin the task to permit dropping the RCU read lock before > + * acquiring the mutex > + */ > + get_task_struct(p); get_task_struct() requires a non-null pointer so you can't move the null check below it. Ben. > + rcu_read_unlock(); > } > + if (!p) > + return -ESRCH; [...]
On Sun, Sep 18, 2016 at 07:28:46PM +0100, Ben Hutchings wrote: > On Sun, Sep 18, 2016 at 05:05:12PM +0200, 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. > > > > Signed-off-by: Jann Horn <jann@thejh.net> > > --- > > kernel/futex.c | 31 +++++++++++++++++++++---------- > > kernel/futex_compat.c | 31 +++++++++++++++++++++---------- > > 2 files changed, 42 insertions(+), 20 deletions(-) > > > > diff --git a/kernel/futex.c b/kernel/futex.c > > index 46cb3a3..002f056 100644 > > --- a/kernel/futex.c > > +++ b/kernel/futex.c > > @@ -3007,31 +3007,42 @@ 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); > > - if (!p) > > - goto err_unlock; > > + /* pin the task to permit dropping the RCU read lock before > > + * acquiring the mutex > > + */ > > + get_task_struct(p); > > get_task_struct() requires a non-null pointer so you can't move the > null check below it. Oh, right, thanks. Will fix that in v2.
diff --git a/kernel/futex.c b/kernel/futex.c index 46cb3a3..002f056 100644 --- a/kernel/futex.c +++ b/kernel/futex.c @@ -3007,31 +3007,42 @@ 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); - if (!p) - goto err_unlock; + /* pin the task to permit dropping the RCU read lock before + * acquiring the mutex + */ + get_task_struct(p); + rcu_read_unlock(); } + if (!p) + 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(); + + mutex_unlock(&p->signal->cred_guard_light); + put_task_struct(p); if (put_user(sizeof(*head), len_ptr)) return -EFAULT; return put_user(head, head_ptr); err_unlock: - rcu_read_unlock(); - + mutex_unlock(¤t->signal->cred_guard_light); +err_put: + put_task_struct(p); return ret; } diff --git a/kernel/futex_compat.c b/kernel/futex_compat.c index 4ae3232..241b4a9 100644 --- a/kernel/futex_compat.c +++ b/kernel/futex_compat.c @@ -143,31 +143,42 @@ COMPAT_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); - if (!p) - goto err_unlock; + /* pin the task to permit dropping the RCU read lock before + * acquiring the mutex + */ + get_task_struct(p); + rcu_read_unlock(); } + if (!p) + 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->compat_robust_list; - rcu_read_unlock(); + + mutex_unlock(&p->signal->cred_guard_light); + put_task_struct(p); if (put_user(sizeof(*head), len_ptr)) return -EFAULT; return put_user(ptr_to_compat(head), head_ptr); err_unlock: - rcu_read_unlock(); - + mutex_unlock(¤t->signal->cred_guard_light); +err_put: + put_task_struct(p); return ret; }
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. Signed-off-by: Jann Horn <jann@thejh.net> --- kernel/futex.c | 31 +++++++++++++++++++++---------- kernel/futex_compat.c | 31 +++++++++++++++++++++---------- 2 files changed, 42 insertions(+), 20 deletions(-)