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] [day] [month] [year] [list]
Date:   Wed, 19 Feb 2020 10:17:11 +0100
From:   Jiri Olsa <jolsa@...hat.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Jiri Olsa <jolsa@...nel.org>, Alexei Starovoitov <ast@...nel.org>,
        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 Wed, Feb 19, 2020 at 12:03:49AM +0100, Daniel Borkmann wrote:
> 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?

there's symbol_start/symbol_end values originally used to sort
bpf_prog objects, and there's prog->bpf_func value used as address
that is displayed in the /proc/kallsyms

I'm putting prog->bpf_func to bpf_ksym->start, so it's later on
displayed as bpf_prog address in /proc/kallsyms in this patch:

	bpf: Add lnode list node to struct bpf_ksym
	---
	@@ -736,13 +736,13 @@ int bpf_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
			return ret;
	 
		rcu_read_lock();
	-       list_for_each_entry_rcu(aux, &bpf_kallsyms, ksym_lnode) {
	+       list_for_each_entry_rcu(ksym, &bpf_kallsyms, lnode) {
			if (it++ != symnum)
				continue;
	 
	-               strncpy(sym, aux->ksym.name, KSYM_NAME_LEN);
	+               strncpy(sym, ksym->name, KSYM_NAME_LEN);
	 
	-               *value = (unsigned long)aux->prog->bpf_func;
	+               *value = ksym->start;
			*type  = BPF_SYM_ELF_TYPE;


and also the prog->bpf_func value is now used as memory 'start' to
sort bpf_prog objects, which will do the same job as symbol_start

but maybe we could have 'kallsym' value in 'bpf_ksym' which would be
used as value to display in /proc/kallsyms kallsyms, like:

  struct bpf_ksym {
    unsigned long  start;
    unsigned long  end;
    unsigned long  kallsyms;
  }

and keep 'start/end' to be the whole memory bounds for sorting to
avoid any confusion and surprises in future

> 
> 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.

there was last review suggestion from Andrii to display the address
of the actual code start for trampolines and dispatchers instead
of the start of the who;e memory image, which is actually what we
need for perf

jirka