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?
> 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);
> +
> 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
>
