Message-ID: <b1e119ad-5380-4fd7-a5a6-89e2204410d2@gmail.com>
Date: Wed, 24 Jul 2024 11:27:14 -0700
From: Kui-Feng Lee <sinquersw@...il.com>
To: Amery Hung <ameryhung@...il.com>, netdev@...r.kernel.org
Cc: bpf@...r.kernel.org, yangpeihao@...u.edu.cn, daniel@...earbox.net,
 andrii@...nel.org, alexei.starovoitov@...il.com, martin.lau@...nel.org,
 toke@...hat.com, jhs@...atatu.com, jiri@...nulli.us, sdf@...gle.com,
 xiyou.wangcong@...il.com, yepeilin.cs@...il.com
Subject: Re: [RFC PATCH v9 03/11] bpf: Allow struct_ops prog to return
 referenced kptr



On 7/23/24 22:36, Kui-Feng Lee wrote:
> 
> 
> On 7/14/24 10:51, Amery Hung wrote:
>> Allow a struct_ops program to return a referenced kptr if the struct_ops
>> operator has pointer to struct as the return type. To make sure the
>> returned pointer continues to be valid in the kernel, several
>> constraints are required:
>>
>> 1) The type of the pointer must matches the return type
>> 2) The pointer originally comes from the kernel (not locally allocated)
>> 3) The pointer is in its unmodified form
>>
>> In addition, since the first user, Qdisc_ops::dequeue, allows a NULL
>> pointer to be returned when there is no skb to be dequeued, we will allow
>> a scalar value with value equals to NULL to be returned.
>>
>> In the future when there is a struct_ops user that always expects a valid
>> pointer to be returned from an operator, we may extend tagging to the
>> return value. We can tell the verifier to only allow NULL pointer return
>> if the return value is tagged with MAY_BE_NULL.
>>
>> The check is split into two parts since check_reference_leak() happens
>> before check_return_code(). We first allow a reference object to leak
>> through return if it is in the return register and the type matches the
>> return type. Then, we check whether the pointer to-be-returned is 
>> valid in
>> check_return_code().
>>
>> Signed-off-by: Amery Hung <amery.hung@...edance.com>
>> ---
>>   kernel/bpf/verifier.c | 50 +++++++++++++++++++++++++++++++++++++++----
>>   1 file changed, 46 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index f614ab283c37..e7f356098902 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -10188,16 +10188,36 @@ 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)
>>   {
>> +    enum bpf_prog_type type = resolve_prog_type(env->prog);
>> +    u32 regno = exception_exit ? BPF_REG_1 : BPF_REG_0;
>> +    struct bpf_reg_state *reg = reg_state(env, regno);
>>       struct bpf_func_state *state = cur_func(env);
>> +    const struct bpf_prog *prog = env->prog;
>> +    const struct btf_type *ret_type = NULL;
>>       bool refs_lingering = false;
>> +    struct btf *btf;
>>       int i;
>>       if (!exception_exit && state->frameno && !state->in_callback_fn)
>>           return 0;
>> +    if (type == BPF_PROG_TYPE_STRUCT_OPS &&
>> +        reg->type & PTR_TO_BTF_ID && reg->ref_obj_id) {
>> +        btf = bpf_prog_get_target_btf(prog);
>> +        ret_type = btf_type_by_id(btf, 
>> prog->aux->attach_func_proto->type);
>> +        if (reg->btf_id != ret_type->type) {
>> +            verbose(env, "Return kptr type, struct %s, doesn't match 
>> function prototype, struct %s\n",
>> +                btf_type_name(reg->btf, reg->btf_id),
>> +                btf_type_name(btf, ret_type->type));
>> +            return -EINVAL;
>> +        }
>> +    }
>> +
>>       for (i = 0; i < state->acquired_refs; i++) {
>>           if (!exception_exit && state->in_callback_fn && 
>> state->refs[i].callback_ref != state->frameno)
>>               continue;
>> +        if (ret_type && reg->ref_obj_id == state->refs[i].id)
>> +            continue;
> 
> Is it possible having two kptrs that both are in the returned type
> passing into a function?

Does it work to remove the ref pointed by reg0 from state
at the location that handles BPF_EXIT in do_check()?

> 
> 
>>           verbose(env, "Unreleased reference id=%d alloc_insn=%d\n",
>>               state->refs[i].id, state->refs[i].insn_idx);
>>           refs_lingering = true;
>> @@ -15677,12 +15697,15 @@ 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;
>> +    struct btf *btf = bpf_prog_get_target_btf(prog);
>> +    bool st_ops_ret_is_kptr = false;
>> +    const struct btf_type *t;
>>       /* LSM and struct_ops func-ptr's return type could be "void" */
>>       if (!is_subprog || frame->in_exception_callback_fn) {
>> @@ -15691,10 +15714,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;
>> +
>> +            t = btf_type_by_id(btf, prog->aux->attach_func_proto->type);
>> +            if (btf_type_is_ptr(t)) {
>> +                /* Allow struct_ops programs to return kptr or null if
>> +                 * the return type is a pointer type.
>> +                 * check_reference_leak has ensured the returning kptr
>> +                 * matches the type of the function prototype and is
>> +                 * the only leaking reference. Thus, we can safely 
>> return
>> +                 * if the pointer is in its unmodified form
>> +                 */
>> +                if (reg->type & PTR_TO_BTF_ID)
>> +                    return __check_ptr_off_reg(env, reg, regno, false);
>> +                st_ops_ret_is_kptr = true;
>> +            }
>>               break;
>>           default:
>>               break;
>> @@ -15716,8 +15755,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";
>> @@ -15804,6 +15841,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 (!st_ops_ret_is_kptr)
>> +            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.

Powered by blists - more mailing lists