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