netdev - Re: [PATCH v5 bpf-next 1/5] libbpf: bpf_program__attach_uprobe_opts() should determine paths for programs/libraries where possible

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:   Sun, 3 Apr 2022 18:14:50 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alan Maguire <alan.maguire@...cle.com>
Cc:     Andrii Nakryiko <andrii@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <ast@...nel.org>, Martin Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        john fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        Yucong Sun <sunyucong@...il.com>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH v5 bpf-next 1/5] libbpf: bpf_program__attach_uprobe_opts()
 should determine paths for programs/libraries where possible

On Wed, Mar 30, 2022 at 8:27 AM Alan Maguire <alan.maguire@...cle.com> wrote:
>
> bpf_program__attach_uprobe_opts() requires a binary_path argument
> specifying binary to instrument.  Supporting simply specifying
> "libc.so.6" or "foo" should be possible too.
>
> Library search checks LD_LIBRARY_PATH, then /usr/lib64, /usr/lib.
> This allows users to run BPF programs prefixed with
> LD_LIBRARY_PATH=/path2/lib while still searching standard locations.
> Similarly for non .so files, we check PATH and /usr/bin, /usr/sbin.
>
> Path determination will be useful for auto-attach of BPF uprobe programs
> using SEC() definition.
>
> Signed-off-by: Alan Maguire <alan.maguire@...cle.com>
> ---

This patch's subject was very long, I took the liberty to shorten it as:

libbpf: auto-resolve programs/libraries when necessary for uprobes

Please yell if you strongly object.

>  tools/lib/bpf/libbpf.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 52 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 809fe20..a7d5954 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -10517,6 +10517,46 @@ static int perf_event_uprobe_open_legacy(const char *probe_name, bool retprobe,
>         return pfd;
>  }
>
> +/* Get full path to program/shared library. */
> +static int resolve_full_path(const char *file, char *result, size_t result_sz)
> +{
> +       const char *search_paths[2];
> +       int i;
> +
> +       if (strstr(file, ".so")) {

this check can be too brittle. It will think that binary called
"i.am.sorry" will be re assumed a shared library. While you can always
subvert this logic, maybe checking for ".so" suffix (not a substring)
and, if no such suffix found, looking for ".so." substring as a second
check? "i.am.so.sorry" still would fool it, but that looks like a very
artificial stretch.

let's improve this in a follow up

> +               search_paths[0] = getenv("LD_LIBRARY_PATH");
> +               search_paths[1] = "/usr/lib64:/usr/lib";

[...]

>  {
>         DECLARE_LIBBPF_OPTS(bpf_perf_event_opts, pe_opts);
>         char errmsg[STRERR_BUFSIZE], *legacy_probe = NULL;
> +       char full_binary_path[PATH_MAX];
>         struct bpf_link *link;
>         size_t ref_ctr_off;
>         int pfd, err;
> @@ -10536,12 +10577,22 @@ static int perf_event_uprobe_open_legacy(const char *probe_name, bool retprobe,
>         ref_ctr_off = OPTS_GET(opts, ref_ctr_offset, 0);
>         pe_opts.bpf_cookie = OPTS_GET(opts, bpf_cookie, 0);
>
> +       if (binary_path && !strchr(binary_path, '/')) {
> +               err = resolve_full_path(binary_path, full_binary_path,
> +                                       sizeof(full_binary_path));
> +               if (err) {
> +                       pr_warn("failed to resolve full path for '%s'\n", binary_path);

we use "prog '%s': " prefix for error messages within
bpf_program__attach_xxx() wherever possible. Fixed up while applying.





> +                       return libbpf_err_ptr(err);
> +               }
> +               binary_path = full_binary_path;
> +       }
> +
>         legacy = determine_uprobe_perf_type() < 0;
>         if (!legacy) {
>                 pfd = perf_event_open_probe(true /* uprobe */, retprobe, binary_path,
>                                             func_offset, pid, ref_ctr_off);
>         } else {
> -               char probe_name[512];
> +               char probe_name[PATH_MAX + 64];
>
>                 if (ref_ctr_off)
>                         return libbpf_err_ptr(-EINVAL);
> --
> 1.8.3.1
>