[PATCH bpf v3 0/6] Fix bugs found by ASAN when running selftests

[PATCH bpf v3 0/6] Fix bugs found by ASAN when running selftests

[Xu Kuohai]

v3:
- Fix error failure of case test_xdp_adjust_tail_grow exposed by this series

v2: https://lore.kernel.org/bpf/20221010070454.577433-1-xukuohai@huaweicloud.com
- Rebase and fix conflict

v1: https://lore.kernel.org/bpf/20221009131830.395569-1-xukuohai@huaweicloud.com

Xu Kuohai (6):
  libbpf: Fix use-after-free in btf_dump_name_dups
  libbpf: Fix memory leak in parse_usdt_arg()
  selftests/bpf: Fix memory leak caused by not destroying skeleton
  selftest/bpf: Fix memory leak in kprobe_multi_test
  selftests/bpf: Fix error failure of case test_xdp_adjust_tail_grow
  selftest/bpf: Fix error usage of ASSERT_OK in xdp_adjust_tail.c

 tools/lib/bpf/btf_dump.c                      | 47 +++++++++++----
 tools/lib/bpf/usdt.c                          | 59 +++++++++++--------
 .../bpf/prog_tests/kprobe_multi_test.c        | 17 +++---
 .../selftests/bpf/prog_tests/map_kptr.c       |  3 +-
 .../selftests/bpf/prog_tests/tracing_struct.c |  3 +-
 .../bpf/prog_tests/xdp_adjust_tail.c          |  7 ++-
 6 files changed, 86 insertions(+), 50 deletions(-)

-- 
2.30.2

[PATCH bpf v3 1/6] libbpf: Fix use-after-free in btf_dump_name_dups

[Xu Kuohai]

ASAN reports an use-after-free in btf_dump_name_dups:

ERROR: AddressSanitizer: heap-use-after-free on address 0xffff927006db at pc 0xaaaab5dfb618 bp 0xffffdd89b890 sp 0xffffdd89b928
READ of size 2 at 0xffff927006db thread T0
    #0 0xaaaab5dfb614 in __interceptor_strcmp.part.0 (test_progs+0x21b614)
    #1 0xaaaab635f144 in str_equal_fn tools/lib/bpf/btf_dump.c:127
    #2 0xaaaab635e3e0 in hashmap_find_entry tools/lib/bpf/hashmap.c:143
    #3 0xaaaab635e72c in hashmap__find tools/lib/bpf/hashmap.c:212
    #4 0xaaaab6362258 in btf_dump_name_dups tools/lib/bpf/btf_dump.c:1525
    #5 0xaaaab636240c in btf_dump_resolve_name tools/lib/bpf/btf_dump.c:1552
    #6 0xaaaab6362598 in btf_dump_type_name tools/lib/bpf/btf_dump.c:1567
    #7 0xaaaab6360b48 in btf_dump_emit_struct_def tools/lib/bpf/btf_dump.c:912
    #8 0xaaaab6360630 in btf_dump_emit_type tools/lib/bpf/btf_dump.c:798
    #9 0xaaaab635f720 in btf_dump__dump_type tools/lib/bpf/btf_dump.c:282
    #10 0xaaaab608523c in test_btf_dump_incremental tools/testing/selftests/bpf/prog_tests/btf_dump.c:236
    #11 0xaaaab6097530 in test_btf_dump tools/testing/selftests/bpf/prog_tests/btf_dump.c:875
    #12 0xaaaab6314ed0 in run_one_test tools/testing/selftests/bpf/test_progs.c:1062
    #13 0xaaaab631a0a8 in main tools/testing/selftests/bpf/test_progs.c:1697
    #14 0xffff9676d214 in __libc_start_main ../csu/libc-start.c:308
    #15 0xaaaab5d65990  (test_progs+0x185990)

0xffff927006db is located 11 bytes inside of 16-byte region [0xffff927006d0,0xffff927006e0)
freed by thread T0 here:
    #0 0xaaaab5e2c7c4 in realloc (test_progs+0x24c7c4)
    #1 0xaaaab634f4a0 in libbpf_reallocarray tools/lib/bpf/libbpf_internal.h:191
    #2 0xaaaab634f840 in libbpf_add_mem tools/lib/bpf/btf.c:163
    #3 0xaaaab636643c in strset_add_str_mem tools/lib/bpf/strset.c:106
    #4 0xaaaab6366560 in strset__add_str tools/lib/bpf/strset.c:157
    #5 0xaaaab6352d70 in btf__add_str tools/lib/bpf/btf.c:1519
    #6 0xaaaab6353e10 in btf__add_field tools/lib/bpf/btf.c:2032
    #7 0xaaaab6084fcc in test_btf_dump_incremental tools/testing/selftests/bpf/prog_tests/btf_dump.c:232
    #8 0xaaaab6097530 in test_btf_dump tools/testing/selftests/bpf/prog_tests/btf_dump.c:875
    #9 0xaaaab6314ed0 in run_one_test tools/testing/selftests/bpf/test_progs.c:1062
    #10 0xaaaab631a0a8 in main tools/testing/selftests/bpf/test_progs.c:1697
    #11 0xffff9676d214 in __libc_start_main ../csu/libc-start.c:308
    #12 0xaaaab5d65990  (test_progs+0x185990)

previously allocated by thread T0 here:
    #0 0xaaaab5e2c7c4 in realloc (test_progs+0x24c7c4)
    #1 0xaaaab634f4a0 in libbpf_reallocarray tools/lib/bpf/libbpf_internal.h:191
    #2 0xaaaab634f840 in libbpf_add_mem tools/lib/bpf/btf.c:163
    #3 0xaaaab636643c in strset_add_str_mem tools/lib/bpf/strset.c:106
    #4 0xaaaab6366560 in strset__add_str tools/lib/bpf/strset.c:157
    #5 0xaaaab6352d70 in btf__add_str tools/lib/bpf/btf.c:1519
    #6 0xaaaab6353ff0 in btf_add_enum_common tools/lib/bpf/btf.c:2070
    #7 0xaaaab6354080 in btf__add_enum tools/lib/bpf/btf.c:2102
    #8 0xaaaab6082f50 in test_btf_dump_incremental tools/testing/selftests/bpf/prog_tests/btf_dump.c:162
    #9 0xaaaab6097530 in test_btf_dump tools/testing/selftests/bpf/prog_tests/btf_dump.c:875
    #10 0xaaaab6314ed0 in run_one_test tools/testing/selftests/bpf/test_progs.c:1062
    #11 0xaaaab631a0a8 in main tools/testing/selftests/bpf/test_progs.c:1697
    #12 0xffff9676d214 in __libc_start_main ../csu/libc-start.c:308
    #13 0xaaaab5d65990  (test_progs+0x185990)

The reason is that the key stored in hash table name_map is a string
address, and the string memory is allocated by realloc() function, when
the memory is resized by realloc() later, the old memory may be freed,
so the address stored in name_map references to a freed memory, causing
use-after-free.

Fix it by storing duplicated string address in name_map.

Fixes: 351131b51c7a ("libbpf: add btf_dump API for BTF-to-C conversion")
Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
---
 tools/lib/bpf/btf_dump.c | 47 +++++++++++++++++++++++++++++++---------
 1 file changed, 37 insertions(+), 10 deletions(-)

diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
index e4da6de68d8f..8365d801cbd0 100644
--- a/tools/lib/bpf/btf_dump.c
+++ b/tools/lib/bpf/btf_dump.c
@@ -219,6 +219,17 @@ static int btf_dump_resize(struct btf_dump *d)
 	return 0;
 }
 
+static void btf_dump_free_names(struct hashmap *map)
+{
+	size_t bkt;
+	struct hashmap_entry *cur;
+
+	hashmap__for_each_entry(map, cur, bkt)
+		free((void *)cur->key);
+
+	hashmap__free(map);
+}
+
 void btf_dump__free(struct btf_dump *d)
 {
 	int i;
@@ -237,8 +248,8 @@ void btf_dump__free(struct btf_dump *d)
 	free(d->cached_names);
 	free(d->emit_queue);
 	free(d->decl_stack);
-	hashmap__free(d->type_names);
-	hashmap__free(d->ident_names);
+	btf_dump_free_names(d->type_names);
+	btf_dump_free_names(d->ident_names);
 
 	free(d);
 }
