[bpf-next] libbpf: Fix segfault due to libelf functions not setting errno

Commit Message

Quentin Monnet Dec. 5, 2024, 1:59 p.m. UTC

Libelf functions do not set errno on failure. Instead, it relies on its
internal _elf_errno value, that can be retrieved via elf_errno (or the
corresponding message via elf_errmsg()). From "man libelf":

    If a libelf function encounters an error it will set an internal
    error code that can be retrieved with elf_errno. Each thread
    maintains its own separate error code. The meaning of each error
    code can be determined with elf_errmsg, which returns a string
    describing the error.

As a consequence, libbpf should not return -errno when a function from
libelf fails, because an empty value will not be interpreted as an error
and won't prevent the program to stop. This is visible in
bpf_linker__add_file(), for example, where we call a succession of
functions that rely on libelf:

    err = err ?: linker_load_obj_file(linker, filename, opts, &obj);
    err = err ?: linker_append_sec_data(linker, &obj);
    err = err ?: linker_append_elf_syms(linker, &obj);
    err = err ?: linker_append_elf_relos(linker, &obj);
    err = err ?: linker_append_btf(linker, &obj);
    err = err ?: linker_append_btf_ext(linker, &obj);

If the object file that we try to process is not, in fact, a correct
object file, linker_load_obj_file() may fail with errno not being set,
and return 0. In this case we attempt to run linker_append_elf_sysms()
and may segfault.

This can happen (and was discovered) with bpftool:

    $ bpftool gen object output.o sample_ret0.bpf.c
    libbpf: failed to get ELF header for sample_ret0.bpf.c: invalid `Elf' handle
    zsh: segmentation fault (core dumped)  bpftool gen object output.o sample_ret0.bpf.c

Fix the issue by returning a non-null error code (-EINVAL) when libelf
functions fail.

Fixes: faf6ed321cf6 ("libbpf: Add BPF static linker APIs")
Signed-off-by: Quentin Monnet <[e-mail]>
---
 tools/lib/bpf/linker.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

Comments

Andrii Nakryiko Dec. 5, 2024, 9:46 p.m. UTC | #1

On Thu, Dec 5, 2024 at 5:59 AM Quentin Monnet <[e-mail]> wrote:
>
> Libelf functions do not set errno on failure. Instead, it relies on its
> internal _elf_errno value, that can be retrieved via elf_errno (or the
> corresponding message via elf_errmsg()). From "man libelf":
>
>     If a libelf function encounters an error it will set an internal
>     error code that can be retrieved with elf_errno. Each thread
>     maintains its own separate error code. The meaning of each error
>     code can be determined with elf_errmsg, which returns a string
>     describing the error.
>
> As a consequence, libbpf should not return -errno when a function from
> libelf fails, because an empty value will not be interpreted as an error
> and won't prevent the program to stop. This is visible in
> bpf_linker__add_file(), for example, where we call a succession of
> functions that rely on libelf:
>
>     err = err ?: linker_load_obj_file(linker, filename, opts, &obj);
>     err = err ?: linker_append_sec_data(linker, &obj);
>     err = err ?: linker_append_elf_syms(linker, &obj);
>     err = err ?: linker_append_elf_relos(linker, &obj);
>     err = err ?: linker_append_btf(linker, &obj);
>     err = err ?: linker_append_btf_ext(linker, &obj);
>
> If the object file that we try to process is not, in fact, a correct
> object file, linker_load_obj_file() may fail with errno not being set,
> and return 0. In this case we attempt to run linker_append_elf_sysms()
> and may segfault.
>
> This can happen (and was discovered) with bpftool:
>
>     $ bpftool gen object output.o sample_ret0.bpf.c
>     libbpf: failed to get ELF header for sample_ret0.bpf.c: invalid `Elf' handle
>     zsh: segmentation fault (core dumped)  bpftool gen object output.o sample_ret0.bpf.c
>
> Fix the issue by returning a non-null error code (-EINVAL) when libelf
> functions fail.
>
> Fixes: faf6ed321cf6 ("libbpf: Add BPF static linker APIs")
> Signed-off-by: Quentin Monnet <[e-mail]>
> ---
>  tools/lib/bpf/linker.c | 22 ++++++++--------------
>  1 file changed, 8 insertions(+), 14 deletions(-)
>

Ok, so *this* is the real issue with SIGSEGV that we were trying to
"prevent" by file path comparison in that bpftool-specific patch,
right? LGTM, I'll apply to bpf-next.

> diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
> index cf71d149fe26..e56ba6e67451 100644
> --- a/tools/lib/bpf/linker.c
> +++ b/tools/lib/bpf/linker.c
> @@ -566,17 +566,15 @@ static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
>         }
>         obj->elf = elf_begin(obj->fd, ELF_C_READ_MMAP, NULL);
>         if (!obj->elf) {
> -               err = -errno;
>                 pr_warn_elf("failed to parse ELF file '%s'", filename);
> -               return err;
> +               return -EINVAL;
>         }
>
>         /* Sanity check ELF file high-level properties */
>         ehdr = elf64_getehdr(obj->elf);
>         if (!ehdr) {
> -               err = -errno;
>                 pr_warn_elf("failed to get ELF header for %s", filename);
> -               return err;
> +               return -EINVAL;
>         }
>
>         /* Linker output endianness set by first input object */
> @@ -606,9 +604,8 @@ static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
>         }
>
>         if (elf_getshdrstrndx(obj->elf, &obj->shstrs_sec_idx)) {
> -               err = -errno;
>                 pr_warn_elf("failed to get SHSTRTAB section index for %s", filename);
> -               return err;
> +               return -EINVAL;
>         }
>
>         scn = NULL;
> @@ -618,26 +615,23 @@ static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
>
>                 shdr = elf64_getshdr(scn);
>                 if (!shdr) {
> -                       err = -errno;
>                         pr_warn_elf("failed to get section #%zu header for %s",
>                                     sec_idx, filename);
> -                       return err;
> +                       return -EINVAL;
>                 }
>
>                 sec_name = elf_strptr(obj->elf, obj->shstrs_sec_idx, shdr->sh_name);
>                 if (!sec_name) {
> -                       err = -errno;
>                         pr_warn_elf("failed to get section #%zu name for %s",
>                                     sec_idx, filename);
> -                       return err;
> +                       return -EINVAL;
>                 }
>
>                 data = elf_getdata(scn, 0);
>                 if (!data) {
> -                       err = -errno;
>                         pr_warn_elf("failed to get section #%zu (%s) data from %s",
>                                     sec_idx, sec_name, filename);
> -                       return err;
> +                       return -EINVAL;
>                 }
>
>                 sec = add_src_sec(obj, sec_name);
> @@ -2680,14 +2674,14 @@ int bpf_linker__finalize(struct bpf_linker *linker)
>
>         /* Finalize ELF layout */
>         if (elf_update(linker->elf, ELF_C_NULL) < 0) {
> -               err = -errno;
> +               err = -EINVAL;
>                 pr_warn_elf("failed to finalize ELF layout");
>                 return libbpf_err(err);
>         }
>
>         /* Write out final ELF contents */
>         if (elf_update(linker->elf, ELF_C_WRITE) < 0) {
> -               err = -errno;
> +               err = -EINVAL;
>                 pr_warn_elf("failed to write ELF contents");
>                 return libbpf_err(err);
>         }
> --
> 2.43.0
>

Quentin Monnet Dec. 5, 2024, 9:56 p.m. UTC | #2

