diff mbox series

[bpf-next] libbpf: fix bpf_object__open_skeleton()'s mishandling of options

Message ID 20240827203721.1145494-1-andrii@kernel.org (mailing list archive)
State Accepted
Commit c634d6f4e12d00c954410ba11db45799a8c77b5b
Delegated to: BPF
Headers show
Series [bpf-next] libbpf: fix bpf_object__open_skeleton()'s mishandling of options | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 7 this patch: 7
netdev/build_tools success Errors and warnings before: 4 this patch: 4
netdev/cc_maintainers fail 1 blamed authors not CCed: martin.lau@linux.dev; 9 maintainers not CCed: sdf@fomichev.me eddyz87@gmail.com haoluo@google.com jolsa@kernel.org song@kernel.org yonghong.song@linux.dev kpsingh@kernel.org martin.lau@linux.dev john.fastabend@gmail.com
netdev/build_clang success Errors and warnings before: 7 this patch: 7
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 7 this patch: 7
netdev/checkpatch warning WARNING: line length of 83 exceeds 80 columns WARNING: line length of 86 exceeds 80 columns WARNING: line length of 92 exceeds 80 columns WARNING: line length of 93 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-13 fail Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-17 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-18 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-14 fail Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 fail Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-PR fail PR summary
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18

Commit Message

Andrii Nakryiko Aug. 27, 2024, 8:37 p.m. UTC 
We do an ugly copying of options in bpf_object__open_skeleton() just to
be able to set object name from skeleton's recorded name (while still
allowing user to override it through opts->object_name).

This is not just ugly, but it also is broken due to memcpy() that
doesn't take into account potential skel_opts' and user-provided opts'
sizes differences due to backward and forward compatibility. This leads
to copying over extra bytes and then failing to validate options
properly. It could, technically, lead also to SIGSEGV, if we are unlucky.

So just get rid of that memory copy completely and instead pass
default object name into bpf_object_open() directly, simplifying all
this significantly. The rule now is that obj_name should be non-NULL for
bpf_object_open() when called with in-memory buffer, so validate that
explicitly as well.

We adopt bpf_object__open_mem() to this as well and generate default
name (based on buffer memory address and size) outside of bpf_object_open().

Fixes: d66562fba1ce ("libbpf: Add BPF object skeleton support")
Reported-by: Daniel Müller <deso@posteo.net>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/lib/bpf/libbpf.c | 52 +++++++++++++++---------------------------
 1 file changed, 19 insertions(+), 33 deletions(-)

Comments

