Re: perf-top: heap-buffer-overflow in elf_sec__is_text reported from ASan

[Jiri Slaby <jirislaby@kernel.org>]

Hi,

On 18. 06. 21, 11:17, Riccardo Mancini wrote:
> ASan reports a heap-buffer-overflow in elf_sec__is_text when using perf-top.
> The bug is introduced by commit 6833e0b: "perf symbols: Resolve symbols against
> debug file first" from Jiri Slaby.
...
> pwndbg> p syms_ss->name
> $4 = 0x607000018f90 "/usr/lib/debug/usr/lib64/libglib-2.0.so.0.6800.2-2.68.2-1.fc34.x86_64.debug"
> pwndbg> p runtime_ss->name
> $5 = 0x6070000190e0 "/root/.debug/.build-id/37/475e3b392fb3971c8ad0d9ac0a4d7e1b93c521/elf"

Out of curiosity, could you post output of 'readelf -S' on both of them?

> Furthermore, the branch in line symbol-elf.c:1241 (the one added in the referred
> patch) is not taken.
> 
> As you can see, sh_name is out-of-range (342 > 332).
> I can also provide a coredump, if it can be useful.
> 
> I have no idea of how the ELF stuff works, but I thought this may be caused by
> the fact that secstrs is built from runtime_ss, while shdr is built from syms_ss
> (since it is the change of the commit). I tried to test this theory with the
> following change:
> 
> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> index a73345730ba9..8d2b692f11a2 100644
> --- a/tools/perf/util/symbol-elf.c
> +++ b/tools/perf/util/symbol-elf.c
> @@ -1146,7 +1146,7 @@ int dso__load_sym(struct dso *dso, struct map *map, struct symsrc *syms_ss,
>   	if (symstrs == NULL)
>   		goto out_elf_end;
>   
> -	sec_strndx = elf_getscn(runtime_ss->elf, runtime_ss->ehdr.e_shstrndx);
> +	sec_strndx = elf_getscn(elf, ehdr.e_shstrndx);
>   	if (sec_strndx == NULL)
>   		goto out_elf_end;
>   
> @@ -1244,6 +1244,14 @@ int dso__load_sym(struct dso *dso, struct map *map, struct symsrc *syms_ss,
>   		 * values for syms (invalid) and runtime (valid).
>   		 */
>   		if (shdr.sh_type == SHT_NOBITS) {
> +			sec_strndx = elf_getscn(runtime_ss->elf, runtime_ss->ehdr.e_shstrndx);
> +			if (sec_strndx == NULL)
> +				goto out_elf_end;
> +
> +			secstrs = elf_getdata(sec_strndx, NULL);
> +			if (secstrs == NULL)
> +				goto out_elf_end;
> +
>   			sec = elf_getscn(runtime_ss->elf, sym.st_shndx);
>   			if (!sec)
>   				goto out_elf_end;
> 
> However, it still overflows, but oddly the branch is not taken before the
> overflow. Is there some kind of state that gets changed in the ELF structs?

Something like that should work. But you should reset sec_strndx also in 
the else branch, IMO. So what about this?

--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -1081,7 +1081,7 @@ int dso__load_sym(struct dso *dso, struct map 
*map, struct symsrc *syms_ss,
         struct maps *kmaps = kmap ? map__kmaps(map) : NULL;
         struct map *curr_map = map;
         struct dso *curr_dso = dso;
-       Elf_Data *symstrs, *secstrs;
+       Elf_Data *symstrs, *secstrs, *secstrs_run, *secstrs_sym;
         uint32_t nr_syms;
         int err = -1;
         uint32_t idx;
@@ -1150,8 +1150,16 @@ int dso__load_sym(struct dso *dso, struct map 
*map, struct symsrc *syms_ss,
         if (sec_strndx == NULL)
                 goto out_elf_end;

-       secstrs = elf_getdata(sec_strndx, NULL);
-       if (secstrs == NULL)
+       secstrs_run = elf_getdata(sec_strndx, NULL);
+       if (secstrs_run == NULL)
+               goto out_elf_end;
+
+       sec_strndx = elf_getscn(elf, ehdr.e_shstrndx);
+       if (sec_strndx == NULL)
+               goto out_elf_end;
+
+       secstrs_sym = elf_getdata(sec_strndx, NULL);
+       if (secstrs_sym == NULL)
                 goto out_elf_end;

         nr_syms = shdr.sh_size / shdr.sh_entsize;
@@ -1237,6 +1245,8 @@ int dso__load_sym(struct dso *dso, struct map 
*map, struct symsrc *syms_ss,

                 gelf_getshdr(sec, &shdr);

+               secstrs = secstrs_sym;
+
                 /*
                  * We have to fallback to runtime when syms' section 
header has
                  * NOBITS set. NOBITS results in file offset 
(sh_offset) not
@@ -1249,6 +1259,7 @@ int dso__load_sym(struct dso *dso, struct map 
*map, struct symsrc *syms_ss,
                                 goto out_elf_end;

                         gelf_getshdr(sec, &shdr);
+                       secstrs = secstrs_run;
                 }

                 if (is_label && !elf_sec__filter(&shdr, secstrs))


thanks,
-- 
js

-- 
js
suse labs