On Fri, 2024-12-20 at 11:55 -0800, Amery Hung wrote:
[...]
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index d9e0af00580b..27d4a170df84 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -386,7 +386,7 @@ int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
> st_ops_desc->value_type = btf_type_by_id(btf, value_id);
>
> for_each_member(i, t, member) {
> - const struct btf_type *func_proto;
> + const struct btf_type *func_proto, *ret_type;
>
> mname = btf_name_by_offset(btf, member->name_off);
> if (!*mname) {
> @@ -409,6 +409,16 @@ int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
> if (!func_proto)
> continue;
>
> + if (func_proto->type) {
> + ret_type = btf_type_resolve_ptr(btf, func_proto->type, NULL);
> + if (ret_type && !__btf_type_is_struct(ret_type)) {
> + pr_warn("func ptr %s in struct %s returns non-struct pointer, which is not supported\n",
> + mname, st_ops->name);
> + err = -EOPNOTSUPP;
> + goto errout;
> + }
> + }
> +
This limitation seems unnecessary, if reference leaks are only allowed
for parameters marked with __ref.
> if (btf_distill_func_proto(log, btf,
> func_proto, mname,
> &st_ops->func_models[i])) {
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 26305571e377..0e6a3c4daa7d 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -10707,6 +10707,8 @@ record_func_key(struct bpf_verifier_env *env, struct bpf_call_arg_meta *meta,
> static int check_reference_leak(struct bpf_verifier_env *env, bool exception_exit)
> {
> struct bpf_verifier_state *state = env->cur_state;
> + enum bpf_prog_type type = resolve_prog_type(env->prog);
> + struct bpf_reg_state *reg = reg_state(env, BPF_REG_0);
> bool refs_lingering = false;
> int i;
>
> @@ -10716,6 +10718,12 @@ static int check_reference_leak(struct bpf_verifier_env *env, bool exception_exi
> for (i = 0; i < state->acquired_refs; i++) {
> if (state->refs[i].type != REF_TYPE_PTR)
> continue;
> + /* Allow struct_ops programs to return a referenced kptr back to
> + * kernel. Type checks are performed later in check_return_code.
> + */
> + if (type == BPF_PROG_TYPE_STRUCT_OPS && !exception_exit &&
> + reg->ref_obj_id == state->refs[i].id)
> + continue;
> verbose(env, "Unreleased reference id=%d alloc_insn=%d\n",
> state->refs[i].id, state->refs[i].insn_idx);
> refs_lingering = true;
> @@ -16320,13 +16328,14 @@ static int check_return_code(struct bpf_verifier_env *env, int regno, const char
> const char *exit_ctx = "At program exit";
> struct tnum enforce_attach_type_range = tnum_unknown;
> const struct bpf_prog *prog = env->prog;
> - struct bpf_reg_state *reg;
> + struct bpf_reg_state *reg = reg_state(env, regno);
> struct bpf_retval_range range = retval_range(0, 1);
> enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
> int err;
> struct bpf_func_state *frame = env->cur_state->frame[0];
> const bool is_subprog = frame->subprogno;
> bool return_32bit = false;
> + const struct btf_type *reg_type, *ret_type = NULL;
>
> /* LSM and struct_ops func-ptr's return type could be "void" */
> if (!is_subprog || frame->in_exception_callback_fn) {
> @@ -16335,10 +16344,26 @@ static int check_return_code(struct bpf_verifier_env *env, int regno, const char
> if (prog->expected_attach_type == BPF_LSM_CGROUP)
> /* See below, can be 0 or 0-1 depending on hook. */
> break;
> - fallthrough;
> + if (!prog->aux->attach_func_proto->type)
> + return 0;
> + break;
> case BPF_PROG_TYPE_STRUCT_OPS:
> if (!prog->aux->attach_func_proto->type)
> return 0;
> +
> + if (frame->in_exception_callback_fn)
> + break;
> +
> + /* Allow a struct_ops program to return a referenced kptr if it
> + * matches the operator's return type and is in its unmodified
> + * form. A scalar zero (i.e., a null pointer) is also allowed.
> + */
> + reg_type = reg->btf ? btf_type_by_id(reg->btf, reg->btf_id) : NULL;
> + ret_type = btf_type_resolve_ptr(prog->aux->attach_btf,
> + prog->aux->attach_func_proto->type,
> + NULL);
This does not enforce the kernel provenance of the pointer.
See my comment for the next patch for an example.
I think such return should only be allowed for parameters marked with
__ref suffix. If so, pointer provenance check would just compare
reg->ref_obj_id value with known ids of __ref arguments.
> + if (ret_type && ret_type == reg_type && reg->ref_obj_id)
> + return __check_ptr_off_reg(env, reg, regno, false);
> break;
> default:
> break;
> @@ -16360,8 +16385,6 @@ static int check_return_code(struct bpf_verifier_env *env, int regno, const char
> return -EACCES;
> }
>
> - reg = cur_regs(env) + regno;
> -
> if (frame->in_async_callback_fn) {
> /* enforce return zero from async callbacks like timer */
> exit_ctx = "At async callback return";
> @@ -16460,6 +16483,11 @@ static int check_return_code(struct bpf_verifier_env *env, int regno, const char
> case BPF_PROG_TYPE_NETFILTER:
> range = retval_range(NF_DROP, NF_ACCEPT);
> break;
> + case BPF_PROG_TYPE_STRUCT_OPS:
> + if (!ret_type)
> + return 0;
> + range = retval_range(0, 0);
> + break;
> case BPF_PROG_TYPE_EXT:
> /* freplace program can return anything as its return value
> * depends on the to-be-replaced kernel func or bpf program.
