On Fri, Aug 30, 2024 at 02:25:54PM -0700, Andrii Nakryiko wrote:
> On Fri, Aug 30, 2024 at 12:30 AM Tony Ambardar <[email protected]>
> wrote:
> >
> > Allow bpf_object__open() to access files of either endianness, and convert
> > included BPF programs to native byte-order in-memory for introspection.
> > Loading BPF objects of non-native byte-order is still disallowed however.
> >
> > Signed-off-by: Tony Ambardar <[email protected]>
> > ---
> > tools/lib/bpf/libbpf.c | 49 +++++++++++++++++++++++++++------
> > tools/lib/bpf/libbpf_internal.h | 11 ++++++++
> > 2 files changed, 52 insertions(+), 8 deletions(-)
> >
>
> [...]
>
> >
> > + /* Validate ELF object endianness... */
> > + if (ehdr->e_ident[EI_DATA] != ELFDATA2LSB &&
> > + ehdr->e_ident[EI_DATA] != ELFDATA2MSB) {
> > + err = -LIBBPF_ERRNO__ENDIAN;
> > + pr_warn("elf: '%s' has unknown byte order\n", obj->path);
> > + goto errout;
> > + }
> > + /* and preserve outside lifetime of bpf_object_open() */
>
> what does it mean "preserve outside lifetime" ?
bpf_object_open() freed ELF data on exit but didn't zero obj->efile.ehdr,
leading to unpredictable use-after-free problems in is_native_endianness().
This is part of the fix but should be clearer e.g. "save after ELF data
freed...". Will update.
>
> > + obj->byteorder = ehdr->e_ident[EI_DATA];
> > +
> > +
> > +
>
> why so many empty lines?..
I'm blind? Fixed, thanks.
>
> > if (elf_getshdrstrndx(elf, &obj->efile.shstrndx)) {
> > pr_warn("elf: failed to get section names section index for
> > %s: %s\n",
> > obj->path, elf_errmsg(-1));
>
> [...]
>
> > err = bpf_object__elf_init(obj);
> > - err = err ? : bpf_object__check_endianness(obj);
> > err = err ? : bpf_object__elf_collect(obj);
> > err = err ? : bpf_object__collect_externs(obj);
> > err = err ? : bpf_object_fixup_btf(obj);
> > @@ -8500,6 +8529,10 @@ static int bpf_object_load(struct bpf_object *obj,
> > int extra_log_level, const ch
> >
> > if (obj->gen_loader)
> > bpf_gen__init(obj->gen_loader, extra_log_level,
> > obj->nr_programs, obj->nr_maps);
>
> nit: add {} around if, both sides should either have or not have {}
>
OK, done.
> > + else if (!is_native_endianness(obj)) {
> > + pr_warn("object '%s' is not native endianness\n",
> > obj->name);
>
> "object '%s': load is not supported in non-native endianness\n"
Clearer, will update.
>
>
> > + return libbpf_err(-LIBBPF_ERRNO__ENDIAN);
> > + }
> >
> > err = bpf_object_prepare_token(obj);
> > err = err ? : bpf_object__probe_loading(obj);
>
> [...]
