netdev - Re: [PATCH bpf-next] bpf: Fix trampoline usage in preempt

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20200121045442.GN2935@paulmck-ThinkPad-P72>
Date:   Mon, 20 Jan 2020 20:54:42 -0800
From:   "Paul E. McKenney" <paulmck@...nel.org>
To:     Alexei Starovoitov <ast@...nel.org>
Cc:     davem@...emloft.net, daniel@...earbox.net, jannh@...gle.com,
        netdev@...r.kernel.org, bpf@...r.kernel.org, kernel-team@...com
Subject: Re: [PATCH bpf-next] bpf: Fix trampoline usage in preempt

On Mon, Jan 20, 2020 at 07:22:31PM -0800, Alexei Starovoitov wrote:
> Though the second half of trampoline page is unused a task could be
> preempted in the middle of the first half of trampoline and two
> updates to trampoline would change the code from underneath the
> preempted task. Hence wait for tasks to voluntarily schedule or go
> to userspace.
> Add similar wait before freeing the trampoline.
> 
> Fixes: fec56f5890d9 ("bpf: Introduce BPF trampoline")
> Reported-by: Jann Horn <jannh@...gle.com>
> Signed-off-by: Alexei Starovoitov <ast@...nel.org>
> ---
>  kernel/bpf/trampoline.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index 79a04417050d..7657ede7aee2 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
> @@ -160,6 +160,14 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr)
>  	if (fexit_cnt)
>  		flags = BPF_TRAMP_F_CALL_ORIG | BPF_TRAMP_F_SKIP_FRAME;
>  
> +	/* Though the second half of trampoline page is unused a task could be
> +	 * preempted in the middle of the first half of trampoline and two
> +	 * updates to trampoline would change the code from underneath the
> +	 * preempted task. Hence wait for tasks to voluntarily schedule or go
> +	 * to userspace.
> +	 */
> +	synchronize_rcu_tasks();

So in this case, although the trampoline is not freed, it is reused.
And we need to clear everyone off of the trampoline before reusing it.

If this states the situation correctly:

Acked-by: Paul E. McKenney <paulmck@...nel.org>

>  	err = arch_prepare_bpf_trampoline(new_image, new_image + PAGE_SIZE / 2,
>  					  &tr->func.model, flags,
>  					  fentry, fentry_cnt,
> @@ -251,6 +259,8 @@ void bpf_trampoline_put(struct bpf_trampoline *tr)
>  		goto out;
>  	if (WARN_ON_ONCE(!hlist_empty(&tr->progs_hlist[BPF_TRAMP_FEXIT])))
>  		goto out;
> +	/* wait for tasks to get out of trampoline before freeing it */
> +	synchronize_rcu_tasks();
>  	bpf_jit_free_exec(tr->image);
>  	hlist_del(&tr->hlist);
>  	kfree(tr);
> -- 
> 2.23.0
>