| Message ID | 20230119021442.1465269-5-memxor@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Dynptr fixes | expand |
On Wed, Jan 18, 2023 at 6:14 PM 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. > > The destroy_if_dynptr_stack_slot function already makes necessary checks > to avoid overwriting referenced dynptr slots. This is done to present a > better error message instead of forgetting dynptr information on stack > and preserving reference state, leading to an inevitable but > undecipherable error at the end about an unreleased reference which has > to be associated back to its allocating call instruction to make any > sense to the user. > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> Acked-by: Joanne Koong <joannelkoong@gmail.com> > --- > kernel/bpf/verifier.c | 34 ++++++++++++++++++++++++++-------- > 1 file changed, 26 insertions(+), 8 deletions(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 09c09d9bfd89..4feaddd5d6dc 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -777,7 +777,7 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_ > { > struct bpf_func_state *state = func(env, reg); > enum bpf_dynptr_type type; > - int spi, i, id; > + int spi, i, id, err; > > spi = dynptr_get_spi(env, reg); > if (spi < 0) > @@ -786,6 +786,22 @@ 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; > > + /* We cannot assume both spi and spi - 1 belong to the same dynptr, > + * hence we need to call destroy_if_dynptr_stack_slot twice for both, > + * to ensure that for the following example: > + * [d1][d1][d2][d2] > + * spi 3 2 1 0 > + * So marking spi = 2 should lead to destruction of both d1 and d2. In > + * case they do belong to same dynptr, second call won't see slot_type > + * as STACK_DYNPTR and will simply skip destruction. > + */ > + err = destroy_if_dynptr_stack_slot(env, state, spi); > + if (err) > + return err; > + err = destroy_if_dynptr_stack_slot(env, state, spi - 1); > + if (err) > + return err; > + > 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; > @@ -931,7 +947,7 @@ static int destroy_if_dynptr_stack_slot(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; > @@ -944,12 +960,14 @@ 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 unreferenced STACK_DYNPTR slots, see > + * mark_stack_slots_dynptr which calls destroy_if_dynptr_stack_slot to > + * ensure dynptr objects at the slots we are touching are completely > + * destructed before we reinitialize them for a new one. For referenced > + * ones, destroy_if_dynptr_stack_slot returns an error early instead of > + * delaying it until the end where the user will get "Unreleased > + * reference" error. > + */ > return true; > } > > -- > 2.39.1 >
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 09c09d9bfd89..4feaddd5d6dc 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -777,7 +777,7 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_ { struct bpf_func_state *state = func(env, reg); enum bpf_dynptr_type type; - int spi, i, id; + int spi, i, id, err; spi = dynptr_get_spi(env, reg); if (spi < 0) @@ -786,6 +786,22 @@ 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; + /* We cannot assume both spi and spi - 1 belong to the same dynptr, + * hence we need to call destroy_if_dynptr_stack_slot twice for both, + * to ensure that for the following example: + * [d1][d1][d2][d2] + * spi 3 2 1 0 + * So marking spi = 2 should lead to destruction of both d1 and d2. In + * case they do belong to same dynptr, second call won't see slot_type + * as STACK_DYNPTR and will simply skip destruction. + */ + err = destroy_if_dynptr_stack_slot(env, state, spi); + if (err) + return err; + err = destroy_if_dynptr_stack_slot(env, state, spi - 1); + if (err) + return err; + 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; @@ -931,7 +947,7 @@ static int destroy_if_dynptr_stack_slot(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; @@ -944,12 +960,14 @@ 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 unreferenced STACK_DYNPTR slots, see + * mark_stack_slots_dynptr which calls destroy_if_dynptr_stack_slot to + * ensure dynptr objects at the slots we are touching are completely + * destructed before we reinitialize them for a new one. For referenced + * ones, destroy_if_dynptr_stack_slot returns an error early instead of + * delaying it until the end where the user will get "Unreleased + * reference" error. + */ 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. The destroy_if_dynptr_stack_slot function already makes necessary checks to avoid overwriting referenced dynptr slots. This is done to present a better error message instead of forgetting dynptr information on stack and preserving reference state, leading to an inevitable but undecipherable error at the end about an unreleased reference which has to be associated back to its allocating call instruction to make any sense to the user. Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> --- kernel/bpf/verifier.c | 34 ++++++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 8 deletions(-)