| Message ID | 20230101083403.332783-5-memxor@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Dynptr fixes | expand |
On Sun, Jan 1, 2023 at 12:34 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> 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? > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > --- > 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); > + > 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 >
On Wed, Jan 4, 2023 at 2:44 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Sun, Jan 1, 2023 at 12:34 AM Kumar Kartikeya Dwivedi > <memxor@gmail.com> 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@gmail.com> > > --- > > 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 > >
On Sat, Jan 07, 2023 at 01:03:24AM IST, Joanne Koong wrote: > On Wed, Jan 4, 2023 at 2:44 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Sun, Jan 1, 2023 at 12:34 AM Kumar Kartikeya Dwivedi > > <memxor@gmail.com> 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 > Ack, I'll make this change. > > > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > > --- > > > 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 > We do, I'll add a comment explaining this. There are two cases. [d1][d1][d2][d2] spi 3 2 1 0 If initializing on spi = 3, it will destroy d1, spi - 1 will see no STACK_DYNPTR and simply ignore the call. But in case we initialize on spi = 2, it will destroy d1 but not d2, only the next call will destroy the dynptr at d2. So for the case where we initialize over slots of two adjacent dynptrs, we'll miss that case without making the 2nd call. The call simply means 'destroy any dynptr which spi belongs to'. So it needs to be made for both, as both spi and spi-1 may belong to different dynptrs.
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); + 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; }
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. Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> --- kernel/bpf/verifier.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)