| 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: <CAEf4BzZAYjaFQEk13F5q3hXBCP0PzbsLdx9X41YnUtuviM4r7g@mail.gmail.com>
Date: Tue, 15 Mar 2022 21:33:10 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Alan Maguire <alan.maguire@...cle.com>
Cc: Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Andrii Nakryiko <andrii@...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 v4 bpf-next 1/5] libbpf: bpf_program__attach_uprobe_opts()
should determine paths for programs/libraries where possible
On Fri, Mar 11, 2022 at 4:11 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>
> ---
> tools/lib/bpf/libbpf.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 50 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 43161fd..b577577 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -10320,6 +10320,45 @@ 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)
> +{
> + char *search_paths[2];
> + int i;
> +
> + if (strstr(file, ".so")) {
> + search_paths[0] = getenv("LD_LIBRARY_PATH");
> + search_paths[1] = (char *)"/usr/lib64:/usr/lib";
> + } else {
> + search_paths[0] = getenv("PATH");
> + search_paths[1] = (char *)"/usr/bin:/usr/sbin";
It's strange you chose to cast to mutable char * instead of sticking
to `const char*`. getenv() returns char *, but that string is not
supposed to be modified, so effectively it is `const char *`. Let's
keep it safe (and it also won't require any casting)
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(search_paths); i++) {
> + char *s, *search_path, *currpath, *saveptr = NULL;
I'll nitpick on naming, and it feels like we've talked about this
before. Please stick to libbpf naming conventions: cur_path, save_ptr,
etc.
> +
> + if (!search_paths[i])
> + continue;
> + search_path = strdup(search_paths[i]);
> + s = search_path;
> + while ((currpath = strtok_r(s, ":", &saveptr)) != NULL) {
hm... I don't see any benefit to using strtok_r, which assumes mutable
input string, according to its input argument type, and thus requires
strdup, etc. Why so complicated? We have a single delimiter, ':',
right? strchr(s, ':'), advance read-only const char * pointer,
snprintf("%*s", len_of_segment, segment_start), where len_of_segment
should be a pointer difference between two ':' occurrences. No
strdup(), no strtok(), wdyt?
> + struct stat sb;
> +
> + s = NULL;
> + snprintf(result, result_sz, "%s/%s", currpath, file);
> + /* ensure it is an executable file/link */
> + if (stat(result, &sb) == 0 && (sb.st_mode & (S_IFREG | S_IFLNK)) &&
> + (sb.st_mode & (S_IXUSR | S_IXGRP | S_IXOTH))) {
wouldn't access(path, R_OK) (or maybe `R_OK | X_OK`, not sure if it's
important) do the same?
> + pr_debug("resolved '%s' to '%s'\n", file, result);
> + free(search_path);
> + return 0;
> + }
> + }
> + free(search_path);
> + }
> + return -ENOENT;
> +}
> +
> LIBBPF_API struct bpf_link *
> bpf_program__attach_uprobe_opts(const struct bpf_program *prog, pid_t pid,
> const char *binary_path, size_t func_offset,
> @@ -10327,6 +10366,7 @@ static int perf_event_uprobe_open_legacy(const char *probe_name, bool retprobe,
> {
> 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;
> @@ -10338,13 +10378,22 @@ static int perf_event_uprobe_open_legacy(const char *probe_name, bool retprobe,
> retprobe = OPTS_GET(opts, retprobe, false);
> ref_ctr_off = OPTS_GET(opts, ref_ctr_offset, 0);
> pe_opts.bpf_cookie = OPTS_GET(opts, bpf_cookie, 0);
nit: empty line to logically separate this piece of logic below?
> + if (binary_path && !strchr(binary_path, '/')) {
> + err = resolve_full_path(binary_path, full_binary_path,
> + sizeof(full_binary_path));
> + if (err) {
> + pr_warn("could not find full path for %s\n", binary_path);
consistency nit: "failed to resolve full path for '%s'\n"?
> + return libbpf_err_ptr(err);
we don't use libbpf_err*() helpers in internal helpers, this is
responsibility of user-facing API functions
> + }
> + 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
>
Powered by blists - more mailing lists