netdev - Re: [PATCH bpf-next v6 02/10] bpf: add bpf_get

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]

Message-ID: <89ee3446-8119-8bf4-ee69-bd43e4167a2e@iogearbox.net>
Date:   Wed, 25 Apr 2018 11:00:40 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Yonghong Song <yhs@...com>, ast@...com, netdev@...r.kernel.org,
        ecree@...arflare.com
Cc:     kernel-team@...com
Subject: Re: [PATCH bpf-next v6 02/10] bpf: add bpf_get_stack helper

On 04/23/2018 11:27 PM, Yonghong Song wrote:
> Currently, stackmap and bpf_get_stackid helper are provided
> for bpf program to get the stack trace. This approach has
> a limitation though. If two stack traces have the same hash,
> only one will get stored in the stackmap table,
> so some stack traces are missing from user perspective.
> 
> This patch implements a new helper, bpf_get_stack, will
> send stack traces directly to bpf program. The bpf program
> is able to see all stack traces, and then can do in-kernel
> processing or send stack traces to user space through
> shared map or bpf_perf_event_output.
> 
> Acked-by: Alexei Starovoitov <ast@...com>
> Signed-off-by: Yonghong Song <yhs@...com>
[...]
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index d315b39..bf22eca 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -31,6 +31,7 @@
>  #include <linux/rbtree_latch.h>
>  #include <linux/kallsyms.h>
>  #include <linux/rcupdate.h>
> +#include <linux/perf_event.h>
>  
>  #include <asm/unaligned.h>
>  
> @@ -1709,6 +1710,10 @@ static void bpf_prog_free_deferred(struct work_struct *work)
>  	aux = container_of(work, struct bpf_prog_aux, work);
>  	if (bpf_prog_is_dev_bound(aux))
>  		bpf_prog_offload_destroy(aux->prog);
> +#ifdef CONFIG_PERF_EVENTS
> +	if (aux->prog->need_callchain_buf)
> +		put_callchain_buffers();
> +#endif
>  	for (i = 0; i < aux->func_cnt; i++)
>  		bpf_jit_free(aux->func[i]);
>  	if (aux->func_cnt) {
[...]
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index fe23dc5a..1ee71f6 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -1360,6 +1360,16 @@ static int bpf_prog_load(union bpf_attr *attr)
>  	if (err)
>  		goto free_used_maps;
>  
> +	if (prog->need_callchain_buf) {
> +#ifdef CONFIG_PERF_EVENTS
> +		err = get_callchain_buffers(sysctl_perf_event_max_stack);
> +#else
> +		err = -ENOTSUPP;
> +#endif
> +		if (err)
> +			goto free_used_maps;
> +	}
> +
>  	err = bpf_prog_new_fd(prog);
>  	if (err < 0) {
>  		/* failed to allocate fd.
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 5dd1dcb..aba9425 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -2460,6 +2460,9 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
>  	if (err)
>  		return err;
>  
> +	if (func_id == BPF_FUNC_get_stack)
> +		env->prog->need_callchain_buf = true;
> +
>  	if (changes_data)
>  		clear_all_pkt_pointers(env);
>  	return 0;

The above three hunks will cause a use-after-free on the perf callchain buffers.

In check_helper_call() you mark the prog with need_callchain_buf, where the
program hasn't fully completed verification phase yet, meaning some buggy prog
will still bail out.

However, you do the get_callchain_buffers() at a much later phase, so when you
bail out with error from bpf_check(), you take the free_used_maps error path
which calls bpf_prog_free().

The latter calls into bpf_prog_free_deferred() where you do the put_callchain_buffers()
since the need_callchain_buf is marked, but without prior get_callchain_buffers().