netdev - Re: [PATCH 03/18] bpf: Add struct bpf

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: <d61ff7d5-f0a7-8828-cf94-54936670f244@iogearbox.net>
Date:   Wed, 19 Feb 2020 00:03:49 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Jiri Olsa <jolsa@...nel.org>, Alexei Starovoitov <ast@...nel.org>
Cc:     netdev@...r.kernel.org, bpf@...r.kernel.org,
        Andrii Nakryiko <andriin@...com>, Yonghong Song <yhs@...com>,
        Song Liu <songliubraving@...com>,
        Martin KaFai Lau <kafai@...com>,
        Jakub Kicinski <kuba@...nel.org>,
        David Miller <davem@...hat.com>,
        Björn Töpel <bjorn.topel@...el.com>,
        John Fastabend <john.fastabend@...il.com>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        Arnaldo Carvalho de Melo <acme@...hat.com>
Subject: Re: [PATCH 03/18] bpf: Add struct bpf_ksym

On 2/16/20 8:29 PM, Jiri Olsa wrote:
> Adding 'struct bpf_ksym' object that will carry the
> kallsym information for bpf symbol. Adding the start
> and end address to begin with. It will be used by
> bpf_prog, bpf_trampoline, bpf_dispatcher.
> 
> Using the bpf_func for program symbol start instead
> of the image start, because it will be used later for
> kallsyms program value and it makes no difference
> (compared to the image start) for sorting bpf programs.
> 
> Signed-off-by: Jiri Olsa <jolsa@...nel.org>
> ---
>   include/linux/bpf.h |  6 ++++++
>   kernel/bpf/core.c   | 26 +++++++++++---------------
>   2 files changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index be7afccc9459..5ad8eea1cd37 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -462,6 +462,11 @@ int arch_prepare_bpf_trampoline(void *image, void *image_end,
>   u64 notrace __bpf_prog_enter(void);
>   void notrace __bpf_prog_exit(struct bpf_prog *prog, u64 start);
>   
> +struct bpf_ksym {
> +	unsigned long		 start;
> +	unsigned long		 end;
> +};
> +
>   enum bpf_tramp_prog_type {
>   	BPF_TRAMP_FENTRY,
>   	BPF_TRAMP_FEXIT,
> @@ -643,6 +648,7 @@ struct bpf_prog_aux {
>   	u32 size_poke_tab;
>   	struct latch_tree_node ksym_tnode;
>   	struct list_head ksym_lnode;
> +	struct bpf_ksym ksym;
>   	const struct bpf_prog_ops *ops;
>   	struct bpf_map **used_maps;
>   	struct bpf_prog *prog;
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 973a20d49749..39a9e4184900 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -524,17 +524,15 @@ int bpf_jit_harden   __read_mostly;
>   long bpf_jit_limit   __read_mostly;
>   
>   static __always_inline void
> -bpf_get_prog_addr_region(const struct bpf_prog *prog,
> -			 unsigned long *symbol_start,
> -			 unsigned long *symbol_end)
> +bpf_get_prog_addr_region(const struct bpf_prog *prog)
>   {
>   	const struct bpf_binary_header *hdr = bpf_jit_binary_hdr(prog);
>   	unsigned long addr = (unsigned long)hdr;
>   
>   	WARN_ON_ONCE(!bpf_prog_ebpf_jited(prog));
>   
> -	*symbol_start = addr;
> -	*symbol_end   = addr + hdr->pages * PAGE_SIZE;
> +	prog->aux->ksym.start = (unsigned long) prog->bpf_func;

Your commit descriptions are too terse. :/ What does "because it will be used
later for kallsyms program value" mean exactly compared to how it's used today
for programs?

Is this a requirement to have them point exactly to prog->bpf_func and if so
why? My concern is that bpf_func has a random offset from hdr, so even if the
/proc/kallsyms would be readable with concrete addresses for !cap_sys_admin
users, it's still not the concrete start address being exposed there, but the
allocated range instead.

> +	prog->aux->ksym.end   = addr + hdr->pages * PAGE_SIZE;
>   }
>