On Wed, Jan 4, 2023 at 2:44 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Sun, Jan 1, 2023 at 12:34 AM Kumar Kartikeya Dwivedi
> <memxor@xxxxxxxxx> wrote:
> >
> > Consider a program like below:
> >
> > void prog(void)
> > {
> > {
> > struct bpf_dynptr ptr;
> > bpf_dynptr_from_mem(...);
> > }
> > ...
> > {
> > struct bpf_dynptr ptr;
> > bpf_dynptr_from_mem(...);
> > }
> > }
> >
> > Here, the C compiler based on lifetime rules in the C standard would be
> > well within in its rights to share stack storage for dynptr 'ptr' as
> > their lifetimes do not overlap in the two distinct scopes. Currently,
> > such an example would be rejected by the verifier, but this is too
> > strict. Instead, we should allow reinitializing over dynptr stack slots
> > and forget information about the old dynptr object.
> >
>
> As mentioned in the previous patch, shouldn't we allow this only for
> dynptrs that don't require OBJ_RELEASE, which would be those with
> ref_obj_id == 0?
>
+1
>
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> > ---
> > kernel/bpf/verifier.c | 16 +++++++++-------
> > 1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index b985d90505cc..e85e8c4be00d 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -786,6 +786,9 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_
> > if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS))
> > return -EINVAL;
> >
> > + destroy_stack_slots_dynptr(env, state, spi);
> > + destroy_stack_slots_dynptr(env, state, spi - 1);
We don't need the 2nd call since destroy_slots_dynptr() destroys both slots
> > +
> > for (i = 0; i < BPF_REG_SIZE; i++) {
> > state->stack[spi].slot_type[i] = STACK_DYNPTR;
> > state->stack[spi - 1].slot_type[i] = STACK_DYNPTR;
> > @@ -901,7 +904,7 @@ static void destroy_stack_slots_dynptr(struct bpf_verifier_env *env,
> > static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> > {
> > struct bpf_func_state *state = func(env, reg);
> > - int spi, i;
> > + int spi;
> >
> > if (reg->type == CONST_PTR_TO_DYNPTR)
> > return false;
> > @@ -914,12 +917,11 @@ static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_
> > if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS))
> > return true;
> >
> > - for (i = 0; i < BPF_REG_SIZE; i++) {
> > - if (state->stack[spi].slot_type[i] == STACK_DYNPTR ||
> > - state->stack[spi - 1].slot_type[i] == STACK_DYNPTR)
> > - return false;
> > - }
> > -
> > + /* We allow overwriting existing STACK_DYNPTR slots, see
> > + * mark_stack_slots_dynptr which calls destroy_stack_slots_dynptr to
> > + * ensure dynptr objects at the slots we are touching are completely
> > + * destructed before we reinitialize them for a new one.
> > + */
> > return true;
> > }
> >
> > --
> > 2.39.0
> >
