[PATCH bpf] libbpf: always NULL out pobj in bpf_prog_load_xattr

Currently, code like the following segfaults if bpf_prog_load_xattr
returns an error:

    struct bpf_object *obj;

    err = bpf_prog_load_xattr(&attr, &obj, &prog_fd);
    bpf_object__close(obj);
    if (err)
        ...

Unconditionally reset pobj to NULL at the start of the function
to fix this.

Signed-off-by: Lorenz Bauer <redacted>
---
 tools/lib/bpf/libbpf.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 11a65db4b93f..2ddf3212b8f7 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c

@@ -3363,6 +3363,8 @@ int bpf_prog_load_xattr(const struct bpf_prog_load_attr *attr,
 	struct bpf_map *map;
 	int err;
 
+	*pobj = NULL;
+
 	if (!attr)
 		return -EINVAL;
 	if (!attr->file)

-- 
2.19.1

On Thu, May 02, 2019 at 11:49:32AM -0400, Lorenz Bauer wrote:

Currently, code like the following segfaults if bpf_prog_load_xattr
returns an error:

    struct bpf_object *obj;

    err = bpf_prog_load_xattr(&attr, &obj, &prog_fd);
    bpf_object__close(obj);

This is a bug.  err should always be checked first before
using obj or prog_fd.  If there is err, calling bpf_object__close(NULL)
is another redundancy.

    if (err)
        ...

Unconditionally reset pobj to NULL at the start of the function
to fix this.

Hence, this change is unnecessary.

On Thu, 2 May 2019 at 17:03, Martin Lau [off-list ref] wrote:

On Thu, May 02, 2019 at 11:49:32AM -0400, Lorenz Bauer wrote:

quoted

Currently, code like the following segfaults if bpf_prog_load_xattr
returns an error:

    struct bpf_object *obj;

    err = bpf_prog_load_xattr(&attr, &obj, &prog_fd);
    bpf_object__close(obj);

This is a bug.  err should always be checked first before
using obj or prog_fd.  If there is err, calling bpf_object__close(NULL)
is another redundancy.

Ack, I'll send another patch. The bug is in a couple of places, so I
figured that this is something people do naturally.

quoted

    if (err)
        ...

Unconditionally reset pobj to NULL at the start of the function
to fix this.

Hence, this change is unnecessary.

quoted

Signed-off-by: Lorenz Bauer <redacted>
---
 tools/lib/bpf/libbpf.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 11a65db4b93f..2ddf3212b8f7 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c

@@ -3363,6 +3363,8 @@ int bpf_prog_load_xattr(const struct bpf_prog_load_attr *attr,
      struct bpf_map *map;
      int err;

+     *pobj = NULL;
+
      if (!attr)
              return -EINVAL;
      if (!attr->file)
--

2.19.1

-- 
Lorenz Bauer  |  Systems Engineer
25 Lavington St., London SE1 0NZ

www.cloudflare.com

There are a few tests which call bpf_object__close on uninitialized
bpf_object*, which may segfault. Explicitly zero-initialise these pointers
to avoid this.

Signed-off-by: Lorenz Bauer <redacted>
---
 tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c  | 2 +-
 tools/testing/selftests/bpf/prog_tests/task_fd_query_tp.c | 2 +-
 tools/testing/selftests/bpf/prog_tests/tp_attach_query.c  | 3 +++
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c b/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
index 23b159d95c3f..b74e2f6e96d0 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c

@@ -15,7 +15,7 @@ static int libbpf_debug_print(enum libbpf_print_level level,
 static int check_load(const char *file)
 {
 	struct bpf_prog_load_attr attr;
-	struct bpf_object *obj;
+	struct bpf_object *obj = NULL;
 	int err, prog_fd;
 
 	memset(&attr, 0, sizeof(struct bpf_prog_load_attr));

diff --git a/tools/testing/selftests/bpf/prog_tests/task_fd_query_tp.c b/tools/testing/selftests/bpf/prog_tests/task_fd_query_tp.c
index d636a4f39476..f9b70e81682b 100644
--- a/tools/testing/selftests/bpf/prog_tests/task_fd_query_tp.c
+++ b/tools/testing/selftests/bpf/prog_tests/task_fd_query_tp.c

@@ -9,7 +9,7 @@ static void test_task_fd_query_tp_core(const char *probe_name,
 	struct perf_event_attr attr = {};
 	__u64 probe_offset, probe_addr;
 	__u32 len, prog_id, fd_type;
-	struct bpf_object *obj;
+	struct bpf_object *obj = NULL;
 	__u32 duration = 0;
 	char buf[256];
 

diff --git a/tools/testing/selftests/bpf/prog_tests/tp_attach_query.c b/tools/testing/selftests/bpf/prog_tests/tp_attach_query.c
index a2f476f91637..fb095e5cd9af 100644
--- a/tools/testing/selftests/bpf/prog_tests/tp_attach_query.c
+++ b/tools/testing/selftests/bpf/prog_tests/tp_attach_query.c

@@ -13,6 +13,9 @@ void test_tp_attach_query(void)
 	struct bpf_prog_info prog_info;
 	char buf[256];
 
+	for (i = 0; i < num_progs; i++)
+		obj[i] = NULL;
+
 	snprintf(buf, sizeof(buf),
 		 "/sys/kernel/debug/tracing/events/sched/sched_switch/id");
 	efd = open(buf, O_RDONLY, 0);

-- 
2.19.1

On Wed, May 08, 2019 at 05:49:32PM +0100, Lorenz Bauer wrote:

There are a few tests which call bpf_object__close on uninitialized
bpf_object*, which may segfault. Explicitly zero-initialise these pointers
to avoid this.

Acked-by: Martin KaFai Lau <redacted>

On Thu, May 9, 2019 at 8:56 AM Martin Lau [off-list ref] wrote:

On Wed, May 08, 2019 at 05:49:32PM +0100, Lorenz Bauer wrote:

quoted

There are a few tests which call bpf_object__close on uninitialized
bpf_object*, which may segfault. Explicitly zero-initialise these pointers
to avoid this.

Acked-by: Martin KaFai Lau <redacted>

Applied. Thanks