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

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

[Andrii Nakryiko]

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@gmail.com>
Reported-by: Liam Wisehart <liamwisehart@meta.com>
Fixes: bb7fa09399b9 ("libbpf: Support symbol versioning for uprobe")
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/lib/bpf/elf.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

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

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

[Andrii Nakryiko]

On Mon, Oct 16, 2023 at 11:28 AM Andrii Nakryiko <andrii@kernel.org> 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@gmail.com>
> Reported-by: Liam Wisehart <liamwisehart@meta.com>
> Fixes: bb7fa09399b9 ("libbpf: Support symbol versioning for uprobe")
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>  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:

  [ 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!

> 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
>

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

[Hengqi Chen]

+ Fangrui

On Tue, Oct 17, 2023 at 4:10 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Mon, Oct 16, 2023 at 11:28 AM Andrii Nakryiko <andrii@kernel.org> 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@gmail.com>
> > Reported-by: Liam Wisehart <liamwisehart@meta.com>
> > Fixes: bb7fa09399b9 ("libbpf: Support symbol versioning for uprobe")
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
> >  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@gmail.com>

--
Hengqi

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

[Fangrui Song]

On 2023-10-17, Hengqi Chen wrote:
>+ Fangrui

Thanks for CCing me. I have spent countless hours studying symbol
versioning...
https://maskray.me/blog/2020-11-26-all-about-symbol-versioning

>On Tue, Oct 17, 2023 at 4:10 AM Andrii Nakryiko
><andrii.nakryiko@gmail.com> wrote:
>>
>> On Mon, Oct 16, 2023 at 11:28 AM Andrii Nakryiko <andrii@kernel.org> 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@gmail.com>
>> > Reported-by: Liam Wisehart <liamwisehart@meta.com>
>> > Fixes: bb7fa09399b9 ("libbpf: Support symbol versioning for uprobe")
>> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
>> > ---
>> >  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.

Yes.

The .gnu.version table assigns a version index to each .dynsym entry. An
entry (version ID) corresponds to a Index: entry in .gnu.version_d or a
Version: entry in .gnu.version_r.

>>   [ 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.

Yes.

>> > 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@gmail.com>

Looks good to me, too.

Review Reviewed-by: Fangrui Song <maskray@google.com>

---

I have a question about a previous patch
"libbpf: Support symbol versioning for uprobe"
(commit bb7fa09399b937cdc4432ac99f9748f5a7f69389 in next/master).

In the function 'symbol_match',

	/* If user specifies symbol version, for dynamic symbols,
	 * get version name from ELF verdef section for comparison.
	 */
	if (sh_type == SHT_DYNSYM) {
		ver_name = elf_get_vername(iter, sym->ver);
		if (!ver_name)
			return false;
		return strcmp(ver_name, lib_ver) == 0;
	}

elf_get_vername only checks verdef, not verneed. Is this an issue?
I am not familiar with tools/lib/bpf or how it is used for uprobe.


Is the function intended to match linker behavior?
Then the rules described at https://maskray.me/blog/2020-11-26-all-about-symbol-versioning#linker-behavior
apply.
I think the current rules are quite good.


>--
>Hengqi

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

[Hengqi Chen]

On Tue, Oct 17, 2023 at 12:06 PM Fangrui Song <maskray@google.com> wrote:
>
> On 2023-10-17, Hengqi Chen wrote:
> >+ Fangrui
>
> Thanks for CCing me. I have spent countless hours studying symbol
> versioning...
> https://maskray.me/blog/2020-11-26-all-about-symbol-versioning
>
> >On Tue, Oct 17, 2023 at 4:10 AM Andrii Nakryiko
> ><andrii.nakryiko@gmail.com> wrote:
> >>
> >> On Mon, Oct 16, 2023 at 11:28 AM Andrii Nakryiko <andrii@kernel.org> 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@gmail.com>
> >> > Reported-by: Liam Wisehart <liamwisehart@meta.com>
> >> > Fixes: bb7fa09399b9 ("libbpf: Support symbol versioning for uprobe")
> >> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> >> > ---
> >> >  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.
>
> Yes.
>
> The .gnu.version table assigns a version index to each .dynsym entry. An
> entry (version ID) corresponds to a Index: entry in .gnu.version_d or a
> Version: entry in .gnu.version_r.
>
> >>   [ 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.
>
> Yes.
>
> >> > 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@gmail.com>
>
> Looks good to me, too.
>
> Review Reviewed-by: Fangrui Song <maskray@google.com>
>
> ---
>
> I have a question about a previous patch
> "libbpf: Support symbol versioning for uprobe"
> (commit bb7fa09399b937cdc4432ac99f9748f5a7f69389 in next/master).
>
> In the function 'symbol_match',
>
>         /* If user specifies symbol version, for dynamic symbols,
>          * get version name from ELF verdef section for comparison.
>          */
>         if (sh_type == SHT_DYNSYM) {
>                 ver_name = elf_get_vername(iter, sym->ver);
>                 if (!ver_name)
>                         return false;
>                 return strcmp(ver_name, lib_ver) == 0;
>         }
>
> elf_get_vername only checks verdef, not verneed. Is this an issue?
> I am not familiar with tools/lib/bpf or how it is used for uprobe.
>
>

We are dealing with symbols defined in an ELF object,  not the shared libraries
it refers to. So I guess we don't need to handle verneed.

> Is the function intended to match linker behavior?
> Then the rules described at https://maskray.me/blog/2020-11-26-all-about-symbol-versioning#linker-behavior
> apply.
> I think the current rules are quite good.
>
>
> >--
> >Hengqi

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

[Manu Bretelle]

On Mon, Oct 16, 2023 at 11:28:40AM -0700, Andrii Nakryiko 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@gmail.com>
> Reported-by: Liam Wisehart <liamwisehart@meta.com>
> Fixes: bb7fa09399b9 ("libbpf: Support symbol versioning for uprobe")
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---

This was applied and tested internally and fixed the issue.

Tested-by: Manu Bretelle <chantr4@gmail.com>

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Mon, 16 Oct 2023 11:28:40 -0700 you 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.
> 
> [...]

Here is the summary with links:
  - [bpf-next] libbpf: don't assume SHT_GNU_verdef presence for SHT_GNU_versym section
    https://git.kernel.org/bpf/bpf-next/c/137df1189d12

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html