On Wed, Jan 18, 2023 at 6:15 PM Kumar Kartikeya Dwivedi
<memxor@xxxxxxxxx> wrote:
>
> Currently, process_dynptr_func first calls dynptr_get_spi and then
> is_dynptr_reg_valid_init and is_dynptr_reg_valid_uninit have to call it
> again to obtain the spi value. Instead of doing this twice, reuse the
> already obtained value (which is by default 0, and is only set for
> PTR_TO_STACK, and only used in that case in aforementioned functions).
> The input value for these two functions will either be -ERANGE or >= 1,
> and can either be permitted or rejected based on the respective check.
>
> Suggested-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
Acked-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> ---
> kernel/bpf/verifier.c | 24 +++++++++++-------------
> 1 file changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 18b54b219fac..7b8de84568a3 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -941,14 +941,12 @@ static int destroy_if_dynptr_stack_slot(struct bpf_verifier_env *env,
> return 0;
> }
>
> -static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> +static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
> + int spi)
> {
> - 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.
> @@ -966,16 +964,16 @@ static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_
> return true;
> }
>
> -static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> +static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
> + int spi)
> {
> struct bpf_func_state *state = func(env, reg);
> - int spi, i;
> + int i;
>
> /* This already represents first slot of initialized bpf_dynptr */
> if (reg->type == CONST_PTR_TO_DYNPTR)
> return true;
>
> - spi = dynptr_get_spi(env, reg);
> if (spi < 0)
> return false;
> if (!state->stack[spi].spilled_ptr.dynptr.first_slot)
> @@ -6132,7 +6130,7 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno,
> enum bpf_arg_type arg_type, struct bpf_call_arg_meta *meta)
> {
> struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno];
> - int err;
> + int err, spi = 0;
>
> /* MEM_UNINIT and MEM_RDONLY are exclusive, when applied to an
> * ARG_PTR_TO_DYNPTR (or ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_*):
> @@ -6146,9 +6144,9 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno,
> * and its alignment for PTR_TO_STACK.
> */
> if (reg->type == PTR_TO_STACK) {
> - err = dynptr_get_spi(env, reg);
> - if (err < 0 && err != -ERANGE)
> - return err;
> + spi = dynptr_get_spi(env, reg);
> + if (spi < 0 && spi != -ERANGE)
> + return spi;
> }
>
> /* MEM_UNINIT - Points to memory that is an appropriate candidate for
> @@ -6167,7 +6165,7 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno,
> * to.
> */
> if (arg_type & MEM_UNINIT) {
> - if (!is_dynptr_reg_valid_uninit(env, reg)) {
> + if (!is_dynptr_reg_valid_uninit(env, reg, spi)) {
> verbose(env, "Dynptr has to be an uninitialized dynptr\n");
> return -EINVAL;
> }
> @@ -6188,7 +6186,7 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno,
> return -EINVAL;
> }
>
> - if (!is_dynptr_reg_valid_init(env, reg)) {
> + if (!is_dynptr_reg_valid_init(env, reg, spi)) {
> verbose(env,
> "Expected an initialized dynptr as arg #%d\n",
> regno);
> --
> 2.39.1
>