2024-12-05 13:46 UTC-0800 ~ Andrii Nakryiko <[e-mail]>
> On Thu, Dec 5, 2024 at 5:59 AM Quentin Monnet <[e-mail]> wrote:
>>
>> Libelf functions do not set errno on failure. Instead, it relies on its
>> internal _elf_errno value, that can be retrieved via elf_errno (or the
>> corresponding message via elf_errmsg()). From "man libelf":
>>
>>     If a libelf function encounters an error it will set an internal
>>     error code that can be retrieved with elf_errno. Each thread
>>     maintains its own separate error code. The meaning of each error
>>     code can be determined with elf_errmsg, which returns a string
>>     describing the error.
>>
>> As a consequence, libbpf should not return -errno when a function from
>> libelf fails, because an empty value will not be interpreted as an error
>> and won't prevent the program to stop. This is visible in
>> bpf_linker__add_file(), for example, where we call a succession of
>> functions that rely on libelf:
>>
>>     err = err ?: linker_load_obj_file(linker, filename, opts, &obj);
>>     err = err ?: linker_append_sec_data(linker, &obj);
>>     err = err ?: linker_append_elf_syms(linker, &obj);
>>     err = err ?: linker_append_elf_relos(linker, &obj);
>>     err = err ?: linker_append_btf(linker, &obj);
>>     err = err ?: linker_append_btf_ext(linker, &obj);
>>
>> If the object file that we try to process is not, in fact, a correct
>> object file, linker_load_obj_file() may fail with errno not being set,
>> and return 0. In this case we attempt to run linker_append_elf_sysms()
>> and may segfault.
>>
>> This can happen (and was discovered) with bpftool:
>>
>>     $ bpftool gen object output.o sample_ret0.bpf.c
>>     libbpf: failed to get ELF header for sample_ret0.bpf.c: invalid `Elf' handle
>>     zsh: segmentation fault (core dumped)  bpftool gen object output.o sample_ret0.bpf.c
>>
>> Fix the issue by returning a non-null error code (-EINVAL) when libelf
>> functions fail.
>>
>> Fixes: faf6ed321cf6 ("libbpf: Add BPF static linker APIs")
>> Signed-off-by: Quentin Monnet <[e-mail]>
>> ---
>>  tools/lib/bpf/linker.c | 22 ++++++++--------------
>>  1 file changed, 8 insertions(+), 14 deletions(-)
>>
> 
> Ok, so *this* is the real issue with SIGSEGV that we were trying to
> "prevent" by file path comparison in that bpftool-specific patch,
> right? LGTM, I'll apply to bpf-next.


Correct, I wanted to find where that segfault was coming from, too :).
Thanks!

[e-mail] Dec. 5, 2024, 11:30 p.m. UTC | #3

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko <[e-mail]>:

On Thu,  5 Dec 2024 13:59:42 +0000 you wrote:
> Libelf functions do not set errno on failure. Instead, it relies on its
> internal _elf_errno value, that can be retrieved via elf_errno (or the
> corresponding message via elf_errmsg()). From "man libelf":
> 
>     If a libelf function encounters an error it will set an internal
>     error code that can be retrieved with elf_errno. Each thread
>     maintains its own separate error code. The meaning of each error
>     code can be determined with elf_errmsg, which returns a string
>     describing the error.
> 
> [...]

Here is the summary with links:
  - [bpf-next] libbpf: Fix segfault due to libelf functions not setting errno
    https://git.kernel.org/bpf/bpf-next/c/e10500b69c3f

You are awesome, thank you!

Patch

diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
index cf71d149fe26..e56ba6e67451 100644
--- a/tools/lib/bpf/linker.c
+++ b/tools/lib/bpf/linker.c
@@ -566,17 +566,15 @@  static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
 	}
 	obj->elf = elf_begin(obj->fd, ELF_C_READ_MMAP, NULL);
 	if (!obj->elf) {
-		err = -errno;
 		pr_warn_elf("failed to parse ELF file '%s'", filename);
-		return err;
+		return -EINVAL;
 	}
 
 	/* Sanity check ELF file high-level properties */
 	ehdr = elf64_getehdr(obj->elf);
 	if (!ehdr) {
-		err = -errno;
 		pr_warn_elf("failed to get ELF header for %s", filename);
-		return err;
+		return -EINVAL;
 	}
 
 	/* Linker output endianness set by first input object */
@@ -606,9 +604,8 @@  static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
 	}
 
 	if (elf_getshdrstrndx(obj->elf, &obj->shstrs_sec_idx)) {
-		err = -errno;
 		pr_warn_elf("failed to get SHSTRTAB section index for %s", filename);
-		return err;
+		return -EINVAL;
 	}
 
 	scn = NULL;
@@ -618,26 +615,23 @@  static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
 
 		shdr = elf64_getshdr(scn);
 		if (!shdr) {
-			err = -errno;
 			pr_warn_elf("failed to get section #%zu header for %s",
 				    sec_idx, filename);
-			return err;
+			return -EINVAL;
 		}
 
 		sec_name = elf_strptr(obj->elf, obj->shstrs_sec_idx, shdr->sh_name);
 		if (!sec_name) {
-			err = -errno;
 			pr_warn_elf("failed to get section #%zu name for %s",
 				    sec_idx, filename);
-			return err;
+			return -EINVAL;
 		}
 
 		data = elf_getdata(scn, 0);
 		if (!data) {
-			err = -errno;
 			pr_warn_elf("failed to get section #%zu (%s) data from %s",
 				    sec_idx, sec_name, filename);
-			return err;
+			return -EINVAL;
 		}
 
 		sec = add_src_sec(obj, sec_name);
@@ -2680,14 +2674,14 @@  int bpf_linker__finalize(struct bpf_linker *linker)
 
 	/* Finalize ELF layout */
 	if (elf_update(linker->elf, ELF_C_NULL) < 0) {
-		err = -errno;
+		err = -EINVAL;
 		pr_warn_elf("failed to finalize ELF layout");
 		return libbpf_err(err);
 	}
 
 	/* Write out final ELF contents */
 	if (elf_update(linker->elf, ELF_C_WRITE) < 0) {
-		err = -errno;
+		err = -EINVAL;
 		pr_warn_elf("failed to write ELF contents");
 		return libbpf_err(err);
 	}