netdev - Re: [PATCH bpf-next] libbpf: fix probe code to return EPERM if encountered

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]

Date:   Fri, 1 May 2020 12:16:50 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Eelco Chaudron <echaudro@...hat.com>
Cc:     bpf <bpf@...r.kernel.org>, "David S. Miller" <davem@...emloft.net>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin Lau <kafai@...com>, Song Liu <songliubraving@...com>,
        Yonghong Song <yhs@...com>, Andrii Nakryiko <andriin@...com>,
        Toke Høiland-Jørgensen <toke@...hat.com>
Subject: Re: [PATCH bpf-next] libbpf: fix probe code to return EPERM if encountered

On Fri, May 1, 2020 at 2:56 AM Eelco Chaudron <echaudro@...hat.com> wrote:
>
>
>
> On 30 Apr 2020, at 20:12, Andrii Nakryiko wrote:
>
> > On Thu, Apr 30, 2020 at 3:24 AM Eelco Chaudron <echaudro@...hat.com>
> > wrote:
> >>
> >> When the probe code was failing for any reason ENOTSUP was returned,
> >> even
> >> if this was due to no having enough lock space. This patch fixes this
> >> by
> >> returning EPERM to the user application, so it can respond and
> >> increase
> >> the RLIMIT_MEMLOCK size.
> >>
> >> Signed-off-by: Eelco Chaudron <echaudro@...hat.com>
> >> ---
> >>  tools/lib/bpf/libbpf.c |    7 ++++++-
> >>  1 file changed, 6 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> >> index 8f480e29a6b0..a62388a151d4 100644
> >> --- a/tools/lib/bpf/libbpf.c
> >> +++ b/tools/lib/bpf/libbpf.c
> >> @@ -3381,8 +3381,13 @@ bpf_object__probe_caps(struct bpf_object *obj)
> >>
> >>         for (i = 0; i < ARRAY_SIZE(probe_fn); i++) {
> >>                 ret = probe_fn[i](obj);
> >> -               if (ret < 0)
> >> +               if (ret < 0) {
> >>                         pr_debug("Probe #%d failed with %d.\n", i,
> >> ret);
> >> +                       if (ret == -EPERM) {
> >> +                               pr_perm_msg(ret);
> >> +                               return ret;
> >
> > I think this is dangerous to do. This detection loop is not supposed
> > to return error to user if any of the features are missing. I'd feel
> > more comfortable if we split bpf_object__probe_name() into two tests:
> > one testing trivial program and another testing same program with
> > name. If the first one fails with EPERM -- then we can return error to
> > user. If anything else fails -- that's ok. Thoughts?
>
> Before sending the patch I briefly checked the existing probes and did
> not see any other code path that could lead to EPERM. But you are right
> that this might not be the case for previous kernels. So you suggest
> something like this?

It both previous as well as future kernel version. We can never be
100% sure. While the idea of probe_caps() is to detect optional
features.

>
> diff --git a/src/libbpf.c b/src/libbpf.c
> index ff91742..fd5fdee 100644
> --- a/src/libbpf.c
> +++ b/src/libbpf.c
> @@ -3130,7 +3130,7 @@ int bpf_map__resize(struct bpf_map *map, __u32
> max_entries)
>   }
>
>   static int
> -bpf_object__probe_name(struct bpf_object *obj)
> +bpf_object__probe_loading(struct bpf_object *obj)
>   {
>          struct bpf_load_program_attr attr;
>          char *cp, errmsg[STRERR_BUFSIZE];
> @@ -3157,8 +3157,26 @@ bpf_object__probe_name(struct bpf_object *obj)
>          }
>          close(ret);
>
> -       /* now try the same program, but with the name */
> +       return 0;
> +}
>
> +static int
> +bpf_object__probe_name(struct bpf_object *obj)
> +{
> +       struct bpf_load_program_attr attr;
> +       struct bpf_insn insns[] = {
> +               BPF_MOV64_IMM(BPF_REG_0, 0),
> +               BPF_EXIT_INSN(),
> +       };
> +       int ret;
> +
> +       /* make sure loading with name works */
> +
> +       memset(&attr, 0, sizeof(attr));
> +       attr.prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
> +       attr.insns = insns;
> +       attr.insns_cnt = ARRAY_SIZE(insns);
> +       attr.license = "GPL";
>          attr.name = "test";
>          ret = bpf_load_program_xattr(&attr, NULL, 0);
>          if (ret >= 0) {
> @@ -3328,6 +3346,11 @@ bpf_object__probe_caps(struct bpf_object *obj)
>          };
>          int i, ret;
>
> +       if (bpf_object__probe_loading(obj) == -EPERM) {
> +               pr_perm_msg(-EPERM);
> +               return -EPERM;
> +       }
> +
>          for (i = 0; i < ARRAY_SIZE(probe_fn); i++) {
>                  ret = probe_fn[i](obj);
>                  if (ret < 0)
>
> Let me know, and I sent out a v2.

Yes, that's the split I had in mind, but I'd move
bpf_object__probe_loading() call directly into bpf_object__load() to
be the first thing to check. probe_caps() should still be non-failing
if any feature is missing. Does it make sense?

>
> Cheers,
>
> Eelco
>