On Mon, Jul 25, 2022 at 12:10 PM Martin KaFai Lau <kafai@xxxxxx> wrote:
>
> On Fri, Jul 22, 2022 at 10:58:06AM -0700, Joanne Koong wrote:
> > When a data slice is obtained from a dynptr (through the bpf_dynptr_data API),
> > the ref obj id of the dynptr must be found and then associated with the data
> > slice.
> >
> > The ref obj id of the dynptr must be found *before* the caller saved regs are
> > reset. Without this fix, the ref obj id tracking is not correct for
> > dynptrs that are at an offset from the frame pointer.
> >
> > Please also note that the data slice's ref obj id must be assigned after the
> > ret types are parsed, since RET_PTR_TO_ALLOC_MEM-type return regs get
> > zero-marked.
> >
> > Fixes: 34d4ef5775f776ec4b0d53a02d588bf3195cada6 ("bpf: Add dynptr data slices");
> > Signed-off-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> > ---
> > kernel/bpf/verifier.c | 62 ++++++++++++++++++++-----------------------
> > 1 file changed, 29 insertions(+), 33 deletions(-)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index c59c3df0fea6..29987b2ea26f 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -5830,7 +5830,8 @@ static u32 stack_slot_get_id(struct bpf_verifier_env *env, struct bpf_reg_state
> >
> > static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> > struct bpf_call_arg_meta *meta,
> > - const struct bpf_func_proto *fn)
> > + const struct bpf_func_proto *fn,
> > + int func_id)
> > {
> > u32 regno = BPF_REG_1 + arg;
> > struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno];
> > @@ -6040,23 +6041,33 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> > }
> >
> > meta->uninit_dynptr_regno = regno;
> > - } else if (!is_dynptr_reg_valid_init(env, reg, arg_type)) {
> > - const char *err_extra = "";
> > + } else {
> > + if (!is_dynptr_reg_valid_init(env, reg, arg_type)) {
> > + const char *err_extra = "";
> >
> > - switch (arg_type & DYNPTR_TYPE_FLAG_MASK) {
> > - case DYNPTR_TYPE_LOCAL:
> > - err_extra = "local ";
> > - break;
> > - case DYNPTR_TYPE_RINGBUF:
> > - err_extra = "ringbuf ";
> > - break;
> > - default:
> > - break;
> > - }
> > + switch (arg_type & DYNPTR_TYPE_FLAG_MASK) {
> > + case DYNPTR_TYPE_LOCAL:
> > + err_extra = "local ";
> > + break;
> > + case DYNPTR_TYPE_RINGBUF:
> > + err_extra = "ringbuf ";
> > + break;
> > + default:
> > + break;
> > + }
> >
> > - verbose(env, "Expected an initialized %sdynptr as arg #%d\n",
> > - err_extra, arg + 1);
> > - return -EINVAL;
> > + verbose(env, "Expected an initialized %sdynptr as arg #%d\n",
> > + err_extra, arg + 1);
> > + return -EINVAL;
> > + }
> > + if (func_id == BPF_FUNC_dynptr_data) {
> > + if (meta->ref_obj_id) {
> > + verbose(env, "verifier internal error: multiple refcounted args in BPF_FUNC_dynptr_data");
> If 'func_id == BPF_FUNC_dynptr_data' is not checked first,
> this verbose (or the earlier one in the 'if (reg->ref_obj_id) {...}')
> may be hit for the bpf_dynptr_write helper?
If the 'func_id == BPF_FUNC_dynptr_data' is not checked first, the
bpf_dynptr_write helper may hit the verbose if the source it's writing
from is ref-counted (for example if the source is a ringbuf record).
bpf_dynptr_write doesn't trigger the earlier "if (reg->ref_obj_id)"
case when the source is ref-counted because the dynptr isn't stored in
a reg; the dynptr's refcount is stored on the stack since the dynptr
is stored on the stack, so in that case there is only 1
reg->ref_obj_id (belonging to the src) found for bpf_dynptr_write.
>
> Overall lgtm.
>
> Acked-by: Martin KaFai Lau <kafai@xxxxxx>
>
>
> > + return -EFAULT;
> > + }
> > + /* Find the id of the dynptr we're tracking the reference of */
> > + meta->ref_obj_id = stack_slot_get_id(env, reg);
> > + }
> > }
> > break;
> > case ARG_CONST_ALLOC_SIZE_OR_ZERO:
> > @@ -7227,7 +7238,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
> > meta.func_id = func_id;
> > /* check args */
> > for (i = 0; i < MAX_BPF_FUNC_REG_ARGS; i++) {
> > - err = check_func_arg(env, i, &meta, fn);
> > + err = check_func_arg(env, i, &meta, fn, func_id);
> > if (err)
> > return err;
> > }
> > @@ -7457,7 +7468,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
> > if (type_may_be_null(regs[BPF_REG_0].type))
> > regs[BPF_REG_0].id = ++env->id_gen;
> >
> > - if (is_ptr_cast_function(func_id)) {
> > + if (is_ptr_cast_function(func_id) || func_id == BPF_FUNC_dynptr_data) {
> > /* For release_reference() */
> > regs[BPF_REG_0].ref_obj_id = meta.ref_obj_id;
> > } else if (is_acquire_function(func_id, meta.map_ptr)) {