Daniel Müller Aug. 28, 2024, 5:28 p.m. UTC | #1 
On Tue, Aug 27, 2024 at 01:37:21PM GMT, Andrii Nakryiko wrote:
> We do an ugly copying of options in bpf_object__open_skeleton() just to
> be able to set object name from skeleton's recorded name (while still
> allowing user to override it through opts->object_name).
> 
> This is not just ugly, but it also is broken due to memcpy() that
> doesn't take into account potential skel_opts' and user-provided opts'
> sizes differences due to backward and forward compatibility. This leads
> to copying over extra bytes and then failing to validate options
> properly. It could, technically, lead also to SIGSEGV, if we are unlucky.
> 
> So just get rid of that memory copy completely and instead pass
> default object name into bpf_object_open() directly, simplifying all
> this significantly. The rule now is that obj_name should be non-NULL for
> bpf_object_open() when called with in-memory buffer, so validate that
> explicitly as well.
> 
> We adopt bpf_object__open_mem() to this as well and generate default
> name (based on buffer memory address and size) outside of bpf_object_open().
> 
> Fixes: d66562fba1ce ("libbpf: Add BPF object skeleton support")
> Reported-by: Daniel Müller <deso@posteo.net>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>  tools/lib/bpf/libbpf.c | 52 +++++++++++++++---------------------------
>  1 file changed, 19 insertions(+), 33 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index e55353887439..d3a542649e6b 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -13761,29 +13763,13 @@ static int populate_skeleton_progs(const struct bpf_object *obj,
>  int bpf_object__open_skeleton(struct bpf_object_skeleton *s,
>  			      const struct bpf_object_open_opts *opts)
>  {
> -	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, skel_opts,
> -		.object_name = s->name,
> -	);
>  	struct bpf_object *obj;
>  	int err;
>  
> -	/* Attempt to preserve opts->object_name, unless overriden by user
> -	 * explicitly. Overwriting object name for skeletons is discouraged,
> -	 * as it breaks global data maps, because they contain object name
> -	 * prefix as their own map name prefix. When skeleton is generated,
> -	 * bpftool is making an assumption that this name will stay the same.
> -	 */
> -	if (opts) {
> -		memcpy(&skel_opts, opts, sizeof(*opts));
> -		if (!opts->object_name)
> -			skel_opts.object_name = s->name;
> -	}
> -
> -	obj = bpf_object__open_mem(s->data, s->data_sz, &skel_opts);
> -	err = libbpf_get_error(obj);
> -	if (err) {
> -		pr_warn("failed to initialize skeleton BPF object '%s': %d\n",
> -			s->name, err);
> +	obj = bpf_object_open(NULL, s->data, s->data_sz, s->name, opts);
> +	if (IS_ERR(obj)) {
> +		err = PTR_ERR(obj);
> +		pr_warn("failed to initialize skeleton BPF object '%s': %d\n", s->name, err);

Ideally we'd do the same dance here for the name that we do in
bpf_object_open, right? Otherwise the warning may be mildly confusing if
  > pr_debug("loading object '%s' from buffer\n", obj_name)
earlier refers to a potentially different name?

Seems minor, though. Thanks for the fix.

Reviewed-by: Daniel Müller <deso@posteo.net>
Andrii Nakryiko Aug. 28, 2024, 6:11 p.m. UTC | #2 
On Wed, Aug 28, 2024 at 10:29 AM Daniel Müller <deso@posteo.net> wrote:
>
> On Tue, Aug 27, 2024 at 01:37:21PM GMT, Andrii Nakryiko wrote:
> > We do an ugly copying of options in bpf_object__open_skeleton() just to
> > be able to set object name from skeleton's recorded name (while still
> > allowing user to override it through opts->object_name).
> >
> > This is not just ugly, but it also is broken due to memcpy() that
> > doesn't take into account potential skel_opts' and user-provided opts'
> > sizes differences due to backward and forward compatibility. This leads
> > to copying over extra bytes and then failing to validate options
> > properly. It could, technically, lead also to SIGSEGV, if we are unlucky.
> >
> > So just get rid of that memory copy completely and instead pass
> > default object name into bpf_object_open() directly, simplifying all
> > this significantly. The rule now is that obj_name should be non-NULL for
> > bpf_object_open() when called with in-memory buffer, so validate that
> > explicitly as well.
> >
> > We adopt bpf_object__open_mem() to this as well and generate default
> > name (based on buffer memory address and size) outside of bpf_object_open().
> >
> > Fixes: d66562fba1ce ("libbpf: Add BPF object skeleton support")
> > Reported-by: Daniel Müller <deso@posteo.net>
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
> >  tools/lib/bpf/libbpf.c | 52 +++++++++++++++---------------------------
> >  1 file changed, 19 insertions(+), 33 deletions(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index e55353887439..d3a542649e6b 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -13761,29 +13763,13 @@ static int populate_skeleton_progs(const struct bpf_object *obj,
> >  int bpf_object__open_skeleton(struct bpf_object_skeleton *s,
> >                             const struct bpf_object_open_opts *opts)
> >  {
> > -     DECLARE_LIBBPF_OPTS(bpf_object_open_opts, skel_opts,
> > -             .object_name = s->name,
> > -     );
> >       struct bpf_object *obj;
> >       int err;
> >
> > -     /* Attempt to preserve opts->object_name, unless overriden by user
> > -      * explicitly. Overwriting object name for skeletons is discouraged,
> > -      * as it breaks global data maps, because they contain object name
> > -      * prefix as their own map name prefix. When skeleton is generated,
> > -      * bpftool is making an assumption that this name will stay the same.
> > -      */
> > -     if (opts) {
> > -             memcpy(&skel_opts, opts, sizeof(*opts));
> > -             if (!opts->object_name)
> > -                     skel_opts.object_name = s->name;
> > -     }
> > -
> > -     obj = bpf_object__open_mem(s->data, s->data_sz, &skel_opts);
> > -     err = libbpf_get_error(obj);
> > -     if (err) {
> > -             pr_warn("failed to initialize skeleton BPF object '%s': %d\n",
> > -                     s->name, err);
> > +     obj = bpf_object_open(NULL, s->data, s->data_sz, s->name, opts);
> > +     if (IS_ERR(obj)) {
> > +             err = PTR_ERR(obj);
> > +             pr_warn("failed to initialize skeleton BPF object '%s': %d\n", s->name, err);
>
> Ideally we'd do the same dance here for the name that we do in
> bpf_object_open, right? Otherwise the warning may be mildly confusing if
>   > pr_debug("loading object '%s' from buffer\n", obj_name)
> earlier refers to a potentially different name?

Yeah, true, but I'm not really happy to add this "name resolution"
duplication of logic here, tbh. Also validation of options, etc. Let's
keep it as is, it's very unlikely someone will be overriding the
object name.

>
> Seems minor, though. Thanks for the fix.
>
> Reviewed-by: Daniel Müller <deso@posteo.net>
Eduard Zingerman Aug. 28, 2024, 8:42 p.m. UTC | #3 
On Tue, 2024-08-27 at 13:37 -0700, Andrii Nakryiko wrote:
> We do an ugly copying of options in bpf_object__open_skeleton() just to
> be able to set object name from skeleton's recorded name (while still
> allowing user to override it through opts->object_name).
> 
> This is not just ugly, but it also is broken due to memcpy() that
> doesn't take into account potential skel_opts' and user-provided opts'
> sizes differences due to backward and forward compatibility. This leads
> to copying over extra bytes and then failing to validate options
> properly. It could, technically, lead also to SIGSEGV, if we are unlucky.
> 
> So just get rid of that memory copy completely and instead pass
> default object name into bpf_object_open() directly, simplifying all
> this significantly. The rule now is that obj_name should be non-NULL for
> bpf_object_open() when called with in-memory buffer, so validate that
> explicitly as well.
> 
> We adopt bpf_object__open_mem() to this as well and generate default
> name (based on buffer memory address and size) outside of bpf_object_open().
> 
> Fixes: d66562fba1ce ("libbpf: Add BPF object skeleton support")
> Reported-by: Daniel Müller <deso@posteo.net>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

[...]
patchwork-bot+netdevbpf@kernel.org Aug. 29, 2024, 3:50 p.m. UTC | #4 
Hello:

This patch was applied to bpf/bpf-next.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Tue, 27 Aug 2024 13:37:21 -0700 you wrote:
> We do an ugly copying of options in bpf_object__open_skeleton() just to
> be able to set object name from skeleton's recorded name (while still
> allowing user to override it through opts->object_name).
> 
> This is not just ugly, but it also is broken due to memcpy() that
> doesn't take into account potential skel_opts' and user-provided opts'
> sizes differences due to backward and forward compatibility. This leads
> to copying over extra bytes and then failing to validate options
> properly. It could, technically, lead also to SIGSEGV, if we are unlucky.
> 
> [...]

Here is the summary with links:
  - [bpf-next] libbpf: fix bpf_object__open_skeleton()'s mishandling of options
    https://git.kernel.org/bpf/bpf-next/c/c634d6f4e12d

You are awesome, thank you!
diff mbox series

Patch

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index e55353887439..d3a542649e6b 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -7905,16 +7905,19 @@  static int bpf_object_init_progs(struct bpf_object *obj, const struct bpf_object
 }
 
 static struct bpf_object *bpf_object_open(const char *path, const void *obj_buf, size_t obj_buf_sz,
+					  const char *obj_name,
 					  const struct bpf_object_open_opts *opts)
 {
-	const char *obj_name, *kconfig, *btf_tmp_path, *token_path;
+	const char *kconfig, *btf_tmp_path, *token_path;
 	struct bpf_object *obj;
-	char tmp_name[64];
 	int err;
 	char *log_buf;
 	size_t log_size;
 	__u32 log_level;
 
+	if (obj_buf && !obj_name)
+		return ERR_PTR(-EINVAL);
+
 	if (elf_version(EV_CURRENT) == EV_NONE) {
 		pr_warn("failed to init libelf for %s\n",
 			path ? : "(mem buf)");
@@ -7924,16 +7927,12 @@  static struct bpf_object *bpf_object_open(const char *path, const void *obj_buf,
 	if (!OPTS_VALID(opts, bpf_object_open_opts))
 		return ERR_PTR(-EINVAL);
 
-	obj_name = OPTS_GET(opts, object_name, NULL);
+	obj_name = OPTS_GET(opts, object_name, NULL) ?: obj_name;
 	if (obj_buf) {
-		if (!obj_name) {
-			snprintf(tmp_name, sizeof(tmp_name), "%lx-%lx",
-				 (unsigned long)obj_buf,
-				 (unsigned long)obj_buf_sz);
-			obj_name = tmp_name;
-		}
 		path = obj_name;
 		pr_debug("loading object '%s' from buffer\n", obj_name);
+	} else {
+		pr_debug("loading object from %s\n", path);
 	}
 
 	log_buf = OPTS_GET(opts, kernel_log_buf, NULL);
@@ -8017,9 +8016,7 @@  bpf_object__open_file(const char *path, const struct bpf_object_open_opts *opts)
 	if (!path)
 		return libbpf_err_ptr(-EINVAL);
 
-	pr_debug("loading %s\n", path);
-
-	return libbpf_ptr(bpf_object_open(path, NULL, 0, opts));
+	return libbpf_ptr(bpf_object_open(path, NULL, 0, NULL, opts));
 }
 
 struct bpf_object *bpf_object__open(const char *path)
@@ -8031,10 +8028,15 @@  struct bpf_object *
 bpf_object__open_mem(const void *obj_buf, size_t obj_buf_sz,
 		     const struct bpf_object_open_opts *opts)
 {
+	char tmp_name[64];
+
 	if (!obj_buf || obj_buf_sz == 0)
 		return libbpf_err_ptr(-EINVAL);
 
-	return libbpf_ptr(bpf_object_open(NULL, obj_buf, obj_buf_sz, opts));
+	/* create a (quite useless) default "name" for this memory buffer object */
+	snprintf(tmp_name, sizeof(tmp_name), "%lx-%zx", (unsigned long)obj_buf, obj_buf_sz);
+
+	return libbpf_ptr(bpf_object_open(NULL, obj_buf, obj_buf_sz, tmp_name, opts));
 }
 
 static int bpf_object_unload(struct bpf_object *obj)
@@ -13761,29 +13763,13 @@  static int populate_skeleton_progs(const struct bpf_object *obj,
 int bpf_object__open_skeleton(struct bpf_object_skeleton *s,
 			      const struct bpf_object_open_opts *opts)
 {
-	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, skel_opts,
-		.object_name = s->name,
-	);
 	struct bpf_object *obj;
 	int err;
 
-	/* Attempt to preserve opts->object_name, unless overriden by user
-	 * explicitly. Overwriting object name for skeletons is discouraged,
-	 * as it breaks global data maps, because they contain object name
-	 * prefix as their own map name prefix. When skeleton is generated,
-	 * bpftool is making an assumption that this name will stay the same.
-	 */
-	if (opts) {
-		memcpy(&skel_opts, opts, sizeof(*opts));
-		if (!opts->object_name)
-			skel_opts.object_name = s->name;
-	}
-
-	obj = bpf_object__open_mem(s->data, s->data_sz, &skel_opts);
-	err = libbpf_get_error(obj);
-	if (err) {
-		pr_warn("failed to initialize skeleton BPF object '%s': %d\n",
-			s->name, err);
+	obj = bpf_object_open(NULL, s->data, s->data_sz, s->name, opts);
+	if (IS_ERR(obj)) {
+		err = PTR_ERR(obj);
+		pr_warn("failed to initialize skeleton BPF object '%s': %d\n", s->name, err);
 		return libbpf_err(err);
 	}