Re: [PATCH 1/5] Add dso__is_pie call to identify ELF PIE

[Leo Yan <leo.yan@linux.dev>]

Hi Steve,

On Tue, Aug 20, 2024 at 04:11:35PM -0600, Steve Clevenger wrote:
> From: "steve.c.clevenger.ampere" <scclevenger@os.amperecomputing.com>
> 
> Add dso__is_pie global to read the .dynamic section DT_FLAGS_1 entry for
> the DF_1_PIE flag. This identifies position executable code.
>  
> Signed-off-by: steve.c.clevenger.ampere <scclevenger@os.amperecomputing.com>
> ---
>  tools/perf/util/symbol-elf.c | 55 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> index e398abfd13a0..1d4bd222b314 100644
> --- a/tools/perf/util/symbol-elf.c
> +++ b/tools/perf/util/symbol-elf.c
> @@ -662,6 +662,61 @@ static int dso__synthesize_plt_got_symbols(struct dso *dso, Elf *elf,
>  	return err;
>  }
>  
> +/*
> + * Check dynamic section DT_FLAGS_1 for a Position Independent
> + * Executable (PIE).
> + */
> +bool dso__is_pie(struct dso *dso)
> +{
> +	Elf *elf = NULL;
> +	Elf_Scn *scn = NULL;
> +	GElf_Ehdr ehdr;
> +	GElf_Shdr shdr;
> +	bool is_pie = false;
> +	char dso_path[PATH_MAX];
> +	int fd = -1;
> +
> +	if (!dso || (elf_version(EV_CURRENT) == EV_NONE))
> +		return is_pie;	// false
> +
> +	dso__build_id_filename(dso, dso_path, sizeof(dso_path), false);
> +
> +	fd = open(dso_path, O_RDONLY);
> +
> +	if (fd < 0) {
> +		pr_debug("%s: cannot read cached %s.\n", __func__, dso_path);
> +		return is_pie;	// false
> +	}
> +
> +	elf = elf_begin(fd, ELF_C_READ, NULL);
> +	gelf_getehdr(elf, &ehdr);
> +
> +	if (ehdr.e_type == ET_DYN) {

The code looks good to me, just several nitpicks.

To avoid indentation, we can firstly check the failure case and directly
exit for it.

        if (ehdr.e_type != ET_DYN)
                goto exit_elf_end;

> +		scn = elf_section_by_name(elf, &ehdr, &shdr, ".dynamic", NULL);

Ditto.

        if (!scn)
                goto exit_elf_end;

> +		if (scn) {	// check DT_FLAGS_1
> +			Elf_Data *data;
> +			GElf_Dyn *entry;
> +			int n_entries = shdr.sh_size / sizeof(GElf_Dyn);
> +
> +			data = (Elf_Data *) elf_getdata(scn, NULL);

For a safe code, it is good to check if pointers (data and
data->d_buf) are valid before dereference them.

       if (!data || !data->d_buf)
               goto exit_elf_end;

With above changes:

Reviewed-by: Leo Yan <leo.yan@arm.com>

> +			for (int i = 0; i < n_entries; i++) {
> +				entry = ((GElf_Dyn *) data->d_buf) + i;
> +				if (entry->d_tag == DT_FLAGS_1) {
> +					if ((entry->d_un.d_val & DF_1_PIE) != 0) {
> +						is_pie = true;
> +						break;
> +					}
> +				}
> +			} // end for
> +		}
> +	}
> +
> +	elf_end(elf);
> +	close(fd);
> +
> +	return is_pie;
> +}
> +
>  /*
>   * We need to check if we have a .dynsym, so that we can handle the
>   * .plt, synthesizing its symbols, that aren't on the symtabs (be it
> -- 
> 2.25.1
> 
>