Re: [PATCH RFC bpf-next 2/7] libbpf: Implement bpf_gen__init_kern_struct_ops() for light skeletons

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 6839 bytes --]

> libbpf: Implement bpf_gen__init_kern_struct_ops() for light skeletons

TL;DR:
- Light-skeleton struct_ops loader may overflow into adjacent kern_vdata
  members: kernel member size/kind unchecked
- Module BTF fds may leak on loader failure paths
- Type lookups may resolve in different BTF objects, failing module struct_ops
  loads

> diff --git a/tools/lib/bpf/gen_loader.c b/tools/lib/bpf/gen_loader.c
> --- a/tools/lib/bpf/gen_loader.c
> +++ b/tools/lib/bpf/gen_loader.c

[ ... ]

> +static void emit_st_ops_member_val(struct bpf_gen *gen, const char *mname,
> +				   const void *mdata, __u32 msize,
> +				   int kern_vdata_off)
> +{

[ ... ]

> +	mdata_off = add_data(gen, mdata, msize);
> +	emit_bpf_get_struct_member_info(gen, mname);
> +
> +	/* Extract member byte offset (lower 32 bits of R7), add kern_data_off, and bound it */
> +	emit(gen, BPF_MOV32_REG(BPF_REG_2, BPF_REG_7));
> +	emit(gen, BPF_ALU32_REG(BPF_ADD, BPF_REG_2, BPF_REG_9));
> +	emit(gen, BPF_JMP_IMM(BPF_JLE, BPF_REG_2, KERN_VDATA_MAX_SIZE - msize, 2));
> +	emit(gen, BPF_MOV64_IMM(BPF_REG_7, -E2BIG));
> +	emit_check_err(gen);

The gen path copies msize bytes, where msize is the size of the local
member, to the kernel member's offset.  The only bound here is the 4096
byte kern_vdata buffer.  The kernel member's size and kind, and val_sz,
are not checked.

bpf_map__init_kern_struct_ops() rejects both cases with -ENOTSUP:

    if (BTF_INFO_KIND(mtype->info) != BTF_INFO_KIND(kern_mtype->info)) {
        ...
        return -ENOTSUP;
    }
    ...
    kern_msize = btf__resolve_size(kern_btf, kern_mtype_id);
    if (kern_msize < 0 || msize != kern_msize) {
        ...
        return -ENOTSUP;
    }

If a local struct_ops definition (a ___flavor type, or a vmlinux.h from
another kernel) has a non-zero scalar member that is wider than the
running kernel's, or is a scalar where the kernel has a function pointer,
can the loader write the extra bytes into the following kernel members of
kern_vdata?

If the init_member() callback accepts those values, the map could be
registered with wrong values, and bytes landing in a function pointer slot
are later read as a prog fd by bpf_struct_ops_map_update_elem().

The regular libbpf load of the same object fails cleanly.  Was dropping
these checks intended, or could bpf_btf_get_struct_member_info() also
return the member size, or the loader at least check against val_sz?

[ ... ]

> +void bpf_gen__init_kern_struct_ops(struct bpf_gen *gen, struct btf *btf,
> +				   struct bpf_struct_ops *st_ops,
> +				   __u32 *vtype_id_off,
> +				   unsigned int *val_sz_off)
> +{

[ ... ]

> +	/* 1. Resolve BTF ID of struct bpf_struct_ops_<tname> */
> +	*vtype_id_off = add_data(gen, NULL, sizeof(__u64));
> +	relo.name = stname;
> +	emit_bpf_find_by_name_kind(gen, &relo);
> +	emit_check_err(gen);
> +	emit(gen, BPF_MOV64_REG(BPF_REG_8, BPF_REG_7));
> +	emit2(gen, BPF_LD_IMM64_RAW_FULL(BPF_REG_1, BPF_PSEUDO_MAP_IDX_VALUE,
> +					 0, 0, 0, *vtype_id_off));
> +	emit(gen, BPF_STX_MEM(BPF_DW, BPF_REG_1, BPF_REG_7, 0));

For a struct_ops type that lives in a module, each
bpf_btf_find_by_name_kind() call (steps 1 and 3) creates a new module BTF
fd.  The fd is kept only in the upper 32 bits of the blob slots
*vtype_id_off and st_ops->type_id_off.

The cleanup label emitted by bpf_gen__init() closes only the loader_stack
slots (btf_fd, inner_map_fd, prog_fd[]) and the map entries of fd_array,
so these fds are not closed when emit_check_err() jumps to cleanup.

This commit adds no close at all.  Later in the series, c8839efa9329
("libbpf: Support BPF_MAP_TYPE_STRUCT_OPS in bpf_gen__map_create()")
closes the step 1 fd right after BPF_MAP_CREATE, and b5342aeba917
("libbpf: Implement bpf_gen__struct_ops_update_elem() for light
skeletons") closes the step 3 fd only in bpf_gen__struct_ops_update_elem(),
which runs after every program has loaded.

Can a failure in between leak these BTF fds in the calling process?  For
example a missing kernel member (-ENOENT from
bpf_btf_get_struct_member_info()), the -E2BIG bound check, BTF load, an
earlier map create, or a verifier rejection of any program would each
jump to cleanup with the fds still open.

bpf_gen__prog_load() already closes attach_btf_obj_fd and the kfunc BTF
fds right after PROG_LOAD ("successful or not, close btf module FDs").
Could these fds be kept in extra loader_stack slots that the cleanup label
closes, or be closed before the next emit_check_err() when not needed
later?

[ ... ]

> +	/* 3. Resolve BTF ID of struct <tname> */
> +	st_ops->type_id_off = add_data(gen, NULL, sizeof(__u64));
> +	relo.name = tname;
> +	emit_bpf_find_by_name_kind(gen, &relo);
> +	emit_check_err(gen);

Step 3 looks up struct <tname> with a fresh, global
bpf_btf_find_by_name_kind() call that is not tied to the BTF object where
step 1 found struct bpf_struct_ops_<tname>.  The kernel's bpf_find_btf_id()
searches vmlinux first and then every module BTF in idr order, so the two
lookups can land in different BTF objects.

The non-gen path looks up the value type first and then looks up tname in
the same btf:

    kern_vtype_id = find_ksym_btf_id(obj, stname, BTF_KIND_STRUCT, &btf, mod_btf);
    ...
    kern_type_id = btf__find_by_name_kind(btf, tname, BTF_KIND_STRUCT);

and bpf_map__init_kern_struct_ops() then sets prog->attach_btf_obj_fd to
mod_btf->fd.

A concrete case is net/smc/smc_hs_bpf.c, which registers the smc_hs_ctrl
struct_ops from smc.ko.  struct smc_hs_ctrl is also fully defined in
vmlinux BTF, because net/ipv4/tcp_output.c includes <net/smc.h>.  In gen
mode:

    step 1 finds bpf_struct_ops_smc_hs_ctrl in smc.ko and returns
           (smc_fd << 32) | vtype_id
    step 3 finds smc_hs_ctrl in vmlinux and returns
           (0 << 32) | type_id

With ee35678a566a ("libbpf: Support BPF_PROG_TYPE_STRUCT_OPS in
bpf_gen__prog_load()"), prog_load takes attach_btf_obj_fd from the upper
half of type_id_off, which is 0.  bpf_prog_load() then falls back to
vmlinux BTF, and check_struct_ops_btf_id() -> bpf_struct_ops_find() does
not find the type in vmlinux's struct_ops_tab, so the load fails with
"attach_btf_id %u is not a supported struct" and -ENOTSUPP.

The same mismatch can occur when another module loaded earlier carries its
own copy of struct <tname> in its split BTF.  No later commit in the
series changes this lookup, and the selftests only use bpf_testmod_ops,
which exists only in bpf_testmod.ko.

Could the interface type be resolved within the BTF object returned in
step 1, or could the step 1 fd be paired with the type id when the step 3
result comes from vmlinux?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/42289585064