On Wed, Jan 18, 2023 at 6:15 PM Kumar Kartikeya Dwivedi
<memxor@xxxxxxxxx> wrote:
>
> Currently, a check on spi resides in dynptr_get_spi, while others
> checking its validity for being within the allocated stack slots happens
> in is_spi_bounds_valid. Almost always barring a couple of cases (where
> being beyond allocated stack slots is not an error as stack slots need
> to be populated), both are used together to make checks. Hence, subsume
> the is_spi_bounds_valid check in dynptr_get_spi, and return -ERANGE to
> specially distinguish the case where spi is valid but not within
> allocated slots in the stack state.
>
> The is_spi_bounds_valid function is still kept around as it is a generic
> helper that will be useful for other objects on stack similar to dynptr
> in the future.
>
> Suggested-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
Acked-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> ---
> kernel/bpf/verifier.c | 75 +++++++++++++++++++------------------------
> 1 file changed, 33 insertions(+), 42 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 4feaddd5d6dc..18b54b219fac 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -643,6 +643,28 @@ static int __get_spi(s32 off)
> return (-off - 1) / BPF_REG_SIZE;
> }
>
> +static struct bpf_func_state *func(struct bpf_verifier_env *env,
> + const struct bpf_reg_state *reg)
> +{
> + struct bpf_verifier_state *cur = env->cur_state;
> +
> + return cur->frame[reg->frameno];
> +}
> +
> +static bool is_spi_bounds_valid(struct bpf_func_state *state, int spi, int nr_slots)
> +{
> + int allocated_slots = state->allocated_stack / BPF_REG_SIZE;
> +
> + /* We need to check that slots between [spi - nr_slots + 1, spi] are
> + * within [0, allocated_stack).
> + *
> + * Please note that the spi grows downwards. For example, a dynptr
> + * takes the size of two stack slots; the first slot will be at
> + * spi and the second slot will be at spi - 1.
> + */
> + return spi - nr_slots + 1 >= 0 && spi < allocated_slots;
> +}
> +
> static int dynptr_get_spi(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> {
> int off, spi;
> @@ -663,29 +685,10 @@ static int dynptr_get_spi(struct bpf_verifier_env *env, struct bpf_reg_state *re
> verbose(env, "cannot pass in dynptr at an offset=%d\n", off);
> return -EINVAL;
> }
> - return spi;
> -}
> -
> -static bool is_spi_bounds_valid(struct bpf_func_state *state, int spi, int nr_slots)
> -{
> - int allocated_slots = state->allocated_stack / BPF_REG_SIZE;
>
> - /* We need to check that slots between [spi - nr_slots + 1, spi] are
> - * within [0, allocated_stack).
> - *
> - * Please note that the spi grows downwards. For example, a dynptr
> - * takes the size of two stack slots; the first slot will be at
> - * spi and the second slot will be at spi - 1.
> - */
> - return spi - nr_slots + 1 >= 0 && spi < allocated_slots;
> -}
> -
> -static struct bpf_func_state *func(struct bpf_verifier_env *env,
> - const struct bpf_reg_state *reg)
> -{
> - struct bpf_verifier_state *cur = env->cur_state;
> -
> - return cur->frame[reg->frameno];
> + if (!is_spi_bounds_valid(func(env, reg), spi, BPF_DYNPTR_NR_SLOTS))
> + return -ERANGE;
> + return spi;
> }
>
> static const char *kernel_type_name(const struct btf* btf, u32 id)
> @@ -783,9 +786,6 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_
> if (spi < 0)
> return spi;
>
> - 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:
> @@ -839,9 +839,6 @@ static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_re
> if (spi < 0)
> return spi;
>
> - if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS))
> - return -EINVAL;
> -
> for (i = 0; i < BPF_REG_SIZE; i++) {
> state->stack[spi].slot_type[i] = STACK_INVALID;
> state->stack[spi - 1].slot_type[i] = STACK_INVALID;
> @@ -946,20 +943,18 @@ 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;
>
> if (reg->type == CONST_PTR_TO_DYNPTR)
> return false;
>
> spi = dynptr_get_spi(env, reg);
> + /* For -ERANGE (i.e. spi not falling into allocated stack slots), we
> + * will do check_mem_access to check and update stack bounds later, so
> + * return true for that case.
> + */
> if (spi < 0)
> - return false;
> -
> - /* We will do check_mem_access to check and update stack bounds later */
> - if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS))
> - return true;
> -
> + return spi == -ERANGE;
> /* 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
> @@ -983,8 +978,7 @@ static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_re
> spi = dynptr_get_spi(env, reg);
> if (spi < 0)
> return false;
> - if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS) ||
> - !state->stack[spi].spilled_ptr.dynptr.first_slot)
> + if (!state->stack[spi].spilled_ptr.dynptr.first_slot)
> return false;
>
> for (i = 0; i < BPF_REG_SIZE; i++) {
> @@ -6153,7 +6147,7 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno,
> */
> if (reg->type == PTR_TO_STACK) {
> err = dynptr_get_spi(env, reg);
> - if (err < 0)
> + if (err < 0 && err != -ERANGE)
> return err;
> }
>
> @@ -6646,10 +6640,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> */
> if (reg->type == PTR_TO_STACK) {
> spi = dynptr_get_spi(env, reg);
> - if (spi < 0)
> - return spi;
> - if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS) ||
> - !state->stack[spi].spilled_ptr.ref_obj_id) {
> + if (spi < 0 || !state->stack[spi].spilled_ptr.ref_obj_id) {
> verbose(env, "arg %d is an unacquired reference\n", regno);
> return -EINVAL;
> }
> --
> 2.39.1
>
