Message ID | 20230120070355.1983560-5-memxor@gmail.com (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | BPF |
Headers | show |
Series | Dynptr fixes | expand |
On Thu, Jan 19, 2023 at 11:04 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > The previous commit implemented destroy_if_dynptr_stack_slot. It > destroys the dynptr which given spi belongs to, but still doesn't > invalidate the slices that belong to such a dynptr. While for the case > of referenced dynptr, we don't allow their overwrite and return an error > early, we still allow it and destroy the dynptr for unreferenced dynptr. > > To be able to enable precise and scoped invalidation of dynptr slices in > this case, we must be able to associate the source dynptr of slices that > have been obtained using bpf_dynptr_data. When doing destruction, only > slices belonging to the dynptr being destructed should be invalidated, > and nothing else. Currently, dynptr slices belonging to different > dynptrs are indistinguishible. > > Hence, allocate a unique id to each dynptr (CONST_PTR_TO_DYNPTR and > those on stack). This will be stored as part of reg->id. Whenever using > bpf_dynptr_data, transfer this unique dynptr id to the returned > PTR_TO_MEM_OR_NULL slice pointer, and store it in a new per-PTR_TO_MEM > dynptr_id register state member. > > Finally, after establishing such a relationship between dynptrs and > their slices, implement precise invalidation logic that only invalidates > slices belong to the destroyed dynptr in destroy_if_dynptr_stack_slot. > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> Acked-by: Joanne Koong <joannelkoong@gmail.com> > --- > include/linux/bpf_verifier.h | 5 +- > kernel/bpf/verifier.c | 74 ++++++++++++++++--- > .../testing/selftests/bpf/progs/dynptr_fail.c | 4 +- > 3 files changed, 68 insertions(+), 15 deletions(-) > > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h > index 127058cfec47..aa83de1fe755 100644 > --- a/include/linux/bpf_verifier.h > +++ b/include/linux/bpf_verifier.h > @@ -70,7 +70,10 @@ struct bpf_reg_state { > u32 btf_id; > }; > > - u32 mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ > + struct { /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ > + u32 mem_size; > + u32 dynptr_id; /* for dynptr slices */ > + }; > > /* For dynptr stack slots */ > struct { > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 5c7f29ca94ec..01cb802776fd 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -255,6 +255,7 @@ struct bpf_call_arg_meta { > int mem_size; > u64 msize_max_value; > int ref_obj_id; > + int dynptr_id; > int map_uid; > int func_id; > struct btf *btf; > @@ -750,23 +751,27 @@ static bool dynptr_type_refcounted(enum bpf_dynptr_type type) > > static void __mark_dynptr_reg(struct bpf_reg_state *reg, > enum bpf_dynptr_type type, > - bool first_slot); > + bool first_slot, int dynptr_id); > > static void __mark_reg_not_init(const struct bpf_verifier_env *env, > struct bpf_reg_state *reg); > > -static void mark_dynptr_stack_regs(struct bpf_reg_state *sreg1, > +static void mark_dynptr_stack_regs(struct bpf_verifier_env *env, > + struct bpf_reg_state *sreg1, > struct bpf_reg_state *sreg2, > enum bpf_dynptr_type type) > { > - __mark_dynptr_reg(sreg1, type, true); > - __mark_dynptr_reg(sreg2, type, false); > + int id = ++env->id_gen; > + > + __mark_dynptr_reg(sreg1, type, true, id); > + __mark_dynptr_reg(sreg2, type, false, id); > } > > -static void mark_dynptr_cb_reg(struct bpf_reg_state *reg, > +static void mark_dynptr_cb_reg(struct bpf_verifier_env *env, > + struct bpf_reg_state *reg, > enum bpf_dynptr_type type) > { > - __mark_dynptr_reg(reg, type, true); > + __mark_dynptr_reg(reg, type, true, ++env->id_gen); > } > > static int destroy_if_dynptr_stack_slot(struct bpf_verifier_env *env, > @@ -795,7 +800,7 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_ > if (type == BPF_DYNPTR_TYPE_INVALID) > return -EINVAL; > > - mark_dynptr_stack_regs(&state->stack[spi].spilled_ptr, > + mark_dynptr_stack_regs(env, &state->stack[spi].spilled_ptr, > &state->stack[spi - 1].spilled_ptr, type); > > if (dynptr_type_refcounted(type)) { > @@ -871,7 +876,9 @@ static void __mark_reg_unknown(const struct bpf_verifier_env *env, > static int destroy_if_dynptr_stack_slot(struct bpf_verifier_env *env, > struct bpf_func_state *state, int spi) > { > - int i; > + struct bpf_func_state *fstate; > + struct bpf_reg_state *dreg; > + int i, dynptr_id; > > /* We always ensure that STACK_DYNPTR is never set partially, > * hence just checking for slot_type[0] is enough. This is > @@ -899,7 +906,19 @@ static int destroy_if_dynptr_stack_slot(struct bpf_verifier_env *env, > state->stack[spi - 1].slot_type[i] = STACK_INVALID; > } > > - /* TODO: Invalidate any slices associated with this dynptr */ > + dynptr_id = state->stack[spi].spilled_ptr.id; > + /* Invalidate any slices associated with this dynptr */ > + bpf_for_each_reg_in_vstate(env->cur_state, fstate, dreg, ({ > + /* Dynptr slices are only PTR_TO_MEM_OR_NULL and PTR_TO_MEM */ > + if (dreg->type != (PTR_TO_MEM | PTR_MAYBE_NULL) && dreg->type != PTR_TO_MEM) > + continue; > + if (dreg->dynptr_id == dynptr_id) { > + if (!env->allow_ptr_leaks) > + __mark_reg_not_init(env, dreg); > + else > + __mark_reg_unknown(env, dreg); > + } > + })); > > /* Do not release reference state, we are destroying dynptr on stack, > * not using some helper to release it. Just reset register. > @@ -1562,7 +1581,7 @@ static void mark_reg_known_zero(struct bpf_verifier_env *env, > } > > static void __mark_dynptr_reg(struct bpf_reg_state *reg, enum bpf_dynptr_type type, > - bool first_slot) > + bool first_slot, int dynptr_id) > { > /* reg->type has no meaning for STACK_DYNPTR, but when we set reg for > * callback arguments, it does need to be CONST_PTR_TO_DYNPTR, so simply > @@ -1570,6 +1589,8 @@ static void __mark_dynptr_reg(struct bpf_reg_state *reg, enum bpf_dynptr_type ty > */ > __mark_reg_known_zero(reg); > reg->type = CONST_PTR_TO_DYNPTR; > + /* Give each dynptr a unique id to uniquely associate slices to it. */ > + reg->id = dynptr_id; > reg->dynptr.type = type; > reg->dynptr.first_slot = first_slot; > } > @@ -6532,6 +6553,19 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env, > } > } > > +static int dynptr_id(struct bpf_verifier_env *env, struct bpf_reg_state *reg) > +{ > + struct bpf_func_state *state = func(env, reg); > + int spi; > + > + if (reg->type == CONST_PTR_TO_DYNPTR) > + return reg->id; > + spi = dynptr_get_spi(env, reg); > + if (spi < 0) > + return spi; > + return state->stack[spi].spilled_ptr.id; > +} > + > static int dynptr_ref_obj_id(struct bpf_verifier_env *env, struct bpf_reg_state *reg) > { > struct bpf_func_state *state = func(env, reg); > @@ -7601,7 +7635,7 @@ static int set_user_ringbuf_callback_state(struct bpf_verifier_env *env, > * callback_fn(const struct bpf_dynptr_t* dynptr, void *callback_ctx); > */ > __mark_reg_not_init(env, &callee->regs[BPF_REG_0]); > - mark_dynptr_cb_reg(&callee->regs[BPF_REG_1], BPF_DYNPTR_TYPE_LOCAL); > + mark_dynptr_cb_reg(env, &callee->regs[BPF_REG_1], BPF_DYNPTR_TYPE_LOCAL); > callee->regs[BPF_REG_2] = caller->regs[BPF_REG_3]; > > /* unused */ > @@ -8107,18 +8141,31 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn > for (i = 0; i < MAX_BPF_FUNC_REG_ARGS; i++) { > if (arg_type_is_dynptr(fn->arg_type[i])) { > struct bpf_reg_state *reg = ®s[BPF_REG_1 + i]; > - int ref_obj_id; > + int id, ref_obj_id; > + > + if (meta.dynptr_id) { > + verbose(env, "verifier internal error: meta.dynptr_id already set\n"); > + return -EFAULT; > + } > > if (meta.ref_obj_id) { > verbose(env, "verifier internal error: meta.ref_obj_id already set\n"); > return -EFAULT; > } > > + id = dynptr_id(env, reg); > + if (id < 0) { > + verbose(env, "verifier internal error: failed to obtain dynptr id\n"); > + return id; > + } > + > ref_obj_id = dynptr_ref_obj_id(env, reg); > if (ref_obj_id < 0) { > verbose(env, "verifier internal error: failed to obtain dynptr ref_obj_id\n"); > return ref_obj_id; > } > + > + meta.dynptr_id = id; > meta.ref_obj_id = ref_obj_id; > break; > } > @@ -8275,6 +8322,9 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn > return -EFAULT; > } > > + if (is_dynptr_ref_function(func_id)) > + regs[BPF_REG_0].dynptr_id = meta.dynptr_id; > + > if (is_ptr_cast_function(func_id) || is_dynptr_ref_function(func_id)) { > /* For release_reference() */ > regs[BPF_REG_0].ref_obj_id = meta.ref_obj_id; > diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c > index 9dc3f23a8270..e43000c63c66 100644 > --- a/tools/testing/selftests/bpf/progs/dynptr_fail.c > +++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c > @@ -67,7 +67,7 @@ static int get_map_val_dynptr(struct bpf_dynptr *ptr) > * bpf_ringbuf_submit/discard_dynptr call > */ > SEC("?raw_tp") > -__failure __msg("Unreleased reference id=1") > +__failure __msg("Unreleased reference id=2") > int ringbuf_missing_release1(void *ctx) > { > struct bpf_dynptr ptr; > @@ -80,7 +80,7 @@ int ringbuf_missing_release1(void *ctx) > } > > SEC("?raw_tp") > -__failure __msg("Unreleased reference id=2") > +__failure __msg("Unreleased reference id=4") > int ringbuf_missing_release2(void *ctx) > { > struct bpf_dynptr ptr1, ptr2; > -- > 2.39.1 >
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h index 127058cfec47..aa83de1fe755 100644 --- a/include/linux/bpf_verifier.h +++ b/include/linux/bpf_verifier.h @@ -70,7 +70,10 @@ struct bpf_reg_state { u32 btf_id; }; - u32 mem_size; /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ + struct { /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */ + u32 mem_size; + u32 dynptr_id; /* for dynptr slices */ + }; /* For dynptr stack slots */ struct { diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 5c7f29ca94ec..01cb802776fd 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -255,6 +255,7 @@ struct bpf_call_arg_meta { int mem_size; u64 msize_max_value; int ref_obj_id; + int dynptr_id; int map_uid; int func_id; struct btf *btf; @@ -750,23 +751,27 @@ static bool dynptr_type_refcounted(enum bpf_dynptr_type type) static void __mark_dynptr_reg(struct bpf_reg_state *reg, enum bpf_dynptr_type type, - bool first_slot); + bool first_slot, int dynptr_id); static void __mark_reg_not_init(const struct bpf_verifier_env *env, struct bpf_reg_state *reg); -static void mark_dynptr_stack_regs(struct bpf_reg_state *sreg1, +static void mark_dynptr_stack_regs(struct bpf_verifier_env *env, + struct bpf_reg_state *sreg1, struct bpf_reg_state *sreg2, enum bpf_dynptr_type type) { - __mark_dynptr_reg(sreg1, type, true); - __mark_dynptr_reg(sreg2, type, false); + int id = ++env->id_gen; + + __mark_dynptr_reg(sreg1, type, true, id); + __mark_dynptr_reg(sreg2, type, false, id); } -static void mark_dynptr_cb_reg(struct bpf_reg_state *reg, +static void mark_dynptr_cb_reg(struct bpf_verifier_env *env, + struct bpf_reg_state *reg, enum bpf_dynptr_type type) { - __mark_dynptr_reg(reg, type, true); + __mark_dynptr_reg(reg, type, true, ++env->id_gen); } static int destroy_if_dynptr_stack_slot(struct bpf_verifier_env *env, @@ -795,7 +800,7 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_ if (type == BPF_DYNPTR_TYPE_INVALID) return -EINVAL; - mark_dynptr_stack_regs(&state->stack[spi].spilled_ptr, + mark_dynptr_stack_regs(env, &state->stack[spi].spilled_ptr, &state->stack[spi - 1].spilled_ptr, type); if (dynptr_type_refcounted(type)) { @@ -871,7 +876,9 @@ static void __mark_reg_unknown(const struct bpf_verifier_env *env, static int destroy_if_dynptr_stack_slot(struct bpf_verifier_env *env, struct bpf_func_state *state, int spi) { - int i; + struct bpf_func_state *fstate; + struct bpf_reg_state *dreg; + int i, dynptr_id; /* We always ensure that STACK_DYNPTR is never set partially, * hence just checking for slot_type[0] is enough. This is @@ -899,7 +906,19 @@ static int destroy_if_dynptr_stack_slot(struct bpf_verifier_env *env, state->stack[spi - 1].slot_type[i] = STACK_INVALID; } - /* TODO: Invalidate any slices associated with this dynptr */ + dynptr_id = state->stack[spi].spilled_ptr.id; + /* Invalidate any slices associated with this dynptr */ + bpf_for_each_reg_in_vstate(env->cur_state, fstate, dreg, ({ + /* Dynptr slices are only PTR_TO_MEM_OR_NULL and PTR_TO_MEM */ + if (dreg->type != (PTR_TO_MEM | PTR_MAYBE_NULL) && dreg->type != PTR_TO_MEM) + continue; + if (dreg->dynptr_id == dynptr_id) { + if (!env->allow_ptr_leaks) + __mark_reg_not_init(env, dreg); + else + __mark_reg_unknown(env, dreg); + } + })); /* Do not release reference state, we are destroying dynptr on stack, * not using some helper to release it. Just reset register. @@ -1562,7 +1581,7 @@ static void mark_reg_known_zero(struct bpf_verifier_env *env, } static void __mark_dynptr_reg(struct bpf_reg_state *reg, enum bpf_dynptr_type type, - bool first_slot) + bool first_slot, int dynptr_id) { /* reg->type has no meaning for STACK_DYNPTR, but when we set reg for * callback arguments, it does need to be CONST_PTR_TO_DYNPTR, so simply @@ -1570,6 +1589,8 @@ static void __mark_dynptr_reg(struct bpf_reg_state *reg, enum bpf_dynptr_type ty */ __mark_reg_known_zero(reg); reg->type = CONST_PTR_TO_DYNPTR; + /* Give each dynptr a unique id to uniquely associate slices to it. */ + reg->id = dynptr_id; reg->dynptr.type = type; reg->dynptr.first_slot = first_slot; } @@ -6532,6 +6553,19 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env, } } +static int dynptr_id(struct bpf_verifier_env *env, struct bpf_reg_state *reg) +{ + struct bpf_func_state *state = func(env, reg); + int spi; + + if (reg->type == CONST_PTR_TO_DYNPTR) + return reg->id; + spi = dynptr_get_spi(env, reg); + if (spi < 0) + return spi; + return state->stack[spi].spilled_ptr.id; +} + static int dynptr_ref_obj_id(struct bpf_verifier_env *env, struct bpf_reg_state *reg) { struct bpf_func_state *state = func(env, reg); @@ -7601,7 +7635,7 @@ static int set_user_ringbuf_callback_state(struct bpf_verifier_env *env, * callback_fn(const struct bpf_dynptr_t* dynptr, void *callback_ctx); */ __mark_reg_not_init(env, &callee->regs[BPF_REG_0]); - mark_dynptr_cb_reg(&callee->regs[BPF_REG_1], BPF_DYNPTR_TYPE_LOCAL); + mark_dynptr_cb_reg(env, &callee->regs[BPF_REG_1], BPF_DYNPTR_TYPE_LOCAL); callee->regs[BPF_REG_2] = caller->regs[BPF_REG_3]; /* unused */ @@ -8107,18 +8141,31 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn for (i = 0; i < MAX_BPF_FUNC_REG_ARGS; i++) { if (arg_type_is_dynptr(fn->arg_type[i])) { struct bpf_reg_state *reg = ®s[BPF_REG_1 + i]; - int ref_obj_id; + int id, ref_obj_id; + + if (meta.dynptr_id) { + verbose(env, "verifier internal error: meta.dynptr_id already set\n"); + return -EFAULT; + } if (meta.ref_obj_id) { verbose(env, "verifier internal error: meta.ref_obj_id already set\n"); return -EFAULT; } + id = dynptr_id(env, reg); + if (id < 0) { + verbose(env, "verifier internal error: failed to obtain dynptr id\n"); + return id; + } + ref_obj_id = dynptr_ref_obj_id(env, reg); if (ref_obj_id < 0) { verbose(env, "verifier internal error: failed to obtain dynptr ref_obj_id\n"); return ref_obj_id; } + + meta.dynptr_id = id; meta.ref_obj_id = ref_obj_id; break; } @@ -8275,6 +8322,9 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn return -EFAULT; } + if (is_dynptr_ref_function(func_id)) + regs[BPF_REG_0].dynptr_id = meta.dynptr_id; + if (is_ptr_cast_function(func_id) || is_dynptr_ref_function(func_id)) { /* For release_reference() */ regs[BPF_REG_0].ref_obj_id = meta.ref_obj_id; diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c index 9dc3f23a8270..e43000c63c66 100644 --- a/tools/testing/selftests/bpf/progs/dynptr_fail.c +++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c @@ -67,7 +67,7 @@ static int get_map_val_dynptr(struct bpf_dynptr *ptr) * bpf_ringbuf_submit/discard_dynptr call */ SEC("?raw_tp") -__failure __msg("Unreleased reference id=1") +__failure __msg("Unreleased reference id=2") int ringbuf_missing_release1(void *ctx) { struct bpf_dynptr ptr; @@ -80,7 +80,7 @@ int ringbuf_missing_release1(void *ctx) } SEC("?raw_tp") -__failure __msg("Unreleased reference id=2") +__failure __msg("Unreleased reference id=4") int ringbuf_missing_release2(void *ctx) { struct bpf_dynptr ptr1, ptr2;
The previous commit implemented destroy_if_dynptr_stack_slot. It destroys the dynptr which given spi belongs to, but still doesn't invalidate the slices that belong to such a dynptr. While for the case of referenced dynptr, we don't allow their overwrite and return an error early, we still allow it and destroy the dynptr for unreferenced dynptr. To be able to enable precise and scoped invalidation of dynptr slices in this case, we must be able to associate the source dynptr of slices that have been obtained using bpf_dynptr_data. When doing destruction, only slices belonging to the dynptr being destructed should be invalidated, and nothing else. Currently, dynptr slices belonging to different dynptrs are indistinguishible. Hence, allocate a unique id to each dynptr (CONST_PTR_TO_DYNPTR and those on stack). This will be stored as part of reg->id. Whenever using bpf_dynptr_data, transfer this unique dynptr id to the returned PTR_TO_MEM_OR_NULL slice pointer, and store it in a new per-PTR_TO_MEM dynptr_id register state member. Finally, after establishing such a relationship between dynptrs and their slices, implement precise invalidation logic that only invalidates slices belong to the destroyed dynptr in destroy_if_dynptr_stack_slot. Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> --- include/linux/bpf_verifier.h | 5 +- kernel/bpf/verifier.c | 74 ++++++++++++++++--- .../testing/selftests/bpf/progs/dynptr_fail.c | 4 +- 3 files changed, 68 insertions(+), 15 deletions(-)