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  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]
Date:   Thu, 10 Sep 2020 12:40:53 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Stanislav Fomichev <sdf@...gle.com>
Cc:     Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        YiFei Zhu <zhuyifei1999@...il.com>
Subject: Re: [PATCH bpf-next v4 3/5] libbpf: Add BPF_PROG_BIND_MAP syscall and
 use it on .metadata section

On Wed, Sep 9, 2020 at 11:25 AM Stanislav Fomichev <sdf@...gle.com> wrote:
>
> From: YiFei Zhu <zhuyifei@...gle.com>
>
> The patch adds a simple wrapper bpf_prog_bind_map around the syscall.
> When the libbpf tries to load a program, it will probe the kernel for
> the support of this syscall and unconditionally bind .rodata section

btw, you subject is out of sync, still mentions .metadata

> to the program.
>
> Cc: YiFei Zhu <zhuyifei1999@...il.com>
> Signed-off-by: YiFei Zhu <zhuyifei@...gle.com>

Please drop zhuyifei@...gle.com from CC list (it's unreachable), when
you submit a new version.

> Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
> ---
>  tools/lib/bpf/bpf.c      | 13 ++++++
>  tools/lib/bpf/bpf.h      |  8 ++++
>  tools/lib/bpf/libbpf.c   | 94 ++++++++++++++++++++++++++++++++--------
>  tools/lib/bpf/libbpf.map |  1 +
>  4 files changed, 98 insertions(+), 18 deletions(-)
>
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index 82b983ff6569..5f6c5676cc45 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -872,3 +872,16 @@ int bpf_enable_stats(enum bpf_stats_type type)
>
>         return sys_bpf(BPF_ENABLE_STATS, &attr, sizeof(attr));
>  }
> +
> +int bpf_prog_bind_map(int prog_fd, int map_fd,
> +                     const struct bpf_prog_bind_opts *opts)
> +{
> +       union bpf_attr attr;
> +

you forgot OPTS_VALID check here

> +       memset(&attr, 0, sizeof(attr));
> +       attr.prog_bind_map.prog_fd = prog_fd;
> +       attr.prog_bind_map.map_fd = map_fd;
> +       attr.prog_bind_map.flags = OPTS_GET(opts, flags, 0);
> +
> +       return sys_bpf(BPF_PROG_BIND_MAP, &attr, sizeof(attr));
> +}

[...]

> -static int probe_kern_global_data(void)
> +static void probe_create_global_data(int *prog, int *map,
> +                                    struct bpf_insn *insns, size_t insns_cnt)
>  {
>         struct bpf_load_program_attr prg_attr;
>         struct bpf_create_map_attr map_attr;
>         char *cp, errmsg[STRERR_BUFSIZE];
> -       struct bpf_insn insns[] = {
> -               BPF_LD_MAP_VALUE(BPF_REG_1, 0, 16),
> -               BPF_ST_MEM(BPF_DW, BPF_REG_1, 0, 42),
> -               BPF_MOV64_IMM(BPF_REG_0, 0),
> -               BPF_EXIT_INSN(),
> -       };
> -       int ret, map;
> +       int err;
>
>         memset(&map_attr, 0, sizeof(map_attr));
>         map_attr.map_type = BPF_MAP_TYPE_ARRAY;
> @@ -3748,26 +3749,40 @@ static int probe_kern_global_data(void)
>         map_attr.value_size = 32;
>         map_attr.max_entries = 1;
>
> -       map = bpf_create_map_xattr(&map_attr);
> -       if (map < 0) {
> -               ret = -errno;
> -               cp = libbpf_strerror_r(ret, errmsg, sizeof(errmsg));
> +       *map = bpf_create_map_xattr(&map_attr);
> +       if (*map < 0) {
> +               err = errno;
> +               cp = libbpf_strerror_r(err, errmsg, sizeof(errmsg));
>                 pr_warn("Error in %s():%s(%d). Couldn't create simple array map.\n",
> -                       __func__, cp, -ret);
> -               return ret;
> +                       __func__, cp, -err);
> +               return;
>         }
>
> -       insns[0].imm = map;
> +       insns[0].imm = *map;

I think I already complained about this? You are assuming that
insns[0] is BPF_LD_MAP_VALUE, which is true only for one case out of
two already! It's just by luck that probe_prog_bind_map works because
the verifier ignores the exit code, apparently.

If this doesn't generalize well, don't generalize. But let's not do a
blind instruction rewrite, which will cause tons of confusion later.

>
>         memset(&prg_attr, 0, sizeof(prg_attr));
>         prg_attr.prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
>         prg_attr.insns = insns;
> -       prg_attr.insns_cnt = ARRAY_SIZE(insns);
> +       prg_attr.insns_cnt = insns_cnt;
>         prg_attr.license = "GPL";
>
> -       ret = bpf_load_program_xattr(&prg_attr, NULL, 0);
> +       *prog = bpf_load_program_xattr(&prg_attr, NULL, 0);
> +}
> +
> +static int probe_kern_global_data(void)
> +{
> +       struct bpf_insn insns[] = {
> +               BPF_LD_MAP_VALUE(BPF_REG_1, 0, 16),
> +               BPF_ST_MEM(BPF_DW, BPF_REG_1, 0, 42),
> +               BPF_MOV64_IMM(BPF_REG_0, 0),
> +               BPF_EXIT_INSN(),
> +       };
> +       int prog = -1, map = -1;
> +
> +       probe_create_global_data(&prog, &map, insns, ARRAY_SIZE(insns));
> +
>         close(map);
> -       return probe_fd(ret);
> +       return probe_fd(prog);
>  }
>
>  static int probe_kern_btf(void)
> @@ -3894,6 +3909,32 @@ static int probe_kern_probe_read_kernel(void)
>         return probe_fd(bpf_load_program_xattr(&attr, NULL, 0));
>  }
>
> +static int probe_prog_bind_map(void)
> +{
> +       struct bpf_insn insns[] = {
> +               BPF_MOV64_IMM(BPF_REG_0, 0),
> +               BPF_EXIT_INSN(),
> +       };
> +       int prog = -1, map = -1, ret = 0;
> +
> +       if (!kernel_supports(FEAT_GLOBAL_DATA))
> +               return 0;
> +
> +       probe_create_global_data(&prog, &map, insns, ARRAY_SIZE(insns));
> +
> +       if (map >= 0 && prog < 0) {
> +               close(map);
> +               return 0;
> +       }
> +
> +       if (!bpf_prog_bind_map(prog, map, NULL))
> +               ret = 1;
> +
> +       close(map);
> +       close(prog);
> +       return ret;
> +}
> +

[...]

> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 92ceb48a5ca2..0b7830f4ff8b 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -308,4 +308,5 @@ LIBBPF_0.2.0 {
>                 perf_buffer__epoll_fd;
>                 perf_buffer__consume_buffer;
>                 xsk_socket__create_shared;
> +               bpf_prog_bind_map;

please keep this list sorted

>  } LIBBPF_0.1.0;
> --
> 2.28.0.526.ge36021eeef-goog
>

Powered by blists - more mailing lists