[SRU groovy 4/9] futex: Ensure the correct return value from futex_lock_pi()

[Thadeu Lima de Souza Cascardo]

On Tue, Mar 09, 2021 at 02:03:27PM -0300, Thadeu Lima de Souza Cascardo wrote:
> From: Thomas Gleixner <tglx at linutronix.de>
> 
> In case that futex_lock_pi() was aborted by a signal or a timeout and the
> task returned without acquiring the rtmutex, but is the designated owner of
> the futex due to a concurrent futex_unlock_pi() fixup_owner() is invoked to
> establish consistent state. In that case it invokes fixup_pi_state_owner()
> which in turn tries to acquire the rtmutex again. If that succeeds then it
> does not propagate this success to fixup_owner() and futex_lock_pi()
> returns -EINTR or -ETIMEOUT despite having the futex locked.
> 
> Return success from fixup_pi_state_owner() in all cases where the current
> task owns the rtmutex and therefore the futex and propagate it correctly
> through fixup_owner(). Fixup the other callsite which does not expect a
> positive return value.
> 
> Fixes: c1e2f0eaf015 ("futex: Avoid violating the 10th rule of futex")
> Signed-off-by: Thomas Gleixner <tglx at linutronix.de>
> Acked-by: Peter Zijlstra (Intel) <peterz at infradead.org>
> Cc: stable at vger.kernel.org
> (cherry picked from commit 12bb3f7f1b03d5913b3f9d4236a488aa7774dfe9)

[cascardo: reverse applying with 04b79c55201f0 for build bisectability]

> CVE-2021-3347
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo at canonical.com>
> ---
>  kernel/futex.c | 31 ++++++++++++++++---------------
>  1 file changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/kernel/futex.c b/kernel/futex.c
> index 22dd904016b1..e8e1dfbcc41a 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -2392,8 +2392,8 @@ static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q,
>  		}
>  
>  		if (__rt_mutex_futex_trylock(&pi_state->pi_mutex)) {
> -			/* We got the lock after all, nothing to fix. */
> -			ret = 0;
> +			/* We got the lock. pi_state is correct. Tell caller. */
> +			ret = 1;
>  			goto out_unlock;
>  		}
>  
> @@ -2421,7 +2421,7 @@ static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q,
>  			 * We raced against a concurrent self; things are
>  			 * already fixed up. Nothing to do.
>  			 */
> -			ret = 0;
> +			ret = 1;
>  			goto out_unlock;
>  		}
>  		newowner = argowner;
> @@ -2467,7 +2467,7 @@ static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q,
>  	raw_spin_unlock(&newowner->pi_lock);
>  	raw_spin_unlock_irq(&pi_state->pi_mutex.wait_lock);
>  
> -	return 0;
> +	return argowner == current;
>  
>  	/*
>  	 * In order to reschedule or handle a page fault, we need to drop the
> @@ -2509,7 +2509,7 @@ static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q,
>  	 * Check if someone else fixed it for us:
>  	 */
>  	if (pi_state->owner != oldowner) {
> -		ret = 0;
> +		ret = argowner == current;
>  		goto out_unlock;
>  	}
>  
> @@ -2542,8 +2542,6 @@ static long futex_wait_restart(struct restart_block *restart);
>   */
>  static int fixup_owner(u32 __user *uaddr, struct futex_q *q, int locked)
>  {
> -	int ret = 0;
> -
>  	if (locked) {
>  		/*
>  		 * Got the lock. We might not be the anticipated owner if we
> @@ -2554,8 +2552,8 @@ static int fixup_owner(u32 __user *uaddr, struct futex_q *q, int locked)
>  		 * stable state, anything else needs more attention.
>  		 */
>  		if (q->pi_state->owner != current)
> -			ret = fixup_pi_state_owner(uaddr, q, current);
> -		return ret ? ret : locked;
> +			return fixup_pi_state_owner(uaddr, q, current);
> +		return 1;
>  	}
>  
>  	/*
> @@ -2566,10 +2564,8 @@ static int fixup_owner(u32 __user *uaddr, struct futex_q *q, int locked)
>  	 * Another speculative read; pi_state->owner == current is unstable
>  	 * but needs our attention.
>  	 */
> -	if (q->pi_state->owner == current) {
> -		ret = fixup_pi_state_owner(uaddr, q, NULL);
> -		return ret;
> -	}
> +	if (q->pi_state->owner == current)
> +		return fixup_pi_state_owner(uaddr, q, NULL);
>  
>  	/*
>  	 * Paranoia check. If we did not take the lock, then we should not be
> @@ -2578,7 +2574,7 @@ static int fixup_owner(u32 __user *uaddr, struct futex_q *q, int locked)
>  	if (WARN_ON_ONCE(rt_mutex_owner(&q->pi_state->pi_mutex) == current))
>  		return fixup_pi_state_owner(uaddr, q, current);
>  
> -	return ret;
> +	return 0;
>  }
>  
>  /**
> @@ -3276,7 +3272,7 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
>  		if (q.pi_state && (q.pi_state->owner != current)) {
>  			spin_lock(q.lock_ptr);
>  			ret = fixup_pi_state_owner(uaddr2, &q, current);
> -			if (ret && rt_mutex_owner(&q.pi_state->pi_mutex) == current) {
> +			if (ret < 0 && rt_mutex_owner(&q.pi_state->pi_mutex) == current) {
>  				pi_state = q.pi_state;
>  				get_pi_state(pi_state);
>  			}
> @@ -3286,6 +3282,11 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
>  			 */
>  			put_pi_state(q.pi_state);
>  			spin_unlock(q.lock_ptr);
> +			/*
> +			 * Adjust the return value. It's either -EFAULT or
> +			 * success (1) but the caller expects 0 for success.
> +			 */
> +			ret = ret < 0 ? ret : 0;
>  		}
>  	} else {
>  		struct rt_mutex *pi_mutex;
> -- 
> 2.27.0
> 
> 
> -- 
> kernel-team mailing list
> kernel-team at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team