Re: [PATCH] libbpf: Fix NULL pointer dereference in find_extern_btf_id

[Andrii Nakryiko]

On Mon, Mar 18, 2024 at 9:47 AM Stanislav Fomichev <sdf@xxxxxxxxxx> wrote:
>
> On 03/18, zhangmingyi wrote:
> > From: Mingyi Zhang <zhangmingyi5@xxxxxxxxxx>
> >
> > During our fuzz testing, we encountered the following error:
> >
> > Program received signal SIGSEGV, Segmentation fault.
> > 0x00000000005915bb in __interceptor_strcmp.part.0 ()
> > (gdb) bt
> >     #0  0x00000000005915bb in __interceptor_strcmp.part.0 ()
> >     #1  0x000000000087dc65 in __wrap_strcmp ()
> >     #2  0x0000000000951ded in find_extern_btf_id () at libbpf.c:3508
> >     #3  0x000000000094d7a1 in bpf_object.collect_externs () at libbpf.c:3712
> >     #4  0x000000000092be3b in bpf_object_open () at libbpf.c:7433
> >     #5  0x000000000092c046 in bpf_object.open_mem () at libbpf.c:7497
> >     #6  0x0000000000924afa in LLVMFuzzerTestOneInput () at fuzz/bpf-object-fuzzer.c:16
> >     #7  0x000000000060be11 in testblitz_engine::fuzzer::Fuzzer::run_one ()
> >     #8  0x000000000087ad92 in tracing::span::Span::in_scope ()
> >     #9  0x00000000006078aa in testblitz_engine::fuzzer::util::walkdir ()
> >     #10 0x00000000005f3217 in testblitz_engine::entrypoint::main::{{closure}} ()
> >     #11 0x00000000005f2601 in main ()
> > (gdb)
> >
> > tname = btf__name_by_offset(btf, t->name_off);
> > if (strcmp(tname, ext_name))
> >         continue;
> >
> > tname is passed directly into strcmp without a null pointer check.
> > When t(btf_type)->name_off >= btf->hdr->str_len, tname is NULL. normally,
> > that's not likely to happen.

Libbpf now does BTF sanity checking, see btf_sanity_check() in btf.c.
Do you still get this crash with latest libbpf?

> > Considering that the bpf_object__open_mem interface is a direct API
> > provided to users, which reads directly from memory. There may be an
> > input similar to this fuzzing, leading to a Segmentation fault.
>
> Are you trying to parse completely bogus elf obj files?
> I don't think we have been hardening against those cases. I see

yep, I agree, it's definitely not a priority to make sure that feeding
malicious or random garbage ELF into libbpf should never crash (we had
libelf crashing before libbpf could even do anything, for example). In
this case I think BTF sanity checking should be good enough.

But in general, I'd definitely wouldn't go out of my way to guard
against some ELF corruption, though if it's just a simple NULL or
bounds check, it's no big deal to add that (though as I said, BTF
sanity check was added specifically so that we don't have to
double-check BTF invariants, like having a valid string reference, all
around the code).

> a bunch of other places where we assume the return of btf__name_by_offset
> is non-null. Do we need to audit all those places as well?