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

[Ben Hutchings <ben@decadent.org.uk>]

[-- Attachment #1: Type: text/plain, Size: 1565 bytes --]

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;
[...]

-- 
Ben Hutchings
Reality is just a crutch for people who can't handle science fiction.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]