On Wed, Jan 18, 2023 at 6:14 PM 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.
>
> 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@xxxxxxxxx>
Acked-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> ---
> 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
>
