| Message ID | 20230119021442.1465269-7-memxor@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Dynptr fixes | expand |
On Wed, Jan 18, 2023 at 6:15 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> 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@gmail.com> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> Acked-by: Joanne Koong <joannelkoong@gmail.com> > --- > 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 >
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);
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@gmail.com> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> --- kernel/bpf/verifier.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-)