linux-kernel - Re: [PATCH] futex: robust futex maybe never be awaked, on rare situation.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 23 Sep 2019 15:05:19 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Yunfeng Cui <cui.yunfeng@....com.cn>
Cc:     christian@...uner.io, keescook@...omium.org, luto@...capital.net,
        wad@...omium.org, akpm@...ux-foundation.org, mingo@...nel.org,
        mhocko@...e.com, elena.reshetova@...el.com, aarcange@...hat.com,
        ldv@...linux.org, arunks@...eaurora.org, guro@...com,
        joel@...lfernandes.org, viro@...iv.linux.org.uk,
        linux-kernel@...r.kernel.org, xue.zhihong@....com.cn,
        wang.yi59@....com.cn, jiang.xuexin@....com.cn,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH] futex: robust futex maybe never be awaked, on rare
 situation.

On Mon, Sep 23, 2019 at 11:18:20AM +0800, Yunfeng Cui wrote:
> I use model checker find a issue of robust and pi futex. On below
> situation, the owner can't find something in pi_state_list, while
> the requester will be blocked, never be awaked.
> 
> CPU0                       CPU1
>                            futex_lock_pi
>                            /*some cs code*/
> futex_lock_pi
>   futex_lock_pi_atomic
>     ...
>     newval = uval | FUTEX_WAITERS;
>     ret = lock_pi_update_atomic(uaddr, uval, newval);
>     ...
>     attach_to_pi_owner
>      ....
>      p = find_get_task_by_vpid(pid);
>      if (!p)
>        return handle_exit_race(uaddr, uval, NULL);
>        ....
>        raw_spin_lock_irq(&p->pi_lock);
>        ....
>        pi_state = alloc_pi_state();
>        ....
>                            do_exit->mm_release
>                            if (unlikely(tsk->robust_list)) {
>                              exit_robust_list(tsk);
>                              tsk->robust_list = NULL;
>                            }
>                            if (unlikely(!list_empty(&tsk->pi_state_list)))
>                              exit_pi_state_list(tsk); /*WILL MISS*/
>       list_add(&pi_state->list, &p->pi_state_list);
>     WILL BLOCKED, NEVER WAKEUP!

Did you forget/overlook the pi_lock fiddling in do_exit() ? I'm thinking
that would make the above impossible.

> Signed-off-by: Yunfeng Cui <cui.yunfeng@....com.cn>
> Reviewed-by: Bo Wang <wang.bo116@....com.cn>
> Reviewed-by: Yi Wang <wang.yi59@....com.cn>
> ---
>  kernel/fork.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 53e780748fe3..58b90f21dac4 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1277,15 +1277,16 @@ void mm_release(struct task_struct *tsk, struct mm_struct *mm)
>  	if (unlikely(tsk->robust_list)) {
>  		exit_robust_list(tsk);
>  		tsk->robust_list = NULL;
> +		/*Check pi_state_list of task on pi_lock be acquired*/
> +		exit_pi_state_list(tsk);
>  	}
>  #ifdef CONFIG_COMPAT
>  	if (unlikely(tsk->compat_robust_list)) {
>  		compat_exit_robust_list(tsk);
>  		tsk->compat_robust_list = NULL;
> +		exit_pi_state_list(tsk);
>  	}
>  #endif
> -	if (unlikely(!list_empty(&tsk->pi_state_list)))
> -		exit_pi_state_list(tsk);
>  #endif

I'm also thinking this breaks all sorts by not unconditionally calling
exit_pi_state_list(). Specifically, it could leak PI-state for !robust
futexes.

>  
>  	uprobe_free_utask(tsk);