Re: [PATCH bpf-next] libbpf: don't assume SHT_GNU_verdef presence for SHT_GNU_versym section

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



+ Fangrui

On Tue, Oct 17, 2023 at 4:10 AM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Mon, Oct 16, 2023 at 11:28 AM Andrii Nakryiko <andrii@xxxxxxxxxx> wrote:
> >
> > Fix too eager assumption that SHT_GNU_verdef ELF section is going to be
> > present whenever binary has SHT_GNU_versym section. It seems like either
> > SHT_GNU_verdef or SHT_GNU_verneed can be used, so failing on missing
> > SHT_GNU_verdef actually breaks use cases in production.
> >
> > One specific reported issue, which was used to manually test this fix,
> > was trying to attach to `readline` function in BASH binary.
> >
> > Cc: Hengqi Chen <hengqi.chen@xxxxxxxxx>
> > Reported-by: Liam Wisehart <liamwisehart@xxxxxxxx>
> > Fixes: bb7fa09399b9 ("libbpf: Support symbol versioning for uprobe")
> > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > ---
> >  tools/lib/bpf/elf.c | 16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)
> >
>
> Hengqi,
>
> Please take a look when you get a chance. I'm not very familiar with
> symbol versioning details, but it seems like we made a too strong
> assumption about verdef always being present. In bash's case we have
> VERNEED, but not VERDEF, and that seems to be ok:
>

Yes, both VERNEED and VERDEF are optional.

>   [ 8] .gnu.version      VERSYM          000000000001c9ca 01c9ca
> 00130c 02   A  6   0  2
>   [ 9] .gnu.version_r    VERNEED         000000000001dcd8 01dcd8
> 0000b0 00   A  7   2  8
>
> So perhaps we need to complete the implementation to take VERNEED into
> account. And also let's add a test that can catch an issue like this
> going forward. Thanks!
>

AFAIK, VERNEED contains version requirements for shared libraries.

> > diff --git a/tools/lib/bpf/elf.c b/tools/lib/bpf/elf.c
> > index 2a158e8a8b7c..2a62bf411bb3 100644
> > --- a/tools/lib/bpf/elf.c
> > +++ b/tools/lib/bpf/elf.c
> > @@ -141,14 +141,15 @@ static int elf_sym_iter_new(struct elf_sym_iter *iter,
> >         iter->versyms = elf_getdata(scn, 0);
> >
> >         scn = elf_find_next_scn_by_type(elf, SHT_GNU_verdef, NULL);
> > -       if (!scn) {
> > -               pr_debug("elf: failed to find verdef ELF sections in '%s'\n", binary_path);
> > -               return -ENOENT;
> > -       }
> > -       if (!gelf_getshdr(scn, &sh))
> > +       if (!scn)
> > +               return 0;
> > +
> > +       iter->verdefs = elf_getdata(scn, 0);
> > +       if (!iter->verdefs || !gelf_getshdr(scn, &sh)) {
> > +               pr_warn("elf: failed to get verdef ELF section in '%s'\n", binary_path);
> >                 return -EINVAL;
> > +       }
> >         iter->verdef_strtabidx = sh.sh_link;
> > -       iter->verdefs = elf_getdata(scn, 0);
> >
> >         return 0;
> >  }
> > @@ -199,6 +200,9 @@ static const char *elf_get_vername(struct elf_sym_iter *iter, int ver)
> >         GElf_Verdef verdef;
> >         int offset;
> >
> > +       if (!iter->verdefs)
> > +               return NULL;
> > +
> >         offset = 0;
> >         while (gelf_getverdef(iter->verdefs, offset, &verdef)) {
> >                 if (verdef.vd_ndx != ver) {
> > --
> > 2.34.1
> >

Anyway, this change look good to me, so

Acked-by: Hengqi Chen <hengqi.chen@xxxxxxxxx>

--
Hengqi





[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux