lkml.org 
[lkml]   [2022]   [Oct]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH bpf v3 1/6] libbpf: Fix use-after-free in btf_dump_name_dups
From
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
>>
> .

\
 
 \ /
  Last update: 2022-10-11 08:27    [W:0.053 / U:1.212 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site