@@ -634,8 +645,8 @@ static void btf_dump_emit_type_chain(struct btf_dump *d,
 
 static const char *btf_dump_type_name(struct btf_dump *d, __u32 id);
 static const char *btf_dump_ident_name(struct btf_dump *d, __u32 id);
-static size_t btf_dump_name_dups(struct btf_dump *d, struct hashmap *name_map,
-				 const char *orig_name);
+static ssize_t btf_dump_name_dups(struct btf_dump *d, struct hashmap *name_map,
+				  const char *orig_name);
 
 static bool btf_dump_is_blacklisted(struct btf_dump *d, __u32 id)
 {
@@ -995,7 +1006,7 @@ static void btf_dump_emit_enum32_val(struct btf_dump *d,
 	bool is_signed = btf_kflag(t);
 	const char *fmt_str;
 	const char *name;
-	size_t dup_cnt;
+	ssize_t dup_cnt;
 	int i;
 
 	for (i = 0; i < vlen; i++, v++) {
@@ -1020,7 +1031,7 @@ static void btf_dump_emit_enum64_val(struct btf_dump *d,
 	bool is_signed = btf_kflag(t);
 	const char *fmt_str;
 	const char *name;
-	size_t dup_cnt;
+	ssize_t dup_cnt;
 	__u64 val;
 	int i;
 
@@ -1521,14 +1532,30 @@ static void btf_dump_emit_type_cast(struct btf_dump *d, __u32 id,
 }
 
 /* return number of duplicates (occurrences) of a given name */
-static size_t btf_dump_name_dups(struct btf_dump *d, struct hashmap *name_map,
-				 const char *orig_name)
+static ssize_t btf_dump_name_dups(struct btf_dump *d, struct hashmap *name_map,
+				  const char *orig_name)
 {
-	size_t dup_cnt = 0;
+	int err;
+	char *old_name;
+	char *new_name;
+	ssize_t dup_cnt = 0;
+
+	new_name = strdup(orig_name);
+	if (!new_name)
+		return -ENOMEM;
 
 	hashmap__find(name_map, orig_name, (void **)&dup_cnt);
 	dup_cnt++;
-	hashmap__set(name_map, orig_name, (void *)dup_cnt, NULL, NULL);
+
+	err = hashmap__set(name_map, new_name, (void *)dup_cnt,
+			   (const void **)&old_name, NULL);
+	if (err) {
+		free(new_name);
+		return err;
+	}
+
+	if (old_name)
+		free(old_name);
 
 	return dup_cnt;
 }
-- 
2.30.2

[PATCH bpf v3 2/6] libbpf: Fix memory leak in parse_usdt_arg()

[Xu Kuohai]

In the arm64 version of parse_usdt_arg(), when sscanf returns 2, reg_name
is allocated but not freed. Fix it.

Fixes: 0f8619929c57 ("libbpf: Usdt aarch64 arg parsing support")
Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
---
 tools/lib/bpf/usdt.c | 59 +++++++++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 26 deletions(-)

diff --git a/tools/lib/bpf/usdt.c b/tools/lib/bpf/usdt.c
index e83b497c2245..f3b5be7415b5 100644
--- a/tools/lib/bpf/usdt.c
+++ b/tools/lib/bpf/usdt.c
@@ -1351,8 +1351,10 @@ static int parse_usdt_arg(const char *arg_str, int arg_num, struct usdt_arg_spec
 	char *reg_name = NULL;
 	int arg_sz, len, reg_off;
 	long off;
+	int ret;
 
-	if (sscanf(arg_str, " %d @ \[ %m[a-z0-9], %ld ] %n", &arg_sz, &reg_name, &off, &len) == 3) {
+	ret = sscanf(arg_str, " %d @ \[ %m[a-z0-9], %ld ] %n", &arg_sz, &reg_name, &off, &len);
+	if (ret == 3) {
 		/* Memory dereference case, e.g., -4@[sp, 96] */
 		arg->arg_type = USDT_ARG_REG_DEREF;
 		arg->val_off = off;
@@ -1361,32 +1363,37 @@ static int parse_usdt_arg(const char *arg_str, int arg_num, struct usdt_arg_spec
 		if (reg_off < 0)
 			return reg_off;
 		arg->reg_off = reg_off;
-	} else if (sscanf(arg_str, " %d @ \[ %m[a-z0-9] ] %n", &arg_sz, &reg_name, &len) == 2) {
-		/* Memory dereference case, e.g., -4@[sp] */
-		arg->arg_type = USDT_ARG_REG_DEREF;
-		arg->val_off = 0;
-		reg_off = calc_pt_regs_off(reg_name);
-		free(reg_name);
-		if (reg_off < 0)
-			return reg_off;
-		arg->reg_off = reg_off;
-	} else if (sscanf(arg_str, " %d @ %ld %n", &arg_sz, &off, &len) == 2) {
-		/* Constant value case, e.g., 4@5 */
-		arg->arg_type = USDT_ARG_CONST;
-		arg->val_off = off;
-		arg->reg_off = 0;
-	} else if (sscanf(arg_str, " %d @ %m[a-z0-9] %n", &arg_sz, &reg_name, &len) == 2) {
-		/* Register read case, e.g., -8@x4 */
-		arg->arg_type = USDT_ARG_REG;
-		arg->val_off = 0;
-		reg_off = calc_pt_regs_off(reg_name);
-		free(reg_name);
-		if (reg_off < 0)
-			return reg_off;
-		arg->reg_off = reg_off;
 	} else {
-		pr_warn("usdt: unrecognized arg #%d spec '%s'\n", arg_num, arg_str);
-		return -EINVAL;
+		if (ret == 2)
+			free(reg_name);
+
+		if (sscanf(arg_str, " %d @ \[ %m[a-z0-9] ] %n", &arg_sz, &reg_name, &len) == 2) {
+			/* Memory dereference case, e.g., -4@[sp] */
+			arg->arg_type = USDT_ARG_REG_DEREF;
+			arg->val_off = 0;
+			reg_off = calc_pt_regs_off(reg_name);
+			free(reg_name);
+			if (reg_off < 0)
+				return reg_off;
+			arg->reg_off = reg_off;
+		} else if (sscanf(arg_str, " %d @ %ld %n", &arg_sz, &off, &len) == 2) {
+			/* Constant value case, e.g., 4@5 */
+			arg->arg_type = USDT_ARG_CONST;
+			arg->val_off = off;
+			arg->reg_off = 0;
+		} else if (sscanf(arg_str, " %d @ %m[a-z0-9] %n", &arg_sz, &reg_name, &len) == 2) {
+			/* Register read case, e.g., -8@x4 */
+			arg->arg_type = USDT_ARG_REG;
+			arg->val_off = 0;
+			reg_off = calc_pt_regs_off(reg_name);
+			free(reg_name);
+			if (reg_off < 0)
+				return reg_off;
+			arg->reg_off = reg_off;
+		} else {
+			pr_warn("usdt: unrecognized arg #%d spec '%s'\n", arg_num, arg_str);
+			return -EINVAL;
+		}
 	}
 
 	arg->arg_signed = arg_sz < 0;
-- 
2.30.2

[PATCH bpf v3 3/6] selftests/bpf: Fix memory leak caused by not destroying skeleton

[Xu Kuohai]

Some test cases does not destroy skeleton object correctly, causing ASAN
to report memory leak warning. Fix it.

Fixes: 0ef6740e9777 ("selftests/bpf: Add tests for kptr_ref refcounting")
Fixes: 1642a3945e22 ("selftests/bpf: Add struct argument tests with fentry/fexit programs.")
Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
---
 tools/testing/selftests/bpf/prog_tests/map_kptr.c       | 3 ++-
 tools/testing/selftests/bpf/prog_tests/tracing_struct.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/map_kptr.c b/tools/testing/selftests/bpf/prog_tests/map_kptr.c
index fdcea7a61491..0d66b1524208 100644
--- a/tools/testing/selftests/bpf/prog_tests/map_kptr.c
+++ b/tools/testing/selftests/bpf/prog_tests/map_kptr.c
@@ -105,7 +105,7 @@ static void test_map_kptr_success(bool test_run)
 	ASSERT_OK(opts.retval, "test_map_kptr_ref2 retval");
 
 	if (test_run)
-		return;
+		goto exit;
 
 	ret = bpf_map__update_elem(skel->maps.array_map,
 				   &key, sizeof(key), buf, sizeof(buf), 0);
@@ -132,6 +132,7 @@ static void test_map_kptr_success(bool test_run)
 	ret = bpf_map__delete_elem(skel->maps.lru_hash_map, &key, sizeof(key), 0);
 	ASSERT_OK(ret, "lru_hash_map delete");
 
+exit:
 	map_kptr__destroy(skel);
 }
 
diff --git a/tools/testing/selftests/bpf/prog_tests/tracing_struct.c b/tools/testing/selftests/bpf/prog_tests/tracing_struct.c
index d5022b91d1e4..48dc9472e160 100644
--- a/tools/testing/selftests/bpf/prog_tests/tracing_struct.c
+++ b/tools/testing/selftests/bpf/prog_tests/tracing_struct.c
@@ -15,7 +15,7 @@ static void test_fentry(void)
 
 	err = tracing_struct__attach(skel);
 	if (!ASSERT_OK(err, "tracing_struct__attach"))
-		return;
+		goto destroy_skel;
 
 	ASSERT_OK(trigger_module_test_read(256), "trigger_read");
 
@@ -54,6 +54,7 @@ static void test_fentry(void)
 	ASSERT_EQ(skel->bss->t5_ret, 1, "t5 ret");
 
 	tracing_struct__detach(skel);
+destroy_skel:
 	tracing_struct__destroy(skel);
 }
 
-- 
2.30.2

[PATCH bpf v3 4/6] selftest/bpf: Fix memory leak in kprobe_multi_test

[Xu Kuohai]

The get_syms() function in kprobe_multi_test.c does not free the string
memory allocated by sscanf correctly. Fix it.

Fixes: 5b6c7e5c4434 ("selftests/bpf: Add attach bench test")
Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
---
 .../bpf/prog_tests/kprobe_multi_test.c          | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
index d457a55ff408..07dd2c5b7f98 100644
--- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
+++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
@@ -360,15 +360,14 @@ static int get_syms(char ***symsp, size_t *cntp)
 		 * to them. Filter out the current culprits - arch_cpu_idle
 		 * and rcu_* functions.
 		 */
-		if (!strcmp(name, "arch_cpu_idle"))
-			continue;
-		if (!strncmp(name, "rcu_", 4))
-			continue;
-		if (!strcmp(name, "bpf_dispatcher_xdp_func"))
-			continue;
-		if (!strncmp(name, "__ftrace_invalid_address__",
-			     sizeof("__ftrace_invalid_address__") - 1))
+		if (!strcmp(name, "arch_cpu_idle") ||
+			!strncmp(name, "rcu_", 4) ||
+			!strcmp(name, "bpf_dispatcher_xdp_func") ||
+			!strncmp(name, "__ftrace_invalid_address__",
+				 sizeof("__ftrace_invalid_address__") - 1)) {
+			free(name);
 			continue;
+		}
 		err = hashmap__add(map, name, NULL);
 		if (err) {
 			free(name);
@@ -394,7 +393,7 @@ static int get_syms(char ***symsp, size_t *cntp)
 	hashmap__free(map);
 	if (err) {
 		for (i = 0; i < cnt; i++)
-			free(syms[cnt]);
+			free(syms[i]);
 		free(syms);
 	}
 	return err;
-- 
2.30.2

[PATCH bpf v3 5/6] selftests/bpf: Fix error failure of case test_xdp_adjust_tail_grow

[Xu Kuohai]

test_xdp_adjust_tail_grow failed with ipv6:
  test_xdp_adjust_tail_grow:FAIL:ipv6 unexpected error: -28 (errno 28)

The reason is that this test case tests ipv4 before ipv6, and when ipv4
test finished, topts.data_size_out was set to 54, which is smaller than the
ipv6 output data size 114, so ipv6 test fails with NOSPC error.

Fix it by reset topts.data_size_out to sizeof(buf) before testing ipv6.

Fixes: 04fcb5f9a104 ("selftests/bpf: Migrate from bpf_prog_test_run")
Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
---
 tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c b/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c
index 9b9cf8458adf..009ee37607df 100644
--- a/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c
@@ -63,6 +63,7 @@ static void test_xdp_adjust_tail_grow(void)
 	expect_sz = sizeof(pkt_v6) + 40; /* Test grow with 40 bytes */
 	topts.data_in = &pkt_v6;
 	topts.data_size_in = sizeof(pkt_v6);
+	topts.data_size_out = sizeof(buf);
 	err = bpf_prog_test_run_opts(prog_fd, &topts);
 	ASSERT_OK(err, "ipv6");
 	ASSERT_EQ(topts.retval, XDP_TX, "ipv6 retval");
-- 
2.30.2

[PATCH bpf v3 6/6] selftest/bpf: Fix error usage of ASSERT_OK in xdp_adjust_tail.c

[Xu Kuohai]

xdp_adjust_tail.c calls ASSERT_OK() to check the return value of
bpf_prog_test_load(), but the condition is not correct. Fix it.

Fixes: 791cad025051 ("bpf: selftests: Get rid of CHECK macro in xdp_adjust_tail.c")
Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
---
 tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c b/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c
index 009ee37607df..39973ea1ce43 100644
--- a/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c
@@ -18,7 +18,7 @@ static void test_xdp_adjust_tail_shrink(void)
 	);
 
 	err = bpf_prog_test_load(file, BPF_PROG_TYPE_XDP, &obj, &prog_fd);
-	if (ASSERT_OK(err, "test_xdp_adjust_tail_shrink"))
+	if (!ASSERT_OK(err, "test_xdp_adjust_tail_shrink"))
 		return;
 
 	err = bpf_prog_test_run_opts(prog_fd, &topts);
@@ -53,7 +53,7 @@ static void test_xdp_adjust_tail_grow(void)
 	);
 
 	err = bpf_prog_test_load(file, BPF_PROG_TYPE_XDP, &obj, &prog_fd);
-	if (ASSERT_OK(err, "test_xdp_adjust_tail_grow"))
+	if (!ASSERT_OK(err, "test_xdp_adjust_tail_grow"))
 		return;
 
 	err = bpf_prog_test_run_opts(prog_fd, &topts);
@@ -90,7 +90,7 @@ static void test_xdp_adjust_tail_grow2(void)
 	);
 
 	err = bpf_prog_test_load(file, BPF_PROG_TYPE_XDP, &obj, &prog_fd);
-	if (ASSERT_OK(err, "test_xdp_adjust_tail_grow"))
+	if (!ASSERT_OK(err, "test_xdp_adjust_tail_grow"))
 		return;
 
 	/* Test case-64 */
-- 
2.30.2

Re: [PATCH bpf v3 1/6] libbpf: Fix use-after-free in btf_dump_name_dups

[Andrii Nakryiko]

On Mon, Oct 10, 2022 at 7:08 AM Xu Kuohai <xukuohai@huawei.com> wrote:
>
> ASAN reports an use-after-free in btf_dump_name_dups:
>
> ERROR: AddressSanitizer: heap-use-after-free on address 0xffff927006db at pc 0xaaaab5dfb618 bp 0xffffdd89b890 sp 0xffffdd89b928
> READ of size 2 at 0xffff927006db thread T0
>     #0 0xaaaab5dfb614 in __interceptor_strcmp.part.0 (test_progs+0x21b614)
>     #1 0xaaaab635f144 in str_equal_fn tools/lib/bpf/btf_dump.c:127
>     #2 0xaaaab635e3e0 in hashmap_find_entry tools/lib/bpf/hashmap.c:143
>     #3 0xaaaab635e72c in hashmap__find tools/lib/bpf/hashmap.c:212
>     #4 0xaaaab6362258 in btf_dump_name_dups tools/lib/bpf/btf_dump.c:1525
>     #5 0xaaaab636240c in btf_dump_resolve_name tools/lib/bpf/btf_dump.c:1552
>     #6 0xaaaab6362598 in btf_dump_type_name tools/lib/bpf/btf_dump.c:1567
>     #7 0xaaaab6360b48 in btf_dump_emit_struct_def tools/lib/bpf/btf_dump.c:912
>     #8 0xaaaab6360630 in btf_dump_emit_type tools/lib/bpf/btf_dump.c:798
>     #9 0xaaaab635f720 in btf_dump__dump_type tools/lib/bpf/btf_dump.c:282
>     #10 0xaaaab608523c in test_btf_dump_incremental tools/testing/selftests/bpf/prog_tests/btf_dump.c:236
>     #11 0xaaaab6097530 in test_btf_dump tools/testing/selftests/bpf/prog_tests/btf_dump.c:875
>     #12 0xaaaab6314ed0 in run_one_test tools/testing/selftests/bpf/test_progs.c:1062
>     #13 0xaaaab631a0a8 in main tools/testing/selftests/bpf/test_progs.c:1697
>     #14 0xffff9676d214 in __libc_start_main ../csu/libc-start.c:308
>     #15 0xaaaab5d65990  (test_progs+0x185990)
>
> 0xffff927006db is located 11 bytes inside of 16-byte region [0xffff927006d0,0xffff927006e0)
> freed by thread T0 here:
>     #0 0xaaaab5e2c7c4 in realloc (test_progs+0x24c7c4)
>     #1 0xaaaab634f4a0 in libbpf_reallocarray tools/lib/bpf/libbpf_internal.h:191
>     #2 0xaaaab634f840 in libbpf_add_mem tools/lib/bpf/btf.c:163
>     #3 0xaaaab636643c in strset_add_str_mem tools/lib/bpf/strset.c:106
>     #4 0xaaaab6366560 in strset__add_str tools/lib/bpf/strset.c:157
>     #5 0xaaaab6352d70 in btf__add_str tools/lib/bpf/btf.c:1519
>     #6 0xaaaab6353e10 in btf__add_field tools/lib/bpf/btf.c:2032
>     #7 0xaaaab6084fcc in test_btf_dump_incremental tools/testing/selftests/bpf/prog_tests/btf_dump.c:232
>     #8 0xaaaab6097530 in test_btf_dump tools/testing/selftests/bpf/prog_tests/btf_dump.c:875
>     #9 0xaaaab6314ed0 in run_one_test tools/testing/selftests/bpf/test_progs.c:1062
>     #10 0xaaaab631a0a8 in main tools/testing/selftests/bpf/test_progs.c:1697
>     #11 0xffff9676d214 in __libc_start_main ../csu/libc-start.c:308
>     #12 0xaaaab5d65990  (test_progs+0x185990)
>
> previously allocated by thread T0 here:
>     #0 0xaaaab5e2c7c4 in realloc (test_progs+0x24c7c4)
>     #1 0xaaaab634f4a0 in libbpf_reallocarray tools/lib/bpf/libbpf_internal.h:191
>     #2 0xaaaab634f840 in libbpf_add_mem tools/lib/bpf/btf.c:163
>     #3 0xaaaab636643c in strset_add_str_mem tools/lib/bpf/strset.c:106
>     #4 0xaaaab6366560 in strset__add_str tools/lib/bpf/strset.c:157
>     #5 0xaaaab6352d70 in btf__add_str tools/lib/bpf/btf.c:1519
>     #6 0xaaaab6353ff0 in btf_add_enum_common tools/lib/bpf/btf.c:2070
>     #7 0xaaaab6354080 in btf__add_enum tools/lib/bpf/btf.c:2102
>     #8 0xaaaab6082f50 in test_btf_dump_incremental tools/testing/selftests/bpf/prog_tests/btf_dump.c:162
>     #9 0xaaaab6097530 in test_btf_dump tools/testing/selftests/bpf/prog_tests/btf_dump.c:875
>     #10 0xaaaab6314ed0 in run_one_test tools/testing/selftests/bpf/test_progs.c:1062
>     #11 0xaaaab631a0a8 in main tools/testing/selftests/bpf/test_progs.c:1697
>     #12 0xffff9676d214 in __libc_start_main ../csu/libc-start.c:308
>     #13 0xaaaab5d65990  (test_progs+0x185990)
>
> The reason is that the key stored in hash table name_map is a string
> address, and the string memory is allocated by realloc() function, when
> the memory is resized by realloc() later, the old memory may be freed,
> so the address stored in name_map references to a freed memory, causing
> use-after-free.
>
> Fix it by storing duplicated string address in name_map.
>
> Fixes: 351131b51c7a ("libbpf: add btf_dump API for BTF-to-C conversion")

this is not quite correct, because when btf_dump API was added struct
btf was immutable. So fixes tag should point to commit that added
btf__add_xxx() APIs, which at that point broke btf_dump APIs.

> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
> ---
>  tools/lib/bpf/btf_dump.c | 47 +++++++++++++++++++++++++++++++---------
>  1 file changed, 37 insertions(+), 10 deletions(-)
>
> diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
> index e4da6de68d8f..8365d801cbd0 100644
> --- a/tools/lib/bpf/btf_dump.c
> +++ b/tools/lib/bpf/btf_dump.c
> @@ -219,6 +219,17 @@ static int btf_dump_resize(struct btf_dump *d)
>         return 0;
>  }
>
> +static void btf_dump_free_names(struct hashmap *map)
> +{
> +       size_t bkt;
> +       struct hashmap_entry *cur;
> +
> +       hashmap__for_each_entry(map, cur, bkt)
> +               free((void *)cur->key);
> +
> +       hashmap__free(map);
> +}
> +
>  void btf_dump__free(struct btf_dump *d)
>  {
>         int i;
> @@ -237,8 +248,8 @@ void btf_dump__free(struct btf_dump *d)
>         free(d->cached_names);
>         free(d->emit_queue);
>         free(d->decl_stack);
> -       hashmap__free(d->type_names);
> -       hashmap__free(d->ident_names);
> +       btf_dump_free_names(d->type_names);
> +       btf_dump_free_names(d->ident_names);
>
>         free(d);
>  }
> @@ -634,8 +645,8 @@ static void btf_dump_emit_type_chain(struct btf_dump *d,
>
>  static const char *btf_dump_type_name(struct btf_dump *d, __u32 id);
>  static const char *btf_dump_ident_name(struct btf_dump *d, __u32 id);
> -static size_t btf_dump_name_dups(struct btf_dump *d, struct hashmap *name_map,
> -                                const char *orig_name);
> +static ssize_t btf_dump_name_dups(struct btf_dump *d, struct hashmap *name_map,
> +                                 const char *orig_name);
>
>  static bool btf_dump_is_blacklisted(struct btf_dump *d, __u32 id)
>  {
> @@ -995,7 +1006,7 @@ static void btf_dump_emit_enum32_val(struct btf_dump *d,
>         bool is_signed = btf_kflag(t);
>         const char *fmt_str;
>         const char *name;
> -       size_t dup_cnt;
> +       ssize_t dup_cnt;
>         int i;
>
>         for (i = 0; i < vlen; i++, v++) {
> @@ -1020,7 +1031,7 @@ static void btf_dump_emit_enum64_val(struct btf_dump *d,
>         bool is_signed = btf_kflag(t);
>         const char *fmt_str;
>         const char *name;
> -       size_t dup_cnt;
> +       ssize_t dup_cnt;
>         __u64 val;
>         int i;
>
> @@ -1521,14 +1532,30 @@ static void btf_dump_emit_type_cast(struct btf_dump *d, __u32 id,
>  }
>
>  /* return number of duplicates (occurrences) of a given name */
> -static size_t btf_dump_name_dups(struct btf_dump *d, struct hashmap *name_map,
> -                                const char *orig_name)
> +static ssize_t btf_dump_name_dups(struct btf_dump *d, struct hashmap *name_map,
> +                                 const char *orig_name)
>  {
> -       size_t dup_cnt = 0;
> +       int err;
> +       char *old_name;
> +       char *new_name;
> +       ssize_t dup_cnt = 0;
> +
> +       new_name = strdup(orig_name);
> +       if (!new_name)
> +               return -ENOMEM;
>
>         hashmap__find(name_map, orig_name, (void **)&dup_cnt);
>         dup_cnt++;
> -       hashmap__set(name_map, orig_name, (void *)dup_cnt, NULL, NULL);
> +
> +       err = hashmap__set(name_map, new_name, (void *)dup_cnt,
> +                          (const void **)&old_name, NULL);
> +       if (err) {
> +               free(new_name);
> +               return err;
> +       }
> +
> +       if (old_name)
> +               free(old_name);
>

you'll notice that btf_dump has lots of void functions and has a bit
different approach to error handling. When the error isn't leading to
a crash, we just ignore it with the idea that if some memory
allocation failed (a quite unlikely event in general), we'll end up
generating incomplete btf_dump output. Same here, no one is checking
btf_dump_name_dups() return result for errors (e.g.,
btf_dump_emit_enum32_val doesn't).

So, I propose to follow that approach here. strdup(orig_name), if that
failed, return 1. Which is exactly the behavior if hashmap__set()
failed due to memory allocation failure.

>         return dup_cnt;
>  }
> --
> 2.30.2
>

Re: [PATCH bpf v3 2/6] libbpf: Fix memory leak in parse_usdt_arg()

[Andrii Nakryiko]

On Mon, Oct 10, 2022 at 7:08 AM Xu Kuohai <xukuohai@huawei.com> wrote:
>
> In the arm64 version of parse_usdt_arg(), when sscanf returns 2, reg_name
> is allocated but not freed. Fix it.
>
> Fixes: 0f8619929c57 ("libbpf: Usdt aarch64 arg parsing support")
> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
> ---
>  tools/lib/bpf/usdt.c | 59 +++++++++++++++++++++++++-------------------
>  1 file changed, 33 insertions(+), 26 deletions(-)
>
> diff --git a/tools/lib/bpf/usdt.c b/tools/lib/bpf/usdt.c
> index e83b497c2245..f3b5be7415b5 100644
> --- a/tools/lib/bpf/usdt.c
> +++ b/tools/lib/bpf/usdt.c
> @@ -1351,8 +1351,10 @@ static int parse_usdt_arg(const char *arg_str, int arg_num, struct usdt_arg_spec
>         char *reg_name = NULL;
>         int arg_sz, len, reg_off;
>         long off;
> +       int ret;
>
> -       if (sscanf(arg_str, " %d @ \[ %m[a-z0-9], %ld ] %n", &arg_sz, &reg_name, &off, &len) == 3) {
> +       ret = sscanf(arg_str, " %d @ \[ %m[a-z0-9], %ld ] %n", &arg_sz, &reg_name, &off, &len);
> +       if (ret == 3) {
>                 /* Memory dereference case, e.g., -4@[sp, 96] */
>                 arg->arg_type = USDT_ARG_REG_DEREF;
>                 arg->val_off = off;
> @@ -1361,32 +1363,37 @@ static int parse_usdt_arg(const char *arg_str, int arg_num, struct usdt_arg_spec
>                 if (reg_off < 0)
>                         return reg_off;
>                 arg->reg_off = reg_off;
> -       } else if (sscanf(arg_str, " %d @ \[ %m[a-z0-9] ] %n", &arg_sz, &reg_name, &len) == 2) {
> -               /* Memory dereference case, e.g., -4@[sp] */
> -               arg->arg_type = USDT_ARG_REG_DEREF;
> -               arg->val_off = 0;
> -               reg_off = calc_pt_regs_off(reg_name);
> -               free(reg_name);
> -               if (reg_off < 0)
> -                       return reg_off;
> -               arg->reg_off = reg_off;
> -       } else if (sscanf(arg_str, " %d @ %ld %n", &arg_sz, &off, &len) == 2) {
> -               /* Constant value case, e.g., 4@5 */
> -               arg->arg_type = USDT_ARG_CONST;
> -               arg->val_off = off;
> -               arg->reg_off = 0;
> -       } else if (sscanf(arg_str, " %d @ %m[a-z0-9] %n", &arg_sz, &reg_name, &len) == 2) {
> -               /* Register read case, e.g., -8@x4 */
> -               arg->arg_type = USDT_ARG_REG;
> -               arg->val_off = 0;
> -               reg_off = calc_pt_regs_off(reg_name);
> -               free(reg_name);
> -               if (reg_off < 0)
> -                       return reg_off;
> -               arg->reg_off = reg_off;
>         } else {
> -               pr_warn("usdt: unrecognized arg #%d spec '%s'\n", arg_num, arg_str);
> -               return -EINVAL;
> +               if (ret == 2)
> +                       free(reg_name);
> +
> +               if (sscanf(arg_str, " %d @ \[ %m[a-z0-9] ] %n", &arg_sz, &reg_name, &len) == 2) {
> +                       /* Memory dereference case, e.g., -4@[sp] */
> +                       arg->arg_type = USDT_ARG_REG_DEREF;
> +                       arg->val_off = 0;
> +                       reg_off = calc_pt_regs_off(reg_name);
> +                       free(reg_name);
> +                       if (reg_off < 0)
> +                               return reg_off;
> +                       arg->reg_off = reg_off;
> +               } else if (sscanf(arg_str, " %d @ %ld %n", &arg_sz, &off, &len) == 2) {
> +                       /* Constant value case, e.g., 4@5 */
> +                       arg->arg_type = USDT_ARG_CONST;
> +                       arg->val_off = off;
> +                       arg->reg_off = 0;
> +               } else if (sscanf(arg_str, " %d @ %m[a-z0-9] %n", &arg_sz, &reg_name, &len) == 2) {
> +                       /* Register read case, e.g., -8@x4 */
> +                       arg->arg_type = USDT_ARG_REG;
> +                       arg->val_off = 0;
> +                       reg_off = calc_pt_regs_off(reg_name);
> +                       free(reg_name);
> +                       if (reg_off < 0)
> +                               return reg_off;
> +                       arg->reg_off = reg_off;
> +               } else {
> +                       pr_warn("usdt: unrecognized arg #%d spec '%s'\n", arg_num, arg_str);
> +                       return -EINVAL;
> +               }
>         }
>

I think all this is more complicated than it has to be. How big  can
register names be? Few characters? Let's get rid of %m[a-z0-9] and
instead use fixed-max-length strings, e.g., %5s. And read register
names into such local char buffers. It will simplify everything
tremendously. Let's use 16-byte buffers and use %15s to match it?
Would that be enough?

>         arg->arg_signed = arg_sz < 0;
> --
> 2.30.2
>

Re: [PATCH bpf v3 4/6] selftest/bpf: Fix memory leak in kprobe_multi_test

[Andrii Nakryiko]

On Mon, Oct 10, 2022 at 7:08 AM Xu Kuohai <xukuohai@huawei.com> wrote:
>
> The get_syms() function in kprobe_multi_test.c does not free the string
> memory allocated by sscanf correctly. Fix it.
>
> Fixes: 5b6c7e5c4434 ("selftests/bpf: Add attach bench test")
> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
> Acked-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  .../bpf/prog_tests/kprobe_multi_test.c          | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> index d457a55ff408..07dd2c5b7f98 100644
> --- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> @@ -360,15 +360,14 @@ static int get_syms(char ***symsp, size_t *cntp)
>                  * to them. Filter out the current culprits - arch_cpu_idle
>                  * and rcu_* functions.
>                  */
> -               if (!strcmp(name, "arch_cpu_idle"))
> -                       continue;
> -               if (!strncmp(name, "rcu_", 4))
> -                       continue;
> -               if (!strcmp(name, "bpf_dispatcher_xdp_func"))
> -                       continue;
> -               if (!strncmp(name, "__ftrace_invalid_address__",
> -                            sizeof("__ftrace_invalid_address__") - 1))
> +               if (!strcmp(name, "arch_cpu_idle") ||
> +                       !strncmp(name, "rcu_", 4) ||
> +                       !strcmp(name, "bpf_dispatcher_xdp_func") ||
> +                       !strncmp(name, "__ftrace_invalid_address__",
> +                                sizeof("__ftrace_invalid_address__") - 1)) {
> +                       free(name);
>                         continue;
> +               }

it seems cleaner if we add if (name) free(name) under error: goto
label. And in the success case when we assign name to syms[cnt] we can
reset name to NULL to avoid double-free. WDYT?


>                 err = hashmap__add(map, name, NULL);
>                 if (err) {
>                         free(name);
> @@ -394,7 +393,7 @@ static int get_syms(char ***symsp, size_t *cntp)
>         hashmap__free(map);
>         if (err) {
>                 for (i = 0; i < cnt; i++)
> -                       free(syms[cnt]);
> +                       free(syms[i]);
>                 free(syms);
>         }
>         return err;
> --
> 2.30.2
>

Re: [PATCH bpf v3 0/6] Fix bugs found by ASAN when running selftests

[Andrii Nakryiko]

On Mon, Oct 10, 2022 at 7:08 AM Xu Kuohai <xukuohai@huawei.com> wrote:
>

Thanks for the fixes! I left a few comments in a few patches, please
address those. But also
please provide a commit message, even if a single line one. Kernel
style dictates that the commit message shouldn't be empty.

And I think none of these fixes are critical enough to go to bpf tree,
please target bpf-next for next revision. Thanks.


> v3:
> - Fix error failure of case test_xdp_adjust_tail_grow exposed by this series
>
> v2: https://lore.kernel.org/bpf/20221010070454.577433-1-xukuohai@huaweicloud.com
> - Rebase and fix conflict
>
> v1: https://lore.kernel.org/bpf/20221009131830.395569-1-xukuohai@huaweicloud.com
>
> Xu Kuohai (6):
>   libbpf: Fix use-after-free in btf_dump_name_dups
>   libbpf: Fix memory leak in parse_usdt_arg()
>   selftests/bpf: Fix memory leak caused by not destroying skeleton
>   selftest/bpf: Fix memory leak in kprobe_multi_test
>   selftests/bpf: Fix error failure of case test_xdp_adjust_tail_grow
>   selftest/bpf: Fix error usage of ASSERT_OK in xdp_adjust_tail.c
>
>  tools/lib/bpf/btf_dump.c                      | 47 +++++++++++----
>  tools/lib/bpf/usdt.c                          | 59 +++++++++++--------
>  .../bpf/prog_tests/kprobe_multi_test.c        | 17 +++---
>  .../selftests/bpf/prog_tests/map_kptr.c       |  3 +-
>  .../selftests/bpf/prog_tests/tracing_struct.c |  3 +-
>  .../bpf/prog_tests/xdp_adjust_tail.c          |  7 ++-
>  6 files changed, 86 insertions(+), 50 deletions(-)
>
> --
> 2.30.2
>

Re: [PATCH bpf v3 1/6] libbpf: Fix use-after-free in btf_dump_name_dups

[Xu Kuohai]

On 10/11/2022 9:32 AM, Andrii Nakryiko wrote:
> On Mon, Oct 10, 2022 at 7:08 AM Xu Kuohai <xukuohai@huawei.com> wrote:
>>
>> ASAN reports an use-after-free in btf_dump_name_dups:
>>
>> ERROR: AddressSanitizer: heap-use-after-free on address 0xffff927006db at pc 0xaaaab5dfb618 bp 0xffffdd89b890 sp 0xffffdd89b928
>> READ of size 2 at 0xffff927006db thread T0
>>      #0 0xaaaab5dfb614 in __interceptor_strcmp.part.0 (test_progs+0x21b614)
>>      #1 0xaaaab635f144 in str_equal_fn tools/lib/bpf/btf_dump.c:127
>>      #2 0xaaaab635e3e0 in hashmap_find_entry tools/lib/bpf/hashmap.c:143
>>      #3 0xaaaab635e72c in hashmap__find tools/lib/bpf/hashmap.c:212
>>      #4 0xaaaab6362258 in btf_dump_name_dups tools/lib/bpf/btf_dump.c:1525
>>      #5 0xaaaab636240c in btf_dump_resolve_name tools/lib/bpf/btf_dump.c:1552
>>      #6 0xaaaab6362598 in btf_dump_type_name tools/lib/bpf/btf_dump.c:1567
>>      #7 0xaaaab6360b48 in btf_dump_emit_struct_def tools/lib/bpf/btf_dump.c:912
>>      #8 0xaaaab6360630 in btf_dump_emit_type tools/lib/bpf/btf_dump.c:798
>>      #9 0xaaaab635f720 in btf_dump__dump_type tools/lib/bpf/btf_dump.c:282
>>      #10 0xaaaab608523c in test_btf_dump_incremental tools/testing/selftests/bpf/prog_tests/btf_dump.c:236
>>      #11 0xaaaab6097530 in test_btf_dump tools/testing/selftests/bpf/prog_tests/btf_dump.c:875
>>      #12 0xaaaab6314ed0 in run_one_test tools/testing/selftests/bpf/test_progs.c:1062
>>      #13 0xaaaab631a0a8 in main tools/testing/selftests/bpf/test_progs.c:1697
>>      #14 0xffff9676d214 in __libc_start_main ../csu/libc-start.c:308
>>      #15 0xaaaab5d65990  (test_progs+0x185990)
>>
>> 0xffff927006db is located 11 bytes inside of 16-byte region [0xffff927006d0,0xffff927006e0)
>> freed by thread T0 here:
>>      #0 0xaaaab5e2c7c4 in realloc (test_progs+0x24c7c4)
>>      #1 0xaaaab634f4a0 in libbpf_reallocarray tools/lib/bpf/libbpf_internal.h:191
>>      #2 0xaaaab634f840 in libbpf_add_mem tools/lib/bpf/btf.c:163
>>      #3 0xaaaab636643c in strset_add_str_mem tools/lib/bpf/strset.c:106
>>      #4 0xaaaab6366560 in strset__add_str tools/lib/bpf/strset.c:157
>>      #5 0xaaaab6352d70 in btf__add_str tools/lib/bpf/btf.c:1519
>>      #6 0xaaaab6353e10 in btf__add_field tools/lib/bpf/btf.c:2032
>>      #7 0xaaaab6084fcc in test_btf_dump_incremental tools/testing/selftests/bpf/prog_tests/btf_dump.c:232
>>      #8 0xaaaab6097530 in test_btf_dump tools/testing/selftests/bpf/prog_tests/btf_dump.c:875
>>      #9 0xaaaab6314ed0 in run_one_test tools/testing/selftests/bpf/test_progs.c:1062
>>      #10 0xaaaab631a0a8 in main tools/testing/selftests/bpf/test_progs.c:1697
>>      #11 0xffff9676d214 in __libc_start_main ../csu/libc-start.c:308
>>      #12 0xaaaab5d65990  (test_progs+0x185990)
>>
>> previously allocated by thread T0 here:
>>      #0 0xaaaab5e2c7c4 in realloc (test_progs+0x24c7c4)
>>      #1 0xaaaab634f4a0 in libbpf_reallocarray tools/lib/bpf/libbpf_internal.h:191
>>      #2 0xaaaab634f840 in libbpf_add_mem tools/lib/bpf/btf.c:163
>>      #3 0xaaaab636643c in strset_add_str_mem tools/lib/bpf/strset.c:106
>>      #4 0xaaaab6366560 in strset__add_str tools/lib/bpf/strset.c:157
>>      #5 0xaaaab6352d70 in btf__add_str tools/lib/bpf/btf.c:1519
>>      #6 0xaaaab6353ff0 in btf_add_enum_common tools/lib/bpf/btf.c:2070
>>      #7 0xaaaab6354080 in btf__add_enum tools/lib/bpf/btf.c:2102
>>      #8 0xaaaab6082f50 in test_btf_dump_incremental tools/testing/selftests/bpf/prog_tests/btf_dump.c:162
>>      #9 0xaaaab6097530 in test_btf_dump tools/testing/selftests/bpf/prog_tests/btf_dump.c:875
>>      #10 0xaaaab6314ed0 in run_one_test tools/testing/selftests/bpf/test_progs.c:1062
>>      #11 0xaaaab631a0a8 in main tools/testing/selftests/bpf/test_progs.c:1697
>>      #12 0xffff9676d214 in __libc_start_main ../csu/libc-start.c:308
>>      #13 0xaaaab5d65990  (test_progs+0x185990)
>>
>> The reason is that the key stored in hash table name_map is a string
>> address, and the string memory is allocated by realloc() function, when
>> the memory is resized by realloc() later, the old memory may be freed,
>> so the address stored in name_map references to a freed memory, causing
>> use-after-free.
>>
>> Fix it by storing duplicated string address in name_map.
>>
>> Fixes: 351131b51c7a ("libbpf: add btf_dump API for BTF-to-C conversion")
> 
> this is not quite correct, because when btf_dump API was added struct
> btf was immutable. So fixes tag should point to commit that added
> btf__add_xxx() APIs, which at that point broke btf_dump APIs.
> 

will change it to

Fixes: 919d2b1dbb07 ("libbpf: Allow modification of BTF and add btf__add_str API")

>> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
>> ---
>>   tools/lib/bpf/btf_dump.c | 47 +++++++++++++++++++++++++++++++---------
>>   1 file changed, 37 insertions(+), 10 deletions(-)
>>
>> diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
>> index e4da6de68d8f..8365d801cbd0 100644
>> --- a/tools/lib/bpf/btf_dump.c
>> +++ b/tools/lib/bpf/btf_dump.c
>> @@ -219,6 +219,17 @@ static int btf_dump_resize(struct btf_dump *d)
>>          return 0;
>>   }
>>
>> +static void btf_dump_free_names(struct hashmap *map)
>> +{
>> +       size_t bkt;
>> +       struct hashmap_entry *cur;
>> +
>> +       hashmap__for_each_entry(map, cur, bkt)
>> +               free((void *)cur->key);
>> +
>> +       hashmap__free(map);
>> +}
>> +
>>   void btf_dump__free(struct btf_dump *d)
>>   {
>>          int i;
>> @@ -237,8 +248,8 @@ void btf_dump__free(struct btf_dump *d)
>>          free(d->cached_names);
>>          free(d->emit_queue);
>>          free(d->decl_stack);
>> -       hashmap__free(d->type_names);
>> -       hashmap__free(d->ident_names);
>> +       btf_dump_free_names(d->type_names);
>> +       btf_dump_free_names(d->ident_names);
>>
>>          free(d);
>>   }
>> @@ -634,8 +645,8 @@ static void btf_dump_emit_type_chain(struct btf_dump *d,
>>
>>   static const char *btf_dump_type_name(struct btf_dump *d, __u32 id);
>>   static const char *btf_dump_ident_name(struct btf_dump *d, __u32 id);
>> -static size_t btf_dump_name_dups(struct btf_dump *d, struct hashmap *name_map,
>> -                                const char *orig_name);
>> +static ssize_t btf_dump_name_dups(struct btf_dump *d, struct hashmap *name_map,
>> +                                 const char *orig_name);
>>
>>   static bool btf_dump_is_blacklisted(struct btf_dump *d, __u32 id)
>>   {
>> @@ -995,7 +1006,7 @@ static void btf_dump_emit_enum32_val(struct btf_dump *d,
>>          bool is_signed = btf_kflag(t);
>>          const char *fmt_str;
>>          const char *name;
>> -       size_t dup_cnt;
>> +       ssize_t dup_cnt;
>>          int i;
>>
>>          for (i = 0; i < vlen; i++, v++) {
>> @@ -1020,7 +1031,7 @@ static void btf_dump_emit_enum64_val(struct btf_dump *d,
>>          bool is_signed = btf_kflag(t);
>>          const char *fmt_str;
>>          const char *name;
>> -       size_t dup_cnt;
>> +       ssize_t dup_cnt;
>>          __u64 val;
>>          int i;
>>
>> @@ -1521,14 +1532,30 @@ static void btf_dump_emit_type_cast(struct btf_dump *d, __u32 id,
>>   }
>>
>>   /* return number of duplicates (occurrences) of a given name */
>> -static size_t btf_dump_name_dups(struct btf_dump *d, struct hashmap *name_map,
>> -                                const char *orig_name)
>> +static ssize_t btf_dump_name_dups(struct btf_dump *d, struct hashmap *name_map,
>> +                                 const char *orig_name)
>>   {
>> -       size_t dup_cnt = 0;
>> +       int err;
>> +       char *old_name;
>> +       char *new_name;
>> +       ssize_t dup_cnt = 0;
>> +
>> +       new_name = strdup(orig_name);
>> +       if (!new_name)
>> +               return -ENOMEM;
>>
>>          hashmap__find(name_map, orig_name, (void **)&dup_cnt);
>>          dup_cnt++;
>> -       hashmap__set(name_map, orig_name, (void *)dup_cnt, NULL, NULL);
>> +
>> +       err = hashmap__set(name_map, new_name, (void *)dup_cnt,
>> +                          (const void **)&old_name, NULL);
>> +       if (err) {
>> +               free(new_name);
>> +               return err;
>> +       }
>> +
>> +       if (old_name)
>> +               free(old_name);
>>
> 
> you'll notice that btf_dump has lots of void functions and has a bit
> different approach to error handling. When the error isn't leading to
> a crash, we just ignore it with the idea that if some memory
> allocation failed (a quite unlikely event in general), we'll end up
> generating incomplete btf_dump output. Same here, no one is checking
> btf_dump_name_dups() return result for errors (e.g.,
> btf_dump_emit_enum32_val doesn't).
> 
> So, I propose to follow that approach here. strdup(orig_name), if that
> failed, return 1. Which is exactly the behavior if hashmap__set()
> failed due to memory allocation failure.
> 

OK, will do

>>          return dup_cnt;
>>   }
>> --
>> 2.30.2
>>
> .

Re: [PATCH bpf v3 2/6] libbpf: Fix memory leak in parse_usdt_arg()

[Xu Kuohai]

On 10/11/2022 9:34 AM, Andrii Nakryiko wrote:
> On Mon, Oct 10, 2022 at 7:08 AM Xu Kuohai <xukuohai@huawei.com> wrote:
>>
>> In the arm64 version of parse_usdt_arg(), when sscanf returns 2, reg_name
>> is allocated but not freed. Fix it.
>>
>> Fixes: 0f8619929c57 ("libbpf: Usdt aarch64 arg parsing support")
>> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
>> ---
>>   tools/lib/bpf/usdt.c | 59 +++++++++++++++++++++++++-------------------
>>   1 file changed, 33 insertions(+), 26 deletions(-)
>>
>> diff --git a/tools/lib/bpf/usdt.c b/tools/lib/bpf/usdt.c
>> index e83b497c2245..f3b5be7415b5 100644
>> --- a/tools/lib/bpf/usdt.c
>> +++ b/tools/lib/bpf/usdt.c
>> @@ -1351,8 +1351,10 @@ static int parse_usdt_arg(const char *arg_str, int arg_num, struct usdt_arg_spec
>>          char *reg_name = NULL;
>>          int arg_sz, len, reg_off;
>>          long off;
>> +       int ret;
>>
>> -       if (sscanf(arg_str, " %d @ \[ %m[a-z0-9], %ld ] %n", &arg_sz, &reg_name, &off, &len) == 3) {
>> +       ret = sscanf(arg_str, " %d @ \[ %m[a-z0-9], %ld ] %n", &arg_sz, &reg_name, &off, &len);
>> +       if (ret == 3) {
>>                  /* Memory dereference case, e.g., -4@[sp, 96] */
>>                  arg->arg_type = USDT_ARG_REG_DEREF;
>>                  arg->val_off = off;
>> @@ -1361,32 +1363,37 @@ static int parse_usdt_arg(const char *arg_str, int arg_num, struct usdt_arg_spec
>>                  if (reg_off < 0)
>>                          return reg_off;
>>                  arg->reg_off = reg_off;
>> -       } else if (sscanf(arg_str, " %d @ \[ %m[a-z0-9] ] %n", &arg_sz, &reg_name, &len) == 2) {
>> -               /* Memory dereference case, e.g., -4@[sp] */
>> -               arg->arg_type = USDT_ARG_REG_DEREF;
>> -               arg->val_off = 0;
>> -               reg_off = calc_pt_regs_off(reg_name);
>> -               free(reg_name);
>> -               if (reg_off < 0)
>> -                       return reg_off;
>> -               arg->reg_off = reg_off;
>> -       } else if (sscanf(arg_str, " %d @ %ld %n", &arg_sz, &off, &len) == 2) {
>> -               /* Constant value case, e.g., 4@5 */
>> -               arg->arg_type = USDT_ARG_CONST;
>> -               arg->val_off = off;
>> -               arg->reg_off = 0;
>> -       } else if (sscanf(arg_str, " %d @ %m[a-z0-9] %n", &arg_sz, &reg_name, &len) == 2) {
>> -               /* Register read case, e.g., -8@x4 */
>> -               arg->arg_type = USDT_ARG_REG;
>> -               arg->val_off = 0;
>> -               reg_off = calc_pt_regs_off(reg_name);
>> -               free(reg_name);
>> -               if (reg_off < 0)
>> -                       return reg_off;
>> -               arg->reg_off = reg_off;
>>          } else {
>> -               pr_warn("usdt: unrecognized arg #%d spec '%s'\n", arg_num, arg_str);
>> -               return -EINVAL;
>> +               if (ret == 2)
>> +                       free(reg_name);
>> +
>> +               if (sscanf(arg_str, " %d @ \[ %m[a-z0-9] ] %n", &arg_sz, &reg_name, &len) == 2) {
>> +                       /* Memory dereference case, e.g., -4@[sp] */
>> +                       arg->arg_type = USDT_ARG_REG_DEREF;
>> +                       arg->val_off = 0;
>> +                       reg_off = calc_pt_regs_off(reg_name);
>> +                       free(reg_name);
>> +                       if (reg_off < 0)
>> +                               return reg_off;
>> +                       arg->reg_off = reg_off;
>> +               } else if (sscanf(arg_str, " %d @ %ld %n", &arg_sz, &off, &len) == 2) {
>> +                       /* Constant value case, e.g., 4@5 */
>> +                       arg->arg_type = USDT_ARG_CONST;
>> +                       arg->val_off = off;
>> +                       arg->reg_off = 0;
>> +               } else if (sscanf(arg_str, " %d @ %m[a-z0-9] %n", &arg_sz, &reg_name, &len) == 2) {
>> +                       /* Register read case, e.g., -8@x4 */
>> +                       arg->arg_type = USDT_ARG_REG;
>> +                       arg->val_off = 0;
>> +                       reg_off = calc_pt_regs_off(reg_name);
>> +                       free(reg_name);
>> +                       if (reg_off < 0)
>> +                               return reg_off;
>> +                       arg->reg_off = reg_off;
>> +               } else {
>> +                       pr_warn("usdt: unrecognized arg #%d spec '%s'\n", arg_num, arg_str);
>> +                       return -EINVAL;
>> +               }
>>          }
>>
> 
> I think all this is more complicated than it has to be. How big  can
> register names be? Few characters? Let's get rid of %m[a-z0-9] and
> instead use fixed-max-length strings, e.g., %5s. And read register
> names into such local char buffers. It will simplify everything
> tremendously. Let's use 16-byte buffers and use %15s to match it?
> Would that be enough?
> 

The valid register names accepted by calc_pt_regs_off() are x0~x31 and sp, so
16-byte buffer is enough. Since %15s matches all non-space characters, will use
%15[a-z0-9] to match it.

>>          arg->arg_signed = arg_sz < 0;
>> --
>> 2.30.2
>>
> .

Re: [PATCH bpf v3 4/6] selftest/bpf: Fix memory leak in kprobe_multi_test

[Xu Kuohai]

On 10/11/2022 9:34 AM, Andrii Nakryiko wrote:
> On Mon, Oct 10, 2022 at 7:08 AM Xu Kuohai <xukuohai@huawei.com> wrote:
>>
>> The get_syms() function in kprobe_multi_test.c does not free the string
>> memory allocated by sscanf correctly. Fix it.
>>
>> Fixes: 5b6c7e5c4434 ("selftests/bpf: Add attach bench test")
>> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
>> Acked-by: Jiri Olsa <jolsa@kernel.org>
>> ---
>>   .../bpf/prog_tests/kprobe_multi_test.c          | 17 ++++++++---------
>>   1 file changed, 8 insertions(+), 9 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
>> index d457a55ff408..07dd2c5b7f98 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
>> @@ -360,15 +360,14 @@ static int get_syms(char ***symsp, size_t *cntp)
>>                   * to them. Filter out the current culprits - arch_cpu_idle
>>                   * and rcu_* functions.
>>                   */
>> -               if (!strcmp(name, "arch_cpu_idle"))
>> -                       continue;
>> -               if (!strncmp(name, "rcu_", 4))
>> -                       continue;
>> -               if (!strcmp(name, "bpf_dispatcher_xdp_func"))
>> -                       continue;
>> -               if (!strncmp(name, "__ftrace_invalid_address__",
>> -                            sizeof("__ftrace_invalid_address__") - 1))
>> +               if (!strcmp(name, "arch_cpu_idle") ||
>> +                       !strncmp(name, "rcu_", 4) ||
>> +                       !strcmp(name, "bpf_dispatcher_xdp_func") ||
>> +                       !strncmp(name, "__ftrace_invalid_address__",
>> +                                sizeof("__ftrace_invalid_address__") - 1)) {
>> +                       free(name);
>>                          continue;
>> +               }
> 
> it seems cleaner if we add if (name) free(name) under error: goto
> label. And in the success case when we assign name to syms[cnt] we can
> reset name to NULL to avoid double-free. WDYT?
> 

Fine, but since free(NULL) works perfectly, will call free(name) unconditionally,
and also initialize name to NULL, and call free(name) before sscanf.

> 
>>                  err = hashmap__add(map, name, NULL);
>>                  if (err) {
>>                          free(name);
>> @@ -394,7 +393,7 @@ static int get_syms(char ***symsp, size_t *cntp)
>>          hashmap__free(map);
>>          if (err) {
>>                  for (i = 0; i < cnt; i++)
>> -                       free(syms[cnt]);
>> +                       free(syms[i]);
>>                  free(syms);
>>          }
>>          return err;
>> --
>> 2.30.2
>>
> .

Re: [PATCH bpf v3 0/6] Fix bugs found by ASAN when running selftests

[Xu Kuohai]

On 10/11/2022 9:37 AM, Andrii Nakryiko wrote:
> On Mon, Oct 10, 2022 at 7:08 AM Xu Kuohai <xukuohai@huawei.com> wrote:
>>
> 
> Thanks for the fixes! I left a few comments in a few patches, please
> address those. But also
> please provide a commit message, even if a single line one. Kernel
> style dictates that the commit message shouldn't be empty.
> 

Will do, thanks

> And I think none of these fixes are critical enough to go to bpf tree,
> please target bpf-next for next revision. Thanks.
> 

Ok, will target to bpf-next branch, targeting bpf tree just because
Documentation/bpf/bpf_devel_QA.rst says bpf-next is for features

> 
>> v3:
>> - Fix error failure of case test_xdp_adjust_tail_grow exposed by this series
>>
>> v2: https://lore.kernel.org/bpf/20221010070454.577433-1-xukuohai@huaweicloud.com
>> - Rebase and fix conflict
>>
>> v1: https://lore.kernel.org/bpf/20221009131830.395569-1-xukuohai@huaweicloud.com
>>
>> Xu Kuohai (6):
>>    libbpf: Fix use-after-free in btf_dump_name_dups
>>    libbpf: Fix memory leak in parse_usdt_arg()
>>    selftests/bpf: Fix memory leak caused by not destroying skeleton
>>    selftest/bpf: Fix memory leak in kprobe_multi_test
>>    selftests/bpf: Fix error failure of case test_xdp_adjust_tail_grow
>>    selftest/bpf: Fix error usage of ASSERT_OK in xdp_adjust_tail.c
>>
>>   tools/lib/bpf/btf_dump.c                      | 47 +++++++++++----
>>   tools/lib/bpf/usdt.c                          | 59 +++++++++++--------
>>   .../bpf/prog_tests/kprobe_multi_test.c        | 17 +++---
>>   .../selftests/bpf/prog_tests/map_kptr.c       |  3 +-
>>   .../selftests/bpf/prog_tests/tracing_struct.c |  3 +-
>>   .../bpf/prog_tests/xdp_adjust_tail.c          |  7 ++-
>>   6 files changed, 86 insertions(+), 50 deletions(-)
>>
>> --
>> 2.30.2
>>
> .