Date: Mon, 30 Nov 2015 20:19:27 +0900
From: Namhyung Kim <namhyung@kernel.org>
To: Wang Nan <wangnan0@huawei.com>
Cc: acme@redhat.com, linux-kernel@vger.kernel.org, pi3orama@163.com,
        lizefen@huawei.com
Subject: Re: [PATCH] tools lib bpf: Fetch map names from correct strtab
Message-ID: <20151130111927.GA15670@danjae.kornet>
References: <3ABFD871-09F8-4C21-9EF7-06FCECA2E106@gmail.com>
 <1448879950-226288-1-git-send-email-wangnan0@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <1448879950-226288-1-git-send-email-wangnan0@huawei.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4480
Lines: 138

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 <namhyung@kernel.org>
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>

Acked-by: Namhyung Kim <namhyung@kernel.org>

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
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/