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.
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.
