On Sat, Mar 26, 2022 at 05:09:52AM IST, Martin KaFai Lau wrote:
> On Fri, Mar 25, 2022 at 08:27:00PM +0530, Kumar Kartikeya Dwivedi wrote:
> > On Wed, Mar 23, 2022 at 02:29:12AM IST, Martin KaFai Lau wrote:
> > > On Sun, Mar 20, 2022 at 09:25:02PM +0530, Kumar Kartikeya Dwivedi wrote:
> > > > static int map_kptr_match_type(struct bpf_verifier_env *env,
> > > > struct bpf_map_value_off_desc *off_desc,
> > > > - struct bpf_reg_state *reg, u32 regno)
> > > > + struct bpf_reg_state *reg, u32 regno,
> > > > + bool ref_ptr)
> > > > {
> > > > const char *targ_name = kernel_type_name(off_desc->btf, off_desc->btf_id);
> > > > const char *reg_name = "";
> > > > + bool fixed_off_ok = true;
> > > >
> > > > if (reg->type != PTR_TO_BTF_ID && reg->type != PTR_TO_BTF_ID_OR_NULL)
> > > > goto bad_type;
> > > > @@ -3525,7 +3530,26 @@ static int map_kptr_match_type(struct bpf_verifier_env *env,
> > > > /* We need to verify reg->type and reg->btf, before accessing reg->btf */
> > > > reg_name = kernel_type_name(reg->btf, reg->btf_id);
> > > >
> > > > - if (__check_ptr_off_reg(env, reg, regno, true))
> > > > + if (ref_ptr) {
> > > > + if (!reg->ref_obj_id) {
> > > > + verbose(env, "R%d must be referenced %s%s\n", regno,
> > > > + reg_type_str(env, PTR_TO_BTF_ID), targ_name);
> > > > + return -EACCES;
> > > > + }
> > > The is_release_function() checkings under check_helper_call() is
> > > not the same?
> > >
> > > > + /* reg->off can be used to store pointer to a certain type formed by
> > > > + * incrementing pointer of a parent structure the object is embedded in,
> > > > + * e.g. map may expect unreferenced struct path *, and user should be
> > > > + * allowed a store using &file->f_path. However, in the case of
> > > > + * referenced pointer, we cannot do this, because the reference is only
> > > > + * for the parent structure, not its embedded object(s), and because
> > > > + * the transfer of ownership happens for the original pointer to and
> > > > + * from the map (before its eventual release).
> > > > + */
> > > > + if (reg->off)
> > > > + fixed_off_ok = false;
> > > I thought the new check_func_arg_reg_off() is supposed to handle the
> > > is_release_function() case. The check_func_arg_reg_off() called
> > > in check_func_arg() can not handle this case?
> > >
> >
> > The difference there is, it wouldn't check for reg->off == 0 if reg->ref_obj_id
> > is 0.
> If ref_obj_id is not 0, check_func_arg_reg_off() will reject reg->off.
> check_func_arg_reg_off is called after check_reg_type().
>
> If ref_obj_id is 0, the is_release_function() check in the
> check_helper_call() should complain:
> verbose(env, "func %s#%d reference has not been acquired before\n",
> func_id_name(func_id), func_id);
>
> I am quite confused why it needs special reg->off and
> reg->ref_obj_id checking here for the map_kptr helper taking
> PTR_TO_BTF_ID arg but not other helper taking PTR_TO_BTF_ID arg.
> The existing checks for the other helper taking PTR_TO_BTF_ID
> arg is not enough?
>
Yes, you're right, it should be enough. We just need to check for the normal
case here, with fixed_off_ok = true, since that can come from BPF_STX. In the
referenced case, earlier it was also possible to store using BPF_XCHG, but not
anymore, so now the checks for the helper should be enough, and complain.
Will drop this in v4, and just keep __check_ptr_off_reg(env, reg, regno, false).
> > So in that case, I should probably check reg->ref_obj_id to be non-zero
> > when ref_ptr is true, and then call check_func_arg_reg_off, with the comment
> > that this would eventually be an argument to the release function, so the
> > argument should be checked with check_func_arg_reg_off.
>
>
>
> >
> > > > + }
> > > > + /* var_off is rejected by __check_ptr_off_reg for PTR_TO_BTF_ID */
> > > > + if (__check_ptr_off_reg(env, reg, regno, fixed_off_ok))
> > > > return -EACCES;
> > > >
> > > > if (!btf_struct_ids_match(&env->log, reg->btf, reg->btf_id, reg->off,
> > >
> > > [ ... ]
> > >
> > > > @@ -5390,6 +5473,7 @@ static const struct bpf_reg_types func_ptr_types = { .types = { PTR_TO_FUNC } };
> > > > static const struct bpf_reg_types stack_ptr_types = { .types = { PTR_TO_STACK } };
> > > > static const struct bpf_reg_types const_str_ptr_types = { .types = { PTR_TO_MAP_VALUE } };
> > > > static const struct bpf_reg_types timer_types = { .types = { PTR_TO_MAP_VALUE } };
> > > > +static const struct bpf_reg_types kptr_types = { .types = { PTR_TO_MAP_VALUE } };
> > > >
> > > > static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = {
> > > > [ARG_PTR_TO_MAP_KEY] = &map_key_value_types,
> > > > @@ -5417,11 +5501,13 @@ static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = {
> > > > [ARG_PTR_TO_STACK] = &stack_ptr_types,
> > > > [ARG_PTR_TO_CONST_STR] = &const_str_ptr_types,
> > > > [ARG_PTR_TO_TIMER] = &timer_types,
> > > > + [ARG_PTR_TO_KPTR] = &kptr_types,
> > > > };
> > > >
> > > > static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
> > > > enum bpf_arg_type arg_type,
> > > > - const u32 *arg_btf_id)
> > > > + const u32 *arg_btf_id,
> > > > + struct bpf_call_arg_meta *meta)
> > > > {
> > > > struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno];
> > > > enum bpf_reg_type expected, type = reg->type;
> > > > @@ -5474,8 +5560,11 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
> > > > arg_btf_id = compatible->btf_id;
> > > > }
> > > >
> > > > - if (!btf_struct_ids_match(&env->log, reg->btf, reg->btf_id, reg->off,
> > > > - btf_vmlinux, *arg_btf_id)) {
> > > > + if (meta->func_id == BPF_FUNC_kptr_xchg) {
> > > > + if (map_kptr_match_type(env, meta->kptr_off_desc, reg, regno, true))
> > > > + return -EACCES;
> > > > + } else if (!btf_struct_ids_match(&env->log, reg->btf, reg->btf_id, reg->off,
> > > > + btf_vmlinux, *arg_btf_id)) {
> > > > verbose(env, "R%d is of type %s but %s is expected\n",
> > > > regno, kernel_type_name(reg->btf, reg->btf_id),
> > > > kernel_type_name(btf_vmlinux, *arg_btf_id));
> >
> > --
> > Kartikeya
--
Kartikeya
