Re: [PATCH bpf-next v2 03/14] bpf: Allow struct_ops prog to return referenced kptr

[Eduard Zingerman <eddyz87@gmail.com>]

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.