On Tue, Jul 20, 2021 at 1:51 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Fri, Jun 11, 2021 at 1:23 PM Andrii Nakryiko
> <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > On Thu, May 13, 2021 at 5:36 PM Alexei Starovoitov
> > <alexei.starovoitov@xxxxxxxxx> wrote:
> > >
> > > From: Alexei Starovoitov <ast@xxxxxxxxxx>
> > >
> > > The BPF program loading process performed by libbpf is quite complex
> > > and consists of the following steps:
> > > "open" phase:
> > > - parse elf file and remember relocations, sections
> > > - collect externs and ksyms including their btf_ids in prog's BTF
> > > - patch BTF datasec (since llvm couldn't do it)
> > > - init maps (old style map_def, BTF based, global data map, kconfig map)
> > > - collect relocations against progs and maps
> > > "load" phase:
> > > - probe kernel features
> > > - load vmlinux BTF
> > > - resolve externs (kconfig and ksym)
> > > - load program BTF
> > > - init struct_ops
> > > - create maps
> > > - apply CO-RE relocations
> > > - patch ld_imm64 insns with src_reg=PSEUDO_MAP, PSEUDO_MAP_VALUE, PSEUDO_BTF_ID
> > > - reposition subprograms and adjust call insns
> > > - sanitize and load progs
> > >
> > > During this process libbpf does sys_bpf() calls to load BTF, create maps,
> > > populate maps and finally load programs.
> > > Instead of actually doing the syscalls generate a trace of what libbpf
> > > would have done and represent it as the "loader program".
> > > The "loader program" consists of single map with:
> > > - union bpf_attr(s)
> > > - BTF bytes
> > > - map value bytes
> > > - insns bytes
> > > and single bpf program that passes bpf_attr(s) and data into bpf_sys_bpf() helper.
> > > Executing such "loader program" via bpf_prog_test_run() command will
> > > replay the sequence of syscalls that libbpf would have done which will result
> > > the same maps created and programs loaded as specified in the elf file.
> > > The "loader program" removes libelf and majority of libbpf dependency from
> > > program loading process.
> > >
> > > kconfig, typeless ksym, struct_ops and CO-RE are not supported yet.
> > >
> > > The order of relocate_data and relocate_calls had to change, so that
> > > bpf_gen__prog_load() can see all relocations for a given program with
> > > correct insn_idx-es.
> > >
> > > Signed-off-by: Alexei Starovoitov <ast@xxxxxxxxxx>
> > > Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > > ---
> > > tools/lib/bpf/Build | 2 +-
> > > tools/lib/bpf/bpf_gen_internal.h | 40 ++
> > > tools/lib/bpf/gen_loader.c | 689 +++++++++++++++++++++++++++++++
> > > tools/lib/bpf/libbpf.c | 226 ++++++++--
> > > tools/lib/bpf/libbpf.h | 12 +
> > > tools/lib/bpf/libbpf.map | 1 +
> > > tools/lib/bpf/libbpf_internal.h | 2 +
> > > tools/lib/bpf/skel_internal.h | 123 ++++++
> > > 8 files changed, 1060 insertions(+), 35 deletions(-)
> > > create mode 100644 tools/lib/bpf/bpf_gen_internal.h
> > > create mode 100644 tools/lib/bpf/gen_loader.c
> > > create mode 100644 tools/lib/bpf/skel_internal.h
> > >
> >
> > [...]
> >
> > > +void bpf_gen__prog_load(struct bpf_gen *gen,
> > > + struct bpf_prog_load_params *load_attr, int prog_idx)
> > > +{
> > > + int attr_size = offsetofend(union bpf_attr, fd_array);
> > > + int prog_load_attr, license, insns, func_info, line_info;
> > > + union bpf_attr attr;
> > > +
> > > + memset(&attr, 0, attr_size);
> > > + pr_debug("gen: prog_load: type %d insns_cnt %zd\n",
> > > + load_attr->prog_type, load_attr->insn_cnt);
> > > + /* add license string to blob of bytes */
> > > + license = add_data(gen, load_attr->license, strlen(load_attr->license) + 1);
> > > + /* add insns to blob of bytes */
> > > + insns = add_data(gen, load_attr->insns,
> > > + load_attr->insn_cnt * sizeof(struct bpf_insn));
> > > +
> > > + attr.prog_type = load_attr->prog_type;
> > > + attr.expected_attach_type = load_attr->expected_attach_type;
> > > + attr.attach_btf_id = load_attr->attach_btf_id;
> > > + attr.prog_ifindex = load_attr->prog_ifindex;
> > > + attr.kern_version = 0;
> > > + attr.insn_cnt = (__u32)load_attr->insn_cnt;
> > > + attr.prog_flags = load_attr->prog_flags;
> > > +
> > > + attr.func_info_rec_size = load_attr->func_info_rec_size;
> > > + attr.func_info_cnt = load_attr->func_info_cnt;
> > > + func_info = add_data(gen, load_attr->func_info,
> > > + attr.func_info_cnt * attr.func_info_rec_size);
> > > +
> > > + attr.line_info_rec_size = load_attr->line_info_rec_size;
> > > + attr.line_info_cnt = load_attr->line_info_cnt;
> > > + line_info = add_data(gen, load_attr->line_info,
> > > + attr.line_info_cnt * attr.line_info_rec_size);
> > > +
> >
> > Hey Alexei,
> >
> > Coverity ([0] and [1]) is complaining that load_attr->func_info and
> > load_attr->line_info can be NULL in some cases, which will lead to
> > NULL deref. I'm not sure if we restrict gen_loader to be only used
> > with BPF applications that have BTF embedded. If not, then it will
> > cause a crash, so we need to protect against that. Please take a look.
> >
> > [0] https://scan3.coverity.com/reports.htm#v40547/p11903/fileInstanceId=53874059&defectInstanceId=10901198&mergedDefectId=349034
> > [1] https://scan3.coverity.com/reports.htm#v40547/p11903/fileInstanceId=53874059&defectInstanceId=10901191&mergedDefectId=349033
> >
> > Not sure why we have two issues above, they both look identical, but
> > for completeness I included both.
>
> I cannot access these links.
Unfortunately Coverity doesn't allow access to those report for users
without account and not explicitly allowed for a given project :( I've
sent invitation to your gmail account, just in case.
> Looking at the code the func_info can be NULL,
> but in such case the line_info_cnt will be zero.
> realloc_data_buf() will succeed as a nop and then there will be:
> memcpy(gen->data_cur, NULL, 0);
> which is ok to do. I double checked.
> So this coverity issue looks like a false positive.
Yep, makes sense. Thanks for checking! I'll mark them as false positives.
