On Tue, Feb 21, 2023 at 10:07:46PM -0800, Joanne Koong wrote:
> @@ -9975,8 +10023,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
> }
> }
>
> - for (i = 0; i < CALLER_SAVED_REGS; i++)
> - mark_reg_not_init(env, regs, caller_saved[i]);
> + mark_reg_not_init(env, regs, caller_saved[BPF_REG_0]);
...
> /* Check return type */
> t = btf_type_skip_modifiers(desc_btf, func_proto->type, NULL);
> @@ -10062,6 +10109,41 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
> regs[BPF_REG_0].type = PTR_TO_BTF_ID | PTR_UNTRUSTED;
> regs[BPF_REG_0].btf = desc_btf;
> regs[BPF_REG_0].btf_id = meta.arg_constant.value;
> + } else if (meta.func_id == special_kfunc_list[KF_bpf_dynptr_slice] ||
> + meta.func_id == special_kfunc_list[KF_bpf_dynptr_slice_rdwr]) {
> + enum bpf_type_flag type_flag = get_dynptr_type_flag(meta.initialized_dynptr.type);
> +
> + mark_reg_known_zero(env, regs, BPF_REG_0);
> +
> + if (!tnum_is_const(regs[BPF_REG_4].var_off)) {
> + verbose(env, "mem_size must be a constant\n");
> + return -EINVAL;
> + }
> + regs[BPF_REG_0].mem_size = regs[BPF_REG_4].var_off.value;
This is a bit fragile.
Instead of moving above 'for' may be let's use meta.arg_constant approach ?
Just doing:
if (is_kfunc_arg_constant(meta->btf, &args[i]) || is_kfunc_arg_mem_size(...))
won't quite work, since we'll be doing mark_chain_precision() twice.
(check_kfunc_mem_size_reg() will do it 2nd time).
Another issue is that __sz allows var_off, but bpf_dynptr_slice() needs tnum_is_const
and the patch is doing it explicitly.
Ideally some combination of __sz and __k is needed.
I don't like our suffix logic, but maybe let's do quick "__szk" for now ?
Then it can do normal __sz check and in addition do tnum_is_const() check
and set meta.arg_constant ?
Then 'for' will stay in place and regs[BPF_REG_4] won't be needed.
> +
> + /* PTR_MAYBE_NULL will be added when is_kfunc_ret_null is checked */
> + regs[BPF_REG_0].type = PTR_TO_MEM | type_flag;
> +
> + if (meta.func_id == special_kfunc_list[KF_bpf_dynptr_slice]) {
> + regs[BPF_REG_0].type |= MEM_RDONLY;
> + } else {
> + /* this will set env->seen_direct_write to true */
> + if (!may_access_direct_pkt_data(env, NULL, BPF_WRITE)) {
> + verbose(env, "the prog does not allow writes to packet data\n");
> + return -EINVAL;
> + }
> + }
> +
> + if (!meta.initialized_dynptr.id) {
> + verbose(env, "verifier internal error: no dynptr id\n");
> + return -EFAULT;
> + }
> + regs[BPF_REG_0].dynptr_id = meta.initialized_dynptr.id;
> +
> + /* we don't need to set BPF_REG_0's ref obj id
> + * because packet slices are not refcounted (see
> + * dynptr_type_refcounted)
> + */
> } else {
> verbose(env, "kernel function %s unhandled dynamic return type\n",
> meta.func_name);
> @@ -10121,6 +10203,9 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
> regs[BPF_REG_0].id = ++env->id_gen;
> } /* else { add_kfunc_call() ensures it is btf_type_is_void(t) } */
>
> + for (i = BPF_REG_1; i < CALLER_SAVED_REGS; i++)
> + mark_reg_not_init(env, regs, caller_saved[i]);
> +
