On Wed, Aug 21, 2024 at 10:46 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote: > > On Wed, 2024-08-21 at 09:59 -0700, Andrii Nakryiko wrote: > > [...] > > > > Fixes: 74753e1462e7 ("libbpf: Replace btf__type_by_id() with btf_type_by_id().") > > > Reported-by: Liu RuiTong <cnitlrt@xxxxxxxxx> > > > Closes: https://lore.kernel.org/bpf/CAK55_s6do7C+DVwbwY_7nKfUz0YLDoiA1v6X3Y9+p0sWzipFSA@xxxxxxxxxxxxxx/ > > > Signed-off-by: Eduard Zingerman <eddyz87@xxxxxxxxx> > > > --- > > > tools/lib/bpf/relo_core.c | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/tools/lib/bpf/relo_core.c b/tools/lib/bpf/relo_core.c > > > index 63a4d5ad12d1..a04724831ebc 100644 > > > --- a/tools/lib/bpf/relo_core.c > > > +++ b/tools/lib/bpf/relo_core.c > > > @@ -1297,6 +1297,11 @@ int bpf_core_calc_relo_insn(const char *prog_name, > > > > > > local_id = relo->type_id; > > > local_type = btf_type_by_id(local_btf, local_id); > > > + if (!local_type) { > > > > This is a meaningless check at least for libbpf's implementation of > > btf_type_by_id(), it never returns NULL. Commit you point to in Fixes > > tag clearly states the differences. > > That is not true on kernel side. > bpf_core_calc_relo_insn() is called from bpf_core_apply(): > > int bpf_core_apply(struct bpf_core_ctx *ctx, const struct bpf_core_relo *relo, > int relo_idx, void *insn) > { > bool need_cands = relo->kind != BPF_CORE_TYPE_ID_LOCAL; > ... > if (need_cands) { > ... > // code below would report an error if relo->type_id is bogus > cc = bpf_core_find_cands(ctx, relo->type_id); > if (IS_ERR(cc)) { > bpf_log(ctx->log, "target candidate search failed for %d\n", > relo->type_id); > err = PTR_ERR(cc); > goto out; > } > ... > } > > err = bpf_core_calc_relo_insn((void *)ctx->log, relo, relo_idx, ctx->btf, &cands, specs, > &targ_res); > ... > } > > If `need_cands` is false the bogus type_id could reach into bpf_core_calc_relo_insn(). > Which is exactly the case with repro submitted by Liu. > There is also a simplified repro here: > https://github.com/kernel-patches/bpf/commit/017a9dcf17e572f9b7c32aa62a81df8ef41cef17 > But I can't submit it as a test yet. > > > > > So you'd need to validate local_id directly against number of types in > > local_btf. > > How is this better than a null check? > because id check will be useful for both kernel and libbpf sides?.. > > > > pw-bot: cr > > > > > > > + pr_warn("prog '%s': relo #%d: bad type id %u\n", > > > > nit: this part of CO-RE-related code normally uses [%u] "syntax" to > > point to BTF type IDs, please adjust for consistency > > Ok >