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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 11 Oct 2022 14:25:57 +0800 From: Xu Kuohai <xukuohai@...wei.com> To: Andrii Nakryiko <andrii.nakryiko@...il.com> CC: <bpf@...r.kernel.org>, <linux-kernel@...r.kernel.org>, <linux-kselftest@...r.kernel.org>, <netdev@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>, Martin KaFai Lau <martin.lau@...ux.dev>, Song Liu <song@...nel.org>, Yonghong Song <yhs@...com>, John Fastabend <john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>, Stanislav Fomichev <sdf@...gle.com>, Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>, Mykola Lysenko <mykolal@...com>, Shuah Khan <shuah@...nel.org>, "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, Jesper Dangaard Brouer <hawk@...nel.org>, Kumar Kartikeya Dwivedi <memxor@...il.com>, Alan Maguire <alan.maguire@...cle.com>, Delyan Kratunov <delyank@...com>, Lorenzo Bianconi <lorenzo@...nel.org> Subject: Re: [PATCH bpf v3 1/6] libbpf: Fix use-after-free in btf_dump_name_dups On 10/11/2022 9:32 AM, Andrii Nakryiko wrote: > On Mon, Oct 10, 2022 at 7:08 AM Xu Kuohai <xukuohai@...wei.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@...wei.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 >> > .
Powered by blists - more mailing lists