by Namhyung Kim

[permalink] [raw]

Subject: Re: [PATCH] tools lib bpf: Fetch map names from correct strtab

On Mon, Nov 30, 2015 at 10:39:10AM +0000, Wang Nan wrote:
> Namhyung Kim pointed out a potential problem in original code that it
> fetches names of maps from section header string table, which is used
> to store section names.
>
> Original code doesn't cause error because of a LLVM behavior that, it
> combines shstrtab into strtab. For example:
>
> $ echo 'int func() {return 0;}' | x86_64-oe-linux-clang -x c -o temp.o -c -
> $ readelf -h ./temp.o
> ELF Header:
> Magic: 7f 45 4c 46 02 01 01 03 00 00 00 00 00 00 00 00
> ...
> Section header string table index: 1
> $ readelf -S ./temp.o
> There are 10 section headers, starting at offset 0x288:
>
> Section Headers:
> [Nr] Name Type Address Offset
> Size EntSize Flags Link Info Align
> [ 0] NULL 0000000000000000 00000000
> 0000000000000000 0000000000000000 0 0 0
> [ 1] .strtab STRTAB 0000000000000000 00000230
> 0000000000000051 0000000000000000 0 0 1
> ...
> $ readelf -p .strtab ./temp.o
>
> String dump of section '.strtab':
> [ 1] .text
> [ 7] .comment
> [ 10] .bss
> [ 15] .note.GNU-stack
> [ 25] .rela.eh_frame
> [ 34] func
> [ 39] .strtab
> [ 41] .symtab
> [ 49] .data
> [ 4f] -
>
> $ readelf -p .shstrtab ./temp.o
> readelf: Warning: Section '.shstrtab' was not dumped because it does not exist!
>
> Where, 'section header string table index' points to '.strtab', and
> symbol names are also stored there.
>
> However, in case of gcc:
>
> $ echo 'int func() {return 0;}' | gcc -x c -o temp.o -c -
> $ readelf -p .shstrtab ./temp.o
>
> String dump of section '.shstrtab':
> [ 1] .symtab
> [ 9] .strtab
> [ 11] .shstrtab
> [ 1b] .text
> [ 21] .data
> [ 27] .bss
> [ 2c] .comment
> [ 35] .note.GNU-stack
> [ 45] .rela.eh_frame
> $ readelf -p .strtab ./temp.o
>
> String dump of section '.strtab':
> [ 1] func
>
> They are separated sections.
>
> Although original code doesn't cause error, we'd better use canonical
> method for fetching symbol names to avoid potential behavior changing.
> This patch learns from readelf's code, fetches string from sh_link
> of .symbol section.
>
> Reported-by: Namhyung Kim <[email protected]>
> Signed-off-by: Wang Nan <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>

Acked-by: Namhyung Kim <[email protected]>

Thanks,
Namhyung

> ---
> tools/lib/bpf/libbpf.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 16485ab..8334a5a 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -195,6 +195,7 @@ struct bpf_object {
> Elf *elf;
> GElf_Ehdr ehdr;
> Elf_Data *symbols;
> + size_t strtabidx;
> struct {
> GElf_Shdr shdr;
> Elf_Data *data;
> @@ -547,7 +548,7 @@ bpf_object__init_maps_name(struct bpf_object *obj, int maps_shndx)
> continue;
>
> map_name = elf_strptr(obj->efile.elf,
> - obj->efile.ehdr.e_shstrndx,
> + obj->efile.strtabidx,
> sym.st_name);
> map_idx = sym.st_value / sizeof(struct bpf_map_def);
> if (map_idx >= obj->nr_maps) {
> @@ -630,8 +631,10 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
> pr_warning("bpf: multiple SYMTAB in %s\n",
> obj->path);
> err = -LIBBPF_ERRNO__FORMAT;
> - } else
> + } else {
> obj->efile.symbols = data;
> + obj->efile.strtabidx = sh.sh_link;
> + }
> } else if ((sh.sh_type == SHT_PROGBITS) &&
> (sh.sh_flags & SHF_EXECINSTR) &&
> (data->d_size > 0)) {
> @@ -667,6 +670,10 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
> goto out;
> }
>
> + if (!obj->efile.strtabidx || obj->efile.strtabidx >= idx) {
> + pr_warning("Corrupted ELF file: index of strtab invalid\n");
> + return LIBBPF_ERRNO__FORMAT;
> + }
> if (maps_shndx >= 0)
> err = bpf_object__init_maps_name(obj, maps_shndx);
> out:
> --
> 1.8.3.4
>