Re: [PATCH bpf-next v4 5/8] libbpf: Support opening bpf objects of either endianness

[Tony Ambardar <tony.ambardar@gmail.com>]

On Fri, Aug 30, 2024 at 02:25:54PM -0700, Andrii Nakryiko wrote:
> On Fri, Aug 30, 2024 at 12:30 AM Tony Ambardar <tony.ambardar@gmail.com> 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 <tony.ambardar@gmail.com>
> > ---
> >  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);
> 
> [...]