On Thu, Dec 5, 2024 at 5:59 AM Quentin Monnet <qmo@xxxxxxxxxx> 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 <qmo@xxxxxxxxxx> > --- > 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 >