[PATCH] src/readelf.c: Access symbol and version data only if available

[Mark Wielaard]

Hi Aaron, Hi Constantine,

On Thu, 2025-04-10 at 10:52 -0400, Aaron Merey wrote:
> handle_dynamic_symtab can attempt to read symbol and version data from
> file offset 0 if the associated DT_ tags aren't found.
> 
> Fix this by only reading symbol and version data when non-zero file
> offsets have been found.
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=32864

So I think the basic observation that elf_getdata_rawchunk happily
returns a chunk of the ELF file starting at offset zero (which is
basically never what you want) is correct. Given that symdata,
symstrdata, versym_data, verdef_data and verneed_data are initialized
to NULL, checking for the offs index to not be zero before trying to
call elf_getdata_rawchunk makes sense.

It is a good update.

But note that the code itself isn't very robust against bad input. Both
verneed_data->d_buf and verdef_data->d_buf are used without checking
whether verneed_data or verdef_data is NULL. At least against bad input
data. It looks to me as if addrs[i_verdefnum] (DT_VERDEFNUM) and
addrs[i_verneednum] (DT_VERNEEDNUM) could be set without
offs[i_verneed] or offs[iverdef] being set. So maybe explicitly also
check that (or file a bug report to check that in the future)?

Thanks,

Mark

> Suggested-by: Constantine Bytensky <cbytensky@gmail.com>
> Signed-off-by: Aaron Merey <amerey@redhat.com>
> ---
>  src/readelf.c | 50 +++++++++++++++++++++++++++++++-------------------
>  1 file changed, 31 insertions(+), 19 deletions(-)
> 
> diff --git a/src/readelf.c b/src/readelf.c
> index 5b0e7b47..0452977f 100644
> --- a/src/readelf.c
> +++ b/src/readelf.c
> @@ -3047,18 +3047,25 @@ handle_dynamic_symtab (Ebl *ebl)
>    Elf_Data *verdef_data = NULL;
>    Elf_Data *verneed_data = NULL;
>  
> -  symdata = elf_getdata_rawchunk (
> -      ebl->elf, offs[i_symtab],
> -      gelf_fsize (ebl->elf, ELF_T_SYM, syments, EV_CURRENT), ELF_T_SYM);
> -  symstrdata = elf_getdata_rawchunk (ebl->elf, offs[i_strtab], addrs[i_strsz],
> -                                     ELF_T_BYTE);
> -  versym_data = elf_getdata_rawchunk (
> -      ebl->elf, offs[i_versym], syments * sizeof (Elf64_Half), ELF_T_HALF);
> +  if (offs[i_symtab] != 0)
> +    symdata = elf_getdata_rawchunk (
> +	ebl->elf, offs[i_symtab],
> +	gelf_fsize (ebl->elf, ELF_T_SYM, syments, EV_CURRENT), ELF_T_SYM);
> +
> +  if (offs[i_strtab] != 0)
> +    symstrdata = elf_getdata_rawchunk (ebl->elf, offs[i_strtab], addrs[i_strsz],
> +				       ELF_T_BYTE);
> +
> +  if (offs[i_versym] != 0)
> +    versym_data = elf_getdata_rawchunk (
> +	ebl->elf, offs[i_versym], syments * sizeof (Elf64_Half), ELF_T_HALF);
>  
>    /* Get the verneed_data without vernaux.  */
> -  verneed_data = elf_getdata_rawchunk (
> -      ebl->elf, offs[i_verneed], addrs[i_verneednum] * sizeof (Elf64_Verneed),
> -      ELF_T_VNEED);
> +  if (offs[i_verneed] != 0)
> +    verneed_data = elf_getdata_rawchunk (
> +	ebl->elf, offs[i_verneed], addrs[i_verneednum] * sizeof (Elf64_Verneed),
> +	ELF_T_VNEED);
> +
>    size_t vernauxnum = 0;
>    size_t vn_next_offset = 0;
>  
> @@ -3071,14 +3078,18 @@ handle_dynamic_symtab (Ebl *ebl)
>      }
>  
>    /* Update the verneed_data to include the vernaux.  */
> -  verneed_data = elf_getdata_rawchunk (
> -      ebl->elf, offs[i_verneed],
> -      (addrs[i_verneednum] + vernauxnum) * sizeof (GElf_Verneed), ELF_T_VNEED);
> +  if (offs[i_verneed] != 0)
> +    verneed_data = elf_getdata_rawchunk (
> +	ebl->elf, offs[i_verneed],
> +	(addrs[i_verneednum] + vernauxnum) * sizeof (GElf_Verneed),
> +	ELF_T_VNEED);
>  
>    /* Get the verdef_data without verdaux.  */
> -  verdef_data = elf_getdata_rawchunk (
> -      ebl->elf, offs[i_verdef], addrs[i_verdefnum] * sizeof (Elf64_Verdef),
> -      ELF_T_VDEF);
> +  if (offs[i_verdef] != 0)
> +    verdef_data = elf_getdata_rawchunk (
> +	ebl->elf, offs[i_verdef], addrs[i_verdefnum] * sizeof (Elf64_Verdef),
> +	ELF_T_VDEF);
> +
>    size_t verdauxnum = 0;
>    size_t vd_next_offset = 0;
>  
> @@ -3091,9 +3102,10 @@ handle_dynamic_symtab (Ebl *ebl)
>      }
>  
>    /* Update the verdef_data to include the verdaux.  */
> -  verdef_data = elf_getdata_rawchunk (
> -      ebl->elf, offs[i_verdef],
> -      (addrs[i_verdefnum] + verdauxnum) * sizeof (GElf_Verdef), ELF_T_VDEF);
> +  if (offs[i_verdef] != 0)
> +    verdef_data = elf_getdata_rawchunk (
> +	ebl->elf, offs[i_verdef],
> +	(addrs[i_verdefnum] + verdauxnum) * sizeof (GElf_Verdef), ELF_T_VDEF);
>  
>    unsigned int nsyms = (unsigned int)syments;
>    process_symtab (ebl, nsyms, 0, 0, 0, symdata, versym_data, symstrdata,