Hi Aaron, Hi Constantine,
On Mon, 2025-05-05 at 10:44 -0400, Aaron Merey wrote:
> handle_dynamic_symtab can attempt to read symbol and version data from
> file offset of 0 or address of 0 if the associated DT_ tags aren't found.
>
> Fix this by only reading symbol and version data when non-zero file
> offsets/addresses have been found.
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=32864
>
> Suggested-by: Constantine Bytensky <cbyten...@gmail.com>
> Signed-off-by: Aaron Merey <ame...@redhat.com>
> ---
> v2 changes: Added checks for unset addrs and missing symbol version data.
>
> src/readelf.c | 82 ++++++++++++++++++++++++++++++---------------------
> 1 file changed, 49 insertions(+), 33 deletions(-)
>
> diff --git a/src/readelf.c b/src/readelf.c
> index 8603b3c4..e9df0344 100644
> --- a/src/readelf.c
> +++ b/src/readelf.c
> @@ -3047,53 +3047,69 @@ 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 && addrs[i_strsz] != 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 && addrs[i_verneednum] != 0)
> + verneed_data = elf_getdata_rawchunk (
> + ebl->elf, offs[i_verneed], addrs[i_verneednum] * sizeof (Elf64_Verneed),
> + ELF_T_VNEED);
> +
All looks good.
> size_t vernauxnum = 0;
> size_t vn_next_offset = 0;
>
> - for (size_t i = 0; i < addrs[i_verneednum]; i++)
> - {
> - GElf_Verneed *verneed
> - = (GElf_Verneed *)(verneed_data->d_buf + vn_next_offset);
> - vernauxnum += verneed->vn_cnt;
> - vn_next_offset += verneed->vn_next;
> - }
> + if (verneed_data != NULL && verneed_data->d_buf != NULL
> + && addrs[i_verneednum] != 0)
> + for (size_t i = 0; i < addrs[i_verneednum]; i++)
> + {
> + GElf_Verneed *verneed
> + = (GElf_Verneed *)(verneed_data->d_buf + vn_next_offset);
> + vernauxnum += verneed->vn_cnt;
> + vn_next_offset += verneed->vn_next;
> + }
The last && addrs[i_verneednum] != 0 is ok, but redundant because of
the i < addrs[i_verneednum] (which would immediately succeed and so
also skips the loop).
But if we are going to make this robust then there is another (pre-
existing) issue. We keep reading the data from the d_buf as a
GElf_Verneed and use verneed->vn_next to give us the next offset. We
need to check if that is sane. First we need to check verneed_data-
>d_size is at least sizeof (GElf_Verneed) and then vn_next_offset must
always be smaller than verneed_data->d_size - sizeof (GElf_Verneed).
> /* 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 && addrs[i_verneednum] != 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 && addrs[i_verdefnum] != 0)
> + verdef_data = elf_getdata_rawchunk (
> + ebl->elf, offs[i_verdef], addrs[i_verdefnum] * sizeof (Elf64_Verdef),
> + ELF_T_VDEF);
> +
Looks good.
> size_t verdauxnum = 0;
> size_t vd_next_offset = 0;
>
> - for (size_t i = 0; i < addrs[i_verdefnum]; i++)
> - {
> - GElf_Verdef *verdef
> - = (GElf_Verdef *)(verdef_data->d_buf + vd_next_offset);
> - verdauxnum += verdef->vd_cnt;
> - vd_next_offset += verdef->vd_next;
> - }
> + if (verdef_data != NULL && verdef_data->d_buf != NULL
> + && addrs[i_verdefnum] != 0)
> + for (size_t i = 0; i < addrs[i_verdefnum]; i++)
> + {
> + GElf_Verdef *verdef
> + = (GElf_Verdef *)(verdef_data->d_buf + vd_next_offset);
> + verdauxnum += verdef->vd_cnt;
> + vd_next_offset += verdef->vd_next;
> + }
Same as above, but now for sizeof (GElf_Verdef).
> /* 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 && addrs[i_verdefnum] != 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,
Looks good.
Cheers,
Mark
