| 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
| ||
|
Message-ID: <20180517115104.7666ff60@cakuba>
Date: Thu, 17 May 2018 11:51:10 -0700
From: Jakub Kicinski <jakub.kicinski@...ronome.com>
To: Sandipan Das <sandipan@...ux.vnet.ibm.com>
Cc: ast@...nel.org, daniel@...earbox.net, netdev@...r.kernel.org,
linuxppc-dev@...ts.ozlabs.org, naveen.n.rao@...ux.vnet.ibm.com
Subject: Re: [PATCH bpf 5/6] tools: bpftool: resolve calls without using imm
field
On Thu, 17 May 2018 12:05:47 +0530, Sandipan Das wrote:
> Currently, we resolve the callee's address for a JITed function
> call by using the imm field of the call instruction as an offset
> from __bpf_call_base. If bpf_jit_kallsyms is enabled, we further
> use this address to get the callee's kernel symbol's name.
>
> For some architectures, such as powerpc64, the imm field is not
> large enough to hold this offset. So, instead of assigning this
> offset to the imm field, the verifier now assigns the subprog
> id. Also, a list of kernel symbol addresses for all the JITed
> functions is provided in the program info. We now use the imm
> field as an index for this list to lookup a callee's symbol's
> address and resolve its name.
>
> Suggested-by: Daniel Borkmann <daniel@...earbox.net>
> Signed-off-by: Sandipan Das <sandipan@...ux.vnet.ibm.com>
A few nit-picks below, thank you for the patch!
> tools/bpf/bpftool/prog.c | 31 +++++++++++++++++++++++++++++++
> tools/bpf/bpftool/xlated_dumper.c | 24 +++++++++++++++++-------
> tools/bpf/bpftool/xlated_dumper.h | 2 ++
> 3 files changed, 50 insertions(+), 7 deletions(-)
>
> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> index 9bdfdf2d3fbe..ac2f62a97e84 100644
> --- a/tools/bpf/bpftool/prog.c
> +++ b/tools/bpf/bpftool/prog.c
> @@ -430,6 +430,10 @@ static int do_dump(int argc, char **argv)
> unsigned char *buf;
> __u32 *member_len;
> __u64 *member_ptr;
> + unsigned int nr_addrs;
> + unsigned long *addrs = NULL;
> + __u32 *ksyms_len;
> + __u64 *ksyms_ptr;
nit: please try to keep the variables ordered longest to shortest like
we do in networking code (please do it in all functions).
> ssize_t n;
> int err;
> int fd;
> @@ -437,6 +441,8 @@ static int do_dump(int argc, char **argv)
> if (is_prefix(*argv, "jited")) {
> member_len = &info.jited_prog_len;
> member_ptr = &info.jited_prog_insns;
> + ksyms_len = &info.nr_jited_ksyms;
> + ksyms_ptr = &info.jited_ksyms;
> } else if (is_prefix(*argv, "xlated")) {
> member_len = &info.xlated_prog_len;
> member_ptr = &info.xlated_prog_insns;
> @@ -496,10 +502,23 @@ static int do_dump(int argc, char **argv)
> return -1;
> }
>
> + nr_addrs = *ksyms_len;
Here and ...
> + if (nr_addrs) {
> + addrs = malloc(nr_addrs * sizeof(__u64));
> + if (!addrs) {
> + p_err("mem alloc failed");
> + free(buf);
> + close(fd);
> + return -1;
You can just jump to err_free here.
> + }
> + }
> +
> memset(&info, 0, sizeof(info));
>
> *member_ptr = ptr_to_u64(buf);
> *member_len = buf_size;
> + *ksyms_ptr = ptr_to_u64(addrs);
> + *ksyms_len = nr_addrs;
... here - this function is getting long, so maybe I'm not seeing
something, but are ksyms_ptr and ksyms_len guaranteed to be initialized?
> err = bpf_obj_get_info_by_fd(fd, &info, &len);
> close(fd);
> @@ -513,6 +532,11 @@ static int do_dump(int argc, char **argv)
> goto err_free;
> }
>
> + if (*ksyms_len > nr_addrs) {
> + p_err("too many addresses returned");
> + goto err_free;
> + }
> +
> if ((member_len == &info.jited_prog_len &&
> info.jited_prog_insns == 0) ||
> (member_len == &info.xlated_prog_len &&
> @@ -558,6 +582,9 @@ static int do_dump(int argc, char **argv)
> dump_xlated_cfg(buf, *member_len);
> } else {
> kernel_syms_load(&dd);
> + dd.jited_ksyms = ksyms_ptr;
> + dd.nr_jited_ksyms = *ksyms_len;
> +
> if (json_output)
> dump_xlated_json(&dd, buf, *member_len, opcodes);
> else
> @@ -566,10 +593,14 @@ static int do_dump(int argc, char **argv)
> }
>
> free(buf);
> + if (addrs)
> + free(addrs);
Free can deal with NULL pointers, no need for an if.
> return 0;
>
> err_free:
> free(buf);
> + if (addrs)
> + free(addrs);
> return -1;
> }
>
> diff --git a/tools/bpf/bpftool/xlated_dumper.c b/tools/bpf/bpftool/xlated_dumper.c
> index 7a3173b76c16..dc8e4eca0387 100644
> --- a/tools/bpf/bpftool/xlated_dumper.c
> +++ b/tools/bpf/bpftool/xlated_dumper.c
> @@ -178,8 +178,12 @@ static const char *print_call_pcrel(struct dump_data *dd,
> snprintf(dd->scratch_buff, sizeof(dd->scratch_buff),
> "%+d#%s", insn->off, sym->name);
> else
else if (address)
saves us the indentation.
> - snprintf(dd->scratch_buff, sizeof(dd->scratch_buff),
> - "%+d#0x%lx", insn->off, address);
> + if (address)
> + snprintf(dd->scratch_buff, sizeof(dd->scratch_buff),
> + "%+d#0x%lx", insn->off, address);
> + else
> + snprintf(dd->scratch_buff, sizeof(dd->scratch_buff),
> + "%+d", insn->off);
> return dd->scratch_buff;
> }
>
> @@ -200,14 +204,20 @@ static const char *print_call(void *private_data,
> const struct bpf_insn *insn)
> {
> struct dump_data *dd = private_data;
> - unsigned long address = dd->address_call_base + insn->imm;
> - struct kernel_sym *sym;
> + unsigned long address = 0;
> + struct kernel_sym *sym = NULL;
>
Hm. Quite a bit of churn. Why not just add these three lines here:
if (insn->src_reg == BPF_PSEUDO_CALL &&
insn->imm < dd->nr_jited_ksyms)
address = dd->jited_ksyms[insn->imm];
> - sym = kernel_syms_search(dd, address);
> - if (insn->src_reg == BPF_PSEUDO_CALL)
> + if (insn->src_reg == BPF_PSEUDO_CALL) {
> + if (dd->nr_jited_ksyms) {
> + address = dd->jited_ksyms[insn->imm];
Perhaps it's paranoid, but it'd please do to bound check insn->imm
against dd->nr_jited_ksyms.
> + sym = kernel_syms_search(dd, address);
> + }
> return print_call_pcrel(dd, sym, address, insn);
> - else
> + } else {
> + address = dd->address_call_base + insn->imm;
> + sym = kernel_syms_search(dd, address);
> return print_call_helper(dd, sym, address);
> + }
> }
>
> static const char *print_imm(void *private_data,
Powered by blists - more mailing lists