| Message ID | 20230101083403.332783-3-memxor@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Dynptr fixes | expand |
On Sun, Jan 1, 2023 at 12:34 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > Currently, the dynptr function is not checking the variable offset part > of PTR_TO_STACK that it needs to check. The fixed offset is considered > when computing the stack pointer index, but if the variable offset was > not a constant (such that it could not be accumulated in reg->off), we > will end up a discrepency where runtime pointer does not point to the > actual stack slot we mark as STACK_DYNPTR. > > It is impossible to precisely track dynptr state when variable offset is > not constant, hence, just like bpf_timer, kptr, bpf_spin_lock, etc. > simply reject the case where reg->var_off is not constant. Then, > consider both reg->off and reg->var_off.value when computing the stack > pointer index. > > A new helper dynptr_get_spi is introduced to hide over these details > since the dynptr needs to be located in multiple places outside the > process_dynptr_func checks, hence once we know it's a PTR_TO_STACK, we > need to enforce these checks in all places. > > Note that it is disallowed for unprivileged users to have a non-constant > var_off, so this problem should only be possible to trigger from > programs having CAP_PERFMON. However, its effects can vary. > > Without the fix, it is possible to replace the contents of the dynptr > arbitrarily by making verifier mark different stack slots than actual > location and then doing writes to the actual stack address of dynptr at > runtime. > > Fixes: 97e03f521050 ("bpf: Add verifier support for dynptrs") > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > --- > kernel/bpf/verifier.c | 83 ++++++++++++++----- > .../bpf/prog_tests/kfunc_dynptr_param.c | 2 +- > .../testing/selftests/bpf/progs/dynptr_fail.c | 6 +- > 3 files changed, 66 insertions(+), 25 deletions(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index f7248235e119..ca970f80e395 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -638,11 +638,34 @@ static void print_liveness(struct bpf_verifier_env *env, > verbose(env, "D"); > } > > -static int get_spi(s32 off) > +static int __get_spi(s32 off) > { > return (-off - 1) / BPF_REG_SIZE; > } > > +static int dynptr_get_spi(struct bpf_verifier_env *env, struct bpf_reg_state *reg) > +{ > + int off, spi; > + > + if (!tnum_is_const(reg->var_off)) { > + verbose(env, "dynptr has to be at the constant offset\n"); > + return -EINVAL; > + } > + > + off = reg->off + reg->var_off.value; > + if (off % BPF_REG_SIZE) { > + verbose(env, "cannot pass in dynptr at an offset=%d\n", reg->off); s/reg->off/off/ ? > + return -EINVAL; > + } > + > + spi = __get_spi(off); > + if (spi < 1) { > + verbose(env, "cannot pass in dynptr at an offset=%d\n", (int)(off + reg->var_off.value)); s/(int)(off + reg->var_off.value)/off/? > + return -EINVAL; > + } > + return spi; > +} > + [...] > @@ -2422,7 +2456,9 @@ static int mark_dynptr_read(struct bpf_verifier_env *env, struct bpf_reg_state * > */ > if (reg->type == CONST_PTR_TO_DYNPTR) > return 0; > - spi = get_spi(reg->off); > + spi = dynptr_get_spi(env, reg); > + if (WARN_ON_ONCE(spi < 0)) > + return spi; > /* Caller ensures dynptr is valid and initialized, which means spi is in > * bounds and spi is the first dynptr slot. Simply mark stack slot as > * read. > @@ -5946,6 +5982,11 @@ static int process_kptr_func(struct bpf_verifier_env *env, int regno, > return 0; > } > > +static bool arg_type_is_release(enum bpf_arg_type type) > +{ > + return type & OBJ_RELEASE; > +} > + no need to move it? > /* There are two register types representing a bpf_dynptr, one is PTR_TO_STACK > * which points to a stack slot, and the other is CONST_PTR_TO_DYNPTR. > * > @@ -5986,12 +6027,14 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno, > } > /* CONST_PTR_TO_DYNPTR already has fixed and var_off as 0 due to > * check_func_arg_reg_off's logic. We only need to check offset > - * alignment for PTR_TO_STACK. > + * and its alignment for PTR_TO_STACK. > */ > - if (reg->type == PTR_TO_STACK && (reg->off % BPF_REG_SIZE)) { > - verbose(env, "cannot pass in dynptr at an offset=%d\n", reg->off); > - return -EINVAL; > + if (reg->type == PTR_TO_STACK) { > + err = dynptr_get_spi(env, reg); > + if (err < 0) > + return err; > } > + > /* MEM_UNINIT - Points to memory that is an appropriate candidate for > * constructing a mutable bpf_dynptr object. > * > @@ -6070,11 +6113,6 @@ static bool arg_type_is_mem_size(enum bpf_arg_type type) > type == ARG_CONST_SIZE_OR_ZERO; > } > > -static bool arg_type_is_release(enum bpf_arg_type type) > -{ > - return type & OBJ_RELEASE; > -} > - > static bool arg_type_is_dynptr(enum bpf_arg_type type) > { > return base_type(type) == ARG_PTR_TO_DYNPTR; > @@ -6404,8 +6442,9 @@ static u32 dynptr_ref_obj_id(struct bpf_verifier_env *env, struct bpf_reg_state why not make dynptr_ref_obj_id return int and <0 on error? There seems to be just one place where we call dynptr_ref_obj_id and we can check and report error there > > if (reg->type == CONST_PTR_TO_DYNPTR) > return reg->ref_obj_id; > - > - spi = get_spi(reg->off); > + spi = dynptr_get_spi(env, reg); > + if (WARN_ON_ONCE(spi < 0)) > + return U32_MAX; > return state->stack[spi].spilled_ptr.ref_obj_id; > } > [...]
On Sun, Jan 1, 2023 at 12:34 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > Currently, the dynptr function is not checking the variable offset part > of PTR_TO_STACK that it needs to check. The fixed offset is considered > when computing the stack pointer index, but if the variable offset was > not a constant (such that it could not be accumulated in reg->off), we > will end up a discrepency where runtime pointer does not point to the > actual stack slot we mark as STACK_DYNPTR. > > It is impossible to precisely track dynptr state when variable offset is > not constant, hence, just like bpf_timer, kptr, bpf_spin_lock, etc. > simply reject the case where reg->var_off is not constant. Then, > consider both reg->off and reg->var_off.value when computing the stack > pointer index. > > A new helper dynptr_get_spi is introduced to hide over these details > since the dynptr needs to be located in multiple places outside the > process_dynptr_func checks, hence once we know it's a PTR_TO_STACK, we > need to enforce these checks in all places. > > Note that it is disallowed for unprivileged users to have a non-constant > var_off, so this problem should only be possible to trigger from > programs having CAP_PERFMON. However, its effects can vary. > > Without the fix, it is possible to replace the contents of the dynptr > arbitrarily by making verifier mark different stack slots than actual > location and then doing writes to the actual stack address of dynptr at > runtime. > > Fixes: 97e03f521050 ("bpf: Add verifier support for dynptrs") > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > --- > kernel/bpf/verifier.c | 83 ++++++++++++++----- > .../bpf/prog_tests/kfunc_dynptr_param.c | 2 +- > .../testing/selftests/bpf/progs/dynptr_fail.c | 6 +- > 3 files changed, 66 insertions(+), 25 deletions(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index f7248235e119..ca970f80e395 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -638,11 +638,34 @@ static void print_liveness(struct bpf_verifier_env *env, > verbose(env, "D"); > } > > -static int get_spi(s32 off) > +static int __get_spi(s32 off) > { > return (-off - 1) / BPF_REG_SIZE; > } > > +static int dynptr_get_spi(struct bpf_verifier_env *env, struct bpf_reg_state *reg) > +{ > + int off, spi; > + > + if (!tnum_is_const(reg->var_off)) { > + verbose(env, "dynptr has to be at the constant offset\n"); > + return -EINVAL; > + } > + > + off = reg->off + reg->var_off.value; > + if (off % BPF_REG_SIZE) { > + verbose(env, "cannot pass in dynptr at an offset=%d\n", reg->off); I think you meant off instead of reg->off? > + return -EINVAL; > + } > + > + spi = __get_spi(off); > + if (spi < 1) { > + verbose(env, "cannot pass in dynptr at an offset=%d\n", (int)(off + reg->var_off.value)); I think you meant off instead of off + reg->var_off.value > + return -EINVAL; > + } I think this if (spi < 1) check should have the same logic is_spi_bounds_valid() does (eg checking against total allocated slots as well). I think we can combine is_spi_bounds_valid() with this function and then every place we call is_spi_bounds_valid() > + 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; > @@ -754,7 +777,9 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_ > enum bpf_dynptr_type type; > int spi, i, id; > > - spi = get_spi(reg->off); > + spi = dynptr_get_spi(env, reg); > + if (spi < 0) > + return spi; > > if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS)) > return -EINVAL; > @@ -792,7 +817,9 @@ static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_re > struct bpf_func_state *state = func(env, reg); > int spi, i; > > - spi = get_spi(reg->off); > + spi = dynptr_get_spi(env, reg); > + if (spi < 0) > + return spi; > > if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS)) > return -EINVAL; > @@ -839,7 +866,11 @@ static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_ > if (reg->type == CONST_PTR_TO_DYNPTR) > return false; > > - spi = get_spi(reg->off); > + spi = dynptr_get_spi(env, reg); > + if (spi < 0) > + return spi; > + > + /* 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; > > @@ -855,14 +886,15 @@ static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_ > static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg) > { > struct bpf_func_state *state = func(env, reg); > - int spi; > - int i; > + int spi, i; > > /* This already represents first slot of initialized bpf_dynptr */ > if (reg->type == CONST_PTR_TO_DYNPTR) > return true; > > - spi = get_spi(reg->off); > + 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) > return false; > @@ -891,7 +923,9 @@ static bool is_dynptr_type_expected(struct bpf_verifier_env *env, struct bpf_reg > if (reg->type == CONST_PTR_TO_DYNPTR) { > return reg->dynptr.type == dynptr_type; > } else { > - spi = get_spi(reg->off); > + spi = dynptr_get_spi(env, reg); > + if (WARN_ON_ONCE(spi < 0)) > + return false; > return state->stack[spi].spilled_ptr.dynptr.type == dynptr_type; > } > } > @@ -2422,7 +2456,9 @@ static int mark_dynptr_read(struct bpf_verifier_env *env, struct bpf_reg_state * > */ > if (reg->type == CONST_PTR_TO_DYNPTR) > return 0; > - spi = get_spi(reg->off); > + spi = dynptr_get_spi(env, reg); > + if (WARN_ON_ONCE(spi < 0)) > + return spi; > /* Caller ensures dynptr is valid and initialized, which means spi is in > * bounds and spi is the first dynptr slot. Simply mark stack slot as > * read. > @@ -5946,6 +5982,11 @@ static int process_kptr_func(struct bpf_verifier_env *env, int regno, > return 0; > } > > +static bool arg_type_is_release(enum bpf_arg_type type) > +{ > + return type & OBJ_RELEASE; > +} nit: I dont think you need this arg_type_is_release() change > + > /* There are two register types representing a bpf_dynptr, one is PTR_TO_STACK > * which points to a stack slot, and the other is CONST_PTR_TO_DYNPTR. > * > @@ -5986,12 +6027,14 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno, > } > /* CONST_PTR_TO_DYNPTR already has fixed and var_off as 0 due to > * check_func_arg_reg_off's logic. We only need to check offset > - * alignment for PTR_TO_STACK. > + * and its alignment for PTR_TO_STACK. > */ > - if (reg->type == PTR_TO_STACK && (reg->off % BPF_REG_SIZE)) { > - verbose(env, "cannot pass in dynptr at an offset=%d\n", reg->off); > - return -EINVAL; > + if (reg->type == PTR_TO_STACK) { > + err = dynptr_get_spi(env, reg); > + if (err < 0) > + return err; > } nit: if we do something like If (reg->type == PTR_TO_STACK) { spi = dynptr_get_spi(env, reg); if (spi < 0) return spi; } else { spi = __get_spi(reg->off); } then we can just pass in spi to is_dynptr_reg_valid_uninit() and is_dynptr_reg_valid_init() instead of having to recompute/check them again > + > /* MEM_UNINIT - Points to memory that is an appropriate candidate for > * constructing a mutable bpf_dynptr object. > * > @@ -6070,11 +6113,6 @@ static bool arg_type_is_mem_size(enum bpf_arg_type type) > type == ARG_CONST_SIZE_OR_ZERO; > } > > -static bool arg_type_is_release(enum bpf_arg_type type) > -{ > - return type & OBJ_RELEASE; > -} > - > static bool arg_type_is_dynptr(enum bpf_arg_type type) > { > return base_type(type) == ARG_PTR_TO_DYNPTR; > @@ -6404,8 +6442,9 @@ static u32 dynptr_ref_obj_id(struct bpf_verifier_env *env, struct bpf_reg_state > > if (reg->type == CONST_PTR_TO_DYNPTR) > return reg->ref_obj_id; > - > - spi = get_spi(reg->off); > + spi = dynptr_get_spi(env, reg); > + if (WARN_ON_ONCE(spi < 0)) > + return U32_MAX; > return state->stack[spi].spilled_ptr.ref_obj_id; > } > > @@ -6479,7 +6518,9 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, > * PTR_TO_STACK. > */ > if (reg->type == PTR_TO_STACK) { > - spi = get_spi(reg->off); > + 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) { > verbose(env, "arg %d is an unacquired reference\n", regno); > diff --git a/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c b/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c > index a9229260a6ce..72800b1e8395 100644 > --- a/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c > +++ b/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c > @@ -18,7 +18,7 @@ static struct { > const char *expected_verifier_err_msg; > int expected_runtime_err; > } kfunc_dynptr_tests[] = { > - {"not_valid_dynptr", "Expected an initialized dynptr as arg #1", 0}, > + {"not_valid_dynptr", "cannot pass in dynptr at an offset=-8", 0}, > {"not_ptr_to_stack", "arg#0 expected pointer to stack or dynptr_ptr", 0}, > {"dynptr_data_null", NULL, -EBADMSG}, > }; > diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c > index 78debc1b3820..32df3647b794 100644 > --- a/tools/testing/selftests/bpf/progs/dynptr_fail.c > +++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c > @@ -382,7 +382,7 @@ int invalid_helper1(void *ctx) > > /* A dynptr can't be passed into a helper function at a non-zero offset */ > SEC("?raw_tp") > -__failure __msg("Expected an initialized dynptr as arg #3") > +__failure __msg("cannot pass in dynptr at an offset=-8") > int invalid_helper2(void *ctx) > { > struct bpf_dynptr ptr; > @@ -444,7 +444,7 @@ int invalid_write2(void *ctx) > * non-const offset > */ > SEC("?raw_tp") > -__failure __msg("Expected an initialized dynptr as arg #1") > +__failure __msg("arg 1 is an unacquired reference") > int invalid_write3(void *ctx) > { > struct bpf_dynptr ptr; > @@ -584,7 +584,7 @@ int invalid_read4(void *ctx) > > /* Initializing a dynptr on an offset should fail */ > SEC("?raw_tp") > -__failure __msg("invalid write to stack") > +__failure __msg("cannot pass in dynptr at an offset=0") > int invalid_offset(void *ctx) > { > struct bpf_dynptr ptr; > -- > 2.39.0 >
On Thu, Jan 5, 2023 at 4:57 PM Joanne Koong <joannelkoong@gmail.com> wrote: > > On Sun, Jan 1, 2023 at 12:34 AM Kumar Kartikeya Dwivedi > <memxor@gmail.com> wrote: > > > > Currently, the dynptr function is not checking the variable offset part > > of PTR_TO_STACK that it needs to check. The fixed offset is considered > > when computing the stack pointer index, but if the variable offset was > > not a constant (such that it could not be accumulated in reg->off), we > > will end up a discrepency where runtime pointer does not point to the > > actual stack slot we mark as STACK_DYNPTR. > > > > It is impossible to precisely track dynptr state when variable offset is > > not constant, hence, just like bpf_timer, kptr, bpf_spin_lock, etc. > > simply reject the case where reg->var_off is not constant. Then, > > consider both reg->off and reg->var_off.value when computing the stack > > pointer index. > > > > A new helper dynptr_get_spi is introduced to hide over these details > > since the dynptr needs to be located in multiple places outside the > > process_dynptr_func checks, hence once we know it's a PTR_TO_STACK, we > > need to enforce these checks in all places. > > > > Note that it is disallowed for unprivileged users to have a non-constant > > var_off, so this problem should only be possible to trigger from > > programs having CAP_PERFMON. However, its effects can vary. > > > > Without the fix, it is possible to replace the contents of the dynptr > > arbitrarily by making verifier mark different stack slots than actual > > location and then doing writes to the actual stack address of dynptr at > > runtime. > > > > Fixes: 97e03f521050 ("bpf: Add verifier support for dynptrs") > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > --- > > kernel/bpf/verifier.c | 83 ++++++++++++++----- > > .../bpf/prog_tests/kfunc_dynptr_param.c | 2 +- > > .../testing/selftests/bpf/progs/dynptr_fail.c | 6 +- > > 3 files changed, 66 insertions(+), 25 deletions(-) > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index f7248235e119..ca970f80e395 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -638,11 +638,34 @@ static void print_liveness(struct bpf_verifier_env *env, > > verbose(env, "D"); > > } > > > > -static int get_spi(s32 off) > > +static int __get_spi(s32 off) > > { > > return (-off - 1) / BPF_REG_SIZE; > > } > > > > +static int dynptr_get_spi(struct bpf_verifier_env *env, struct bpf_reg_state *reg) > > +{ > > + int off, spi; > > + > > + if (!tnum_is_const(reg->var_off)) { > > + verbose(env, "dynptr has to be at the constant offset\n"); > > + return -EINVAL; > > + } > > + > > + off = reg->off + reg->var_off.value; > > + if (off % BPF_REG_SIZE) { > > + verbose(env, "cannot pass in dynptr at an offset=%d\n", reg->off); > > I think you meant off instead of reg->off? > > > + return -EINVAL; > > + } > > + > > + spi = __get_spi(off); > > + if (spi < 1) { > > + verbose(env, "cannot pass in dynptr at an offset=%d\n", (int)(off + reg->var_off.value)); > > I think you meant off instead of off + reg->var_off.value > > > + return -EINVAL; > > + } > > I think this if (spi < 1) check should have the same logic > is_spi_bounds_valid() does (eg checking against total allocated slots > as well). I think we can combine is_spi_bounds_valid() with this > function and then every place we call is_spi_bounds_valid() > missing a word: and then remove* every place we call is_spi_bounds_valid() > > + 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; > > @@ -754,7 +777,9 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_ > > enum bpf_dynptr_type type; > > int spi, i, id; > > > > - spi = get_spi(reg->off); > > + spi = dynptr_get_spi(env, reg); > > + if (spi < 0) > > + return spi; > > > > if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS)) > > return -EINVAL; > > @@ -792,7 +817,9 @@ static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_re > > struct bpf_func_state *state = func(env, reg); > > int spi, i; > > > > - spi = get_spi(reg->off); > > + spi = dynptr_get_spi(env, reg); > > + if (spi < 0) > > + return spi; > > > > if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS)) > > return -EINVAL; > > @@ -839,7 +866,11 @@ static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_ > > if (reg->type == CONST_PTR_TO_DYNPTR) > > return false; > > > > - spi = get_spi(reg->off); > > + spi = dynptr_get_spi(env, reg); > > + if (spi < 0) > > + return spi; > > + > > + /* 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; > > > > @@ -855,14 +886,15 @@ static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_ > > static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg) > > { > > struct bpf_func_state *state = func(env, reg); > > - int spi; > > - int i; > > + int spi, i; > > > > /* This already represents first slot of initialized bpf_dynptr */ > > if (reg->type == CONST_PTR_TO_DYNPTR) > > return true; > > > > - spi = get_spi(reg->off); > > + 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) > > return false; > > @@ -891,7 +923,9 @@ static bool is_dynptr_type_expected(struct bpf_verifier_env *env, struct bpf_reg > > if (reg->type == CONST_PTR_TO_DYNPTR) { > > return reg->dynptr.type == dynptr_type; > > } else { > > - spi = get_spi(reg->off); > > + spi = dynptr_get_spi(env, reg); > > + if (WARN_ON_ONCE(spi < 0)) > > + return false; > > return state->stack[spi].spilled_ptr.dynptr.type == dynptr_type; > > } > > } > > @@ -2422,7 +2456,9 @@ static int mark_dynptr_read(struct bpf_verifier_env *env, struct bpf_reg_state * > > */ > > if (reg->type == CONST_PTR_TO_DYNPTR) > > return 0; > > - spi = get_spi(reg->off); > > + spi = dynptr_get_spi(env, reg); > > + if (WARN_ON_ONCE(spi < 0)) > > + return spi; > > /* Caller ensures dynptr is valid and initialized, which means spi is in > > * bounds and spi is the first dynptr slot. Simply mark stack slot as > > * read. > > @@ -5946,6 +5982,11 @@ static int process_kptr_func(struct bpf_verifier_env *env, int regno, > > return 0; > > } > > > > +static bool arg_type_is_release(enum bpf_arg_type type) > > +{ > > + return type & OBJ_RELEASE; > > +} > > nit: I dont think you need this arg_type_is_release() change > > > + > > /* There are two register types representing a bpf_dynptr, one is PTR_TO_STACK > > * which points to a stack slot, and the other is CONST_PTR_TO_DYNPTR. > > * > > @@ -5986,12 +6027,14 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno, > > } > > /* CONST_PTR_TO_DYNPTR already has fixed and var_off as 0 due to > > * check_func_arg_reg_off's logic. We only need to check offset > > - * alignment for PTR_TO_STACK. > > + * and its alignment for PTR_TO_STACK. > > */ > > - if (reg->type == PTR_TO_STACK && (reg->off % BPF_REG_SIZE)) { > > - verbose(env, "cannot pass in dynptr at an offset=%d\n", reg->off); > > - return -EINVAL; > > > + if (reg->type == PTR_TO_STACK) { > > + err = dynptr_get_spi(env, reg); > > + if (err < 0) > > + return err; > > } > > nit: if we do something like > > If (reg->type == PTR_TO_STACK) { > spi = dynptr_get_spi(env, reg); > if (spi < 0) > return spi; > } else { > spi = __get_spi(reg->off); > } > > then we can just pass in spi to is_dynptr_reg_valid_uninit() and > is_dynptr_reg_valid_init() instead of having to recompute/check them > again > > > + > > /* MEM_UNINIT - Points to memory that is an appropriate candidate for > > * constructing a mutable bpf_dynptr object. > > * > > @@ -6070,11 +6113,6 @@ static bool arg_type_is_mem_size(enum bpf_arg_type type) > > type == ARG_CONST_SIZE_OR_ZERO; > > } > > > > -static bool arg_type_is_release(enum bpf_arg_type type) > > -{ > > - return type & OBJ_RELEASE; > > -} > > - > > static bool arg_type_is_dynptr(enum bpf_arg_type type) > > { > > return base_type(type) == ARG_PTR_TO_DYNPTR; > > @@ -6404,8 +6442,9 @@ static u32 dynptr_ref_obj_id(struct bpf_verifier_env *env, struct bpf_reg_state > > > > if (reg->type == CONST_PTR_TO_DYNPTR) > > return reg->ref_obj_id; > > - > > - spi = get_spi(reg->off); > > + spi = dynptr_get_spi(env, reg); > > + if (WARN_ON_ONCE(spi < 0)) > > + return U32_MAX; > > return state->stack[spi].spilled_ptr.ref_obj_id; > > } > > > > @@ -6479,7 +6518,9 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, > > * PTR_TO_STACK. > > */ > > if (reg->type == PTR_TO_STACK) { > > - spi = get_spi(reg->off); > > + 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) { > > verbose(env, "arg %d is an unacquired reference\n", regno); > > diff --git a/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c b/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c > > index a9229260a6ce..72800b1e8395 100644 > > --- a/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c > > +++ b/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c > > @@ -18,7 +18,7 @@ static struct { > > const char *expected_verifier_err_msg; > > int expected_runtime_err; > > } kfunc_dynptr_tests[] = { > > - {"not_valid_dynptr", "Expected an initialized dynptr as arg #1", 0}, > > + {"not_valid_dynptr", "cannot pass in dynptr at an offset=-8", 0}, > > {"not_ptr_to_stack", "arg#0 expected pointer to stack or dynptr_ptr", 0}, > > {"dynptr_data_null", NULL, -EBADMSG}, > > }; > > diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c > > index 78debc1b3820..32df3647b794 100644 > > --- a/tools/testing/selftests/bpf/progs/dynptr_fail.c > > +++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c > > @@ -382,7 +382,7 @@ int invalid_helper1(void *ctx) > > > > /* A dynptr can't be passed into a helper function at a non-zero offset */ > > SEC("?raw_tp") > > -__failure __msg("Expected an initialized dynptr as arg #3") > > +__failure __msg("cannot pass in dynptr at an offset=-8") > > int invalid_helper2(void *ctx) > > { > > struct bpf_dynptr ptr; > > @@ -444,7 +444,7 @@ int invalid_write2(void *ctx) > > * non-const offset > > */ > > SEC("?raw_tp") > > -__failure __msg("Expected an initialized dynptr as arg #1") > > +__failure __msg("arg 1 is an unacquired reference") > > int invalid_write3(void *ctx) > > { > > struct bpf_dynptr ptr; > > @@ -584,7 +584,7 @@ int invalid_read4(void *ctx) > > > > /* Initializing a dynptr on an offset should fail */ > > SEC("?raw_tp") > > -__failure __msg("invalid write to stack") > > +__failure __msg("cannot pass in dynptr at an offset=0") > > int invalid_offset(void *ctx) > > { > > struct bpf_dynptr ptr; > > -- > > 2.39.0 > >
On Thu, Jan 05, 2023 at 04:02:11AM IST, Andrii Nakryiko wrote: > On Sun, Jan 1, 2023 at 12:34 AM Kumar Kartikeya Dwivedi > <memxor@gmail.com> wrote: > > > > Currently, the dynptr function is not checking the variable offset part > > of PTR_TO_STACK that it needs to check. The fixed offset is considered > > when computing the stack pointer index, but if the variable offset was > > not a constant (such that it could not be accumulated in reg->off), we > > will end up a discrepency where runtime pointer does not point to the > > actual stack slot we mark as STACK_DYNPTR. > > > > It is impossible to precisely track dynptr state when variable offset is > > not constant, hence, just like bpf_timer, kptr, bpf_spin_lock, etc. > > simply reject the case where reg->var_off is not constant. Then, > > consider both reg->off and reg->var_off.value when computing the stack > > pointer index. > > > > A new helper dynptr_get_spi is introduced to hide over these details > > since the dynptr needs to be located in multiple places outside the > > process_dynptr_func checks, hence once we know it's a PTR_TO_STACK, we > > need to enforce these checks in all places. > > > > Note that it is disallowed for unprivileged users to have a non-constant > > var_off, so this problem should only be possible to trigger from > > programs having CAP_PERFMON. However, its effects can vary. > > > > Without the fix, it is possible to replace the contents of the dynptr > > arbitrarily by making verifier mark different stack slots than actual > > location and then doing writes to the actual stack address of dynptr at > > runtime. > > > > Fixes: 97e03f521050 ("bpf: Add verifier support for dynptrs") > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > --- > > kernel/bpf/verifier.c | 83 ++++++++++++++----- > > .../bpf/prog_tests/kfunc_dynptr_param.c | 2 +- > > .../testing/selftests/bpf/progs/dynptr_fail.c | 6 +- > > 3 files changed, 66 insertions(+), 25 deletions(-) > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index f7248235e119..ca970f80e395 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -638,11 +638,34 @@ static void print_liveness(struct bpf_verifier_env *env, > > verbose(env, "D"); > > } > > > > -static int get_spi(s32 off) > > +static int __get_spi(s32 off) > > { > > return (-off - 1) / BPF_REG_SIZE; > > } > > > > +static int dynptr_get_spi(struct bpf_verifier_env *env, struct bpf_reg_state *reg) > > +{ > > + int off, spi; > > + > > + if (!tnum_is_const(reg->var_off)) { > > + verbose(env, "dynptr has to be at the constant offset\n"); > > + return -EINVAL; > > + } > > + > > + off = reg->off + reg->var_off.value; > > + if (off % BPF_REG_SIZE) { > > + verbose(env, "cannot pass in dynptr at an offset=%d\n", reg->off); > > s/reg->off/off/ ? > Yep, thanks for catching. > > + return -EINVAL; > > + } > > + > > + spi = __get_spi(off); > > + if (spi < 1) { > > + verbose(env, "cannot pass in dynptr at an offset=%d\n", (int)(off + reg->var_off.value)); > > s/(int)(off + reg->var_off.value)/off/? > Same, yes. > > + return -EINVAL; > > + } > > + return spi; > > +} > > + > > [...] > > > @@ -2422,7 +2456,9 @@ static int mark_dynptr_read(struct bpf_verifier_env *env, struct bpf_reg_state * > > */ > > if (reg->type == CONST_PTR_TO_DYNPTR) > > return 0; > > - spi = get_spi(reg->off); > > + spi = dynptr_get_spi(env, reg); > > + if (WARN_ON_ONCE(spi < 0)) > > + return spi; > > /* Caller ensures dynptr is valid and initialized, which means spi is in > > * bounds and spi is the first dynptr slot. Simply mark stack slot as > > * read. > > @@ -5946,6 +5982,11 @@ static int process_kptr_func(struct bpf_verifier_env *env, int regno, > > return 0; > > } > > > > +static bool arg_type_is_release(enum bpf_arg_type type) > > +{ > > + return type & OBJ_RELEASE; > > +} > > + > > no need to move it? > Yeah, will fix. > > /* There are two register types representing a bpf_dynptr, one is PTR_TO_STACK > > * which points to a stack slot, and the other is CONST_PTR_TO_DYNPTR. > > * > > @@ -5986,12 +6027,14 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno, > > } > > /* CONST_PTR_TO_DYNPTR already has fixed and var_off as 0 due to > > * check_func_arg_reg_off's logic. We only need to check offset > > - * alignment for PTR_TO_STACK. > > + * and its alignment for PTR_TO_STACK. > > */ > > - if (reg->type == PTR_TO_STACK && (reg->off % BPF_REG_SIZE)) { > > - verbose(env, "cannot pass in dynptr at an offset=%d\n", reg->off); > > - return -EINVAL; > > + if (reg->type == PTR_TO_STACK) { > > + err = dynptr_get_spi(env, reg); > > + if (err < 0) > > + return err; > > } > > + > > /* MEM_UNINIT - Points to memory that is an appropriate candidate for > > * constructing a mutable bpf_dynptr object. > > * > > @@ -6070,11 +6113,6 @@ static bool arg_type_is_mem_size(enum bpf_arg_type type) > > type == ARG_CONST_SIZE_OR_ZERO; > > } > > > > -static bool arg_type_is_release(enum bpf_arg_type type) > > -{ > > - return type & OBJ_RELEASE; > > -} > > - > > static bool arg_type_is_dynptr(enum bpf_arg_type type) > > { > > return base_type(type) == ARG_PTR_TO_DYNPTR; > > @@ -6404,8 +6442,9 @@ static u32 dynptr_ref_obj_id(struct bpf_verifier_env *env, struct bpf_reg_state > > why not make dynptr_ref_obj_id return int and <0 on error? There seems > to be just one place where we call dynptr_ref_obj_id and we can check > and report error there > Good suggestion, I'll make that change. > > > > if (reg->type == CONST_PTR_TO_DYNPTR) > > return reg->ref_obj_id; > > - > > - spi = get_spi(reg->off); > > + spi = dynptr_get_spi(env, reg); > > + if (WARN_ON_ONCE(spi < 0)) > > + return U32_MAX; > > return state->stack[spi].spilled_ptr.ref_obj_id; > > } > > > > [...]
On Fri, Jan 06, 2023 at 06:27:06AM IST, Joanne Koong wrote: > On Sun, Jan 1, 2023 at 12:34 AM Kumar Kartikeya Dwivedi > <memxor@gmail.com> wrote: > > > > Currently, the dynptr function is not checking the variable offset part > > of PTR_TO_STACK that it needs to check. The fixed offset is considered > > when computing the stack pointer index, but if the variable offset was > > not a constant (such that it could not be accumulated in reg->off), we > > will end up a discrepency where runtime pointer does not point to the > > actual stack slot we mark as STACK_DYNPTR. > > > > It is impossible to precisely track dynptr state when variable offset is > > not constant, hence, just like bpf_timer, kptr, bpf_spin_lock, etc. > > simply reject the case where reg->var_off is not constant. Then, > > consider both reg->off and reg->var_off.value when computing the stack > > pointer index. > > > > A new helper dynptr_get_spi is introduced to hide over these details > > since the dynptr needs to be located in multiple places outside the > > process_dynptr_func checks, hence once we know it's a PTR_TO_STACK, we > > need to enforce these checks in all places. > > > > Note that it is disallowed for unprivileged users to have a non-constant > > var_off, so this problem should only be possible to trigger from > > programs having CAP_PERFMON. However, its effects can vary. > > > > Without the fix, it is possible to replace the contents of the dynptr > > arbitrarily by making verifier mark different stack slots than actual > > location and then doing writes to the actual stack address of dynptr at > > runtime. > > > > Fixes: 97e03f521050 ("bpf: Add verifier support for dynptrs") > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > --- > > kernel/bpf/verifier.c | 83 ++++++++++++++----- > > .../bpf/prog_tests/kfunc_dynptr_param.c | 2 +- > > .../testing/selftests/bpf/progs/dynptr_fail.c | 6 +- > > 3 files changed, 66 insertions(+), 25 deletions(-) > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index f7248235e119..ca970f80e395 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -638,11 +638,34 @@ static void print_liveness(struct bpf_verifier_env *env, > > verbose(env, "D"); > > } > > > > -static int get_spi(s32 off) > > +static int __get_spi(s32 off) > > { > > return (-off - 1) / BPF_REG_SIZE; > > } > > > > +static int dynptr_get_spi(struct bpf_verifier_env *env, struct bpf_reg_state *reg) > > +{ > > + int off, spi; > > + > > + if (!tnum_is_const(reg->var_off)) { > > + verbose(env, "dynptr has to be at the constant offset\n"); > > + return -EINVAL; > > + } > > + > > + off = reg->off + reg->var_off.value; > > + if (off % BPF_REG_SIZE) { > > + verbose(env, "cannot pass in dynptr at an offset=%d\n", reg->off); > > I think you meant off instead of reg->off? > Ack. > > + return -EINVAL; > > + } > > + > > + spi = __get_spi(off); > > + if (spi < 1) { > > + verbose(env, "cannot pass in dynptr at an offset=%d\n", (int)(off + reg->var_off.value)); > > I think you meant off instead of off + reg->var_off.value > Ack. > > + return -EINVAL; > > + } > > I think this if (spi < 1) check should have the same logic > is_spi_bounds_valid() does (eg checking against total allocated slots > as well). I think we can combine is_spi_bounds_valid() with this > function and then every place we call is_spi_bounds_valid() > Ok, I'll combine both. > > + 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; > > @@ -754,7 +777,9 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_ > > enum bpf_dynptr_type type; > > int spi, i, id; > > > > - spi = get_spi(reg->off); > > + spi = dynptr_get_spi(env, reg); > > + if (spi < 0) > > + return spi; > > > > if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS)) > > return -EINVAL; > > @@ -792,7 +817,9 @@ static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_re > > struct bpf_func_state *state = func(env, reg); > > int spi, i; > > > > - spi = get_spi(reg->off); > > + spi = dynptr_get_spi(env, reg); > > + if (spi < 0) > > + return spi; > > > > if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS)) > > return -EINVAL; > > @@ -839,7 +866,11 @@ static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_ > > if (reg->type == CONST_PTR_TO_DYNPTR) > > return false; > > > > - spi = get_spi(reg->off); > > + spi = dynptr_get_spi(env, reg); > > + if (spi < 0) > > + return spi; > > + > > + /* 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; > > > > @@ -855,14 +886,15 @@ static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_ > > static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg) > > { > > struct bpf_func_state *state = func(env, reg); > > - int spi; > > - int i; > > + int spi, i; > > > > /* This already represents first slot of initialized bpf_dynptr */ > > if (reg->type == CONST_PTR_TO_DYNPTR) > > return true; > > > > - spi = get_spi(reg->off); > > + 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) > > return false; > > @@ -891,7 +923,9 @@ static bool is_dynptr_type_expected(struct bpf_verifier_env *env, struct bpf_reg > > if (reg->type == CONST_PTR_TO_DYNPTR) { > > return reg->dynptr.type == dynptr_type; > > } else { > > - spi = get_spi(reg->off); > > + spi = dynptr_get_spi(env, reg); > > + if (WARN_ON_ONCE(spi < 0)) > > + return false; > > return state->stack[spi].spilled_ptr.dynptr.type == dynptr_type; > > } > > } > > @@ -2422,7 +2456,9 @@ static int mark_dynptr_read(struct bpf_verifier_env *env, struct bpf_reg_state * > > */ > > if (reg->type == CONST_PTR_TO_DYNPTR) > > return 0; > > - spi = get_spi(reg->off); > > + spi = dynptr_get_spi(env, reg); > > + if (WARN_ON_ONCE(spi < 0)) > > + return spi; > > /* Caller ensures dynptr is valid and initialized, which means spi is in > > * bounds and spi is the first dynptr slot. Simply mark stack slot as > > * read. > > @@ -5946,6 +5982,11 @@ static int process_kptr_func(struct bpf_verifier_env *env, int regno, > > return 0; > > } > > > > +static bool arg_type_is_release(enum bpf_arg_type type) > > +{ > > + return type & OBJ_RELEASE; > > +} > > nit: I dont think you need this arg_type_is_release() change > Ack. > > + > > /* There are two register types representing a bpf_dynptr, one is PTR_TO_STACK > > * which points to a stack slot, and the other is CONST_PTR_TO_DYNPTR. > > * > > @@ -5986,12 +6027,14 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno, > > } > > /* CONST_PTR_TO_DYNPTR already has fixed and var_off as 0 due to > > * check_func_arg_reg_off's logic. We only need to check offset > > - * alignment for PTR_TO_STACK. > > + * and its alignment for PTR_TO_STACK. > > */ > > - if (reg->type == PTR_TO_STACK && (reg->off % BPF_REG_SIZE)) { > > - verbose(env, "cannot pass in dynptr at an offset=%d\n", reg->off); > > - return -EINVAL; > > > + if (reg->type == PTR_TO_STACK) { > > + err = dynptr_get_spi(env, reg); > > + if (err < 0) > > + return err; > > } > > nit: if we do something like > > If (reg->type == PTR_TO_STACK) { > spi = dynptr_get_spi(env, reg); > if (spi < 0) > return spi; > } else { > spi = __get_spi(reg->off); > } > > then we can just pass in spi to is_dynptr_reg_valid_uninit() and > is_dynptr_reg_valid_init() instead of having to recompute/check them > again > Seems a little misleading to set it to something in the else branch (where stack pointer index has no meaning), but I do see your point, I guess it can be ignored for the other case and set to 0 by default. > [...]
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index f7248235e119..ca970f80e395 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -638,11 +638,34 @@ static void print_liveness(struct bpf_verifier_env *env, verbose(env, "D"); } -static int get_spi(s32 off) +static int __get_spi(s32 off) { return (-off - 1) / BPF_REG_SIZE; } +static int dynptr_get_spi(struct bpf_verifier_env *env, struct bpf_reg_state *reg) +{ + int off, spi; + + if (!tnum_is_const(reg->var_off)) { + verbose(env, "dynptr has to be at the constant offset\n"); + return -EINVAL; + } + + off = reg->off + reg->var_off.value; + if (off % BPF_REG_SIZE) { + verbose(env, "cannot pass in dynptr at an offset=%d\n", reg->off); + return -EINVAL; + } + + spi = __get_spi(off); + if (spi < 1) { + verbose(env, "cannot pass in dynptr at an offset=%d\n", (int)(off + reg->var_off.value)); + 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; @@ -754,7 +777,9 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_ enum bpf_dynptr_type type; int spi, i, id; - spi = get_spi(reg->off); + spi = dynptr_get_spi(env, reg); + if (spi < 0) + return spi; if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS)) return -EINVAL; @@ -792,7 +817,9 @@ static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_re struct bpf_func_state *state = func(env, reg); int spi, i; - spi = get_spi(reg->off); + spi = dynptr_get_spi(env, reg); + if (spi < 0) + return spi; if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS)) return -EINVAL; @@ -839,7 +866,11 @@ static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_ if (reg->type == CONST_PTR_TO_DYNPTR) return false; - spi = get_spi(reg->off); + spi = dynptr_get_spi(env, reg); + if (spi < 0) + return spi; + + /* 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; @@ -855,14 +886,15 @@ static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_ static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg) { struct bpf_func_state *state = func(env, reg); - int spi; - int i; + int spi, i; /* This already represents first slot of initialized bpf_dynptr */ if (reg->type == CONST_PTR_TO_DYNPTR) return true; - spi = get_spi(reg->off); + 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) return false; @@ -891,7 +923,9 @@ static bool is_dynptr_type_expected(struct bpf_verifier_env *env, struct bpf_reg if (reg->type == CONST_PTR_TO_DYNPTR) { return reg->dynptr.type == dynptr_type; } else { - spi = get_spi(reg->off); + spi = dynptr_get_spi(env, reg); + if (WARN_ON_ONCE(spi < 0)) + return false; return state->stack[spi].spilled_ptr.dynptr.type == dynptr_type; } } @@ -2422,7 +2456,9 @@ static int mark_dynptr_read(struct bpf_verifier_env *env, struct bpf_reg_state * */ if (reg->type == CONST_PTR_TO_DYNPTR) return 0; - spi = get_spi(reg->off); + spi = dynptr_get_spi(env, reg); + if (WARN_ON_ONCE(spi < 0)) + return spi; /* Caller ensures dynptr is valid and initialized, which means spi is in * bounds and spi is the first dynptr slot. Simply mark stack slot as * read. @@ -5946,6 +5982,11 @@ static int process_kptr_func(struct bpf_verifier_env *env, int regno, return 0; } +static bool arg_type_is_release(enum bpf_arg_type type) +{ + return type & OBJ_RELEASE; +} + /* There are two register types representing a bpf_dynptr, one is PTR_TO_STACK * which points to a stack slot, and the other is CONST_PTR_TO_DYNPTR. * @@ -5986,12 +6027,14 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno, } /* CONST_PTR_TO_DYNPTR already has fixed and var_off as 0 due to * check_func_arg_reg_off's logic. We only need to check offset - * alignment for PTR_TO_STACK. + * and its alignment for PTR_TO_STACK. */ - if (reg->type == PTR_TO_STACK && (reg->off % BPF_REG_SIZE)) { - verbose(env, "cannot pass in dynptr at an offset=%d\n", reg->off); - return -EINVAL; + if (reg->type == PTR_TO_STACK) { + err = dynptr_get_spi(env, reg); + if (err < 0) + return err; } + /* MEM_UNINIT - Points to memory that is an appropriate candidate for * constructing a mutable bpf_dynptr object. * @@ -6070,11 +6113,6 @@ static bool arg_type_is_mem_size(enum bpf_arg_type type) type == ARG_CONST_SIZE_OR_ZERO; } -static bool arg_type_is_release(enum bpf_arg_type type) -{ - return type & OBJ_RELEASE; -} - static bool arg_type_is_dynptr(enum bpf_arg_type type) { return base_type(type) == ARG_PTR_TO_DYNPTR; @@ -6404,8 +6442,9 @@ static u32 dynptr_ref_obj_id(struct bpf_verifier_env *env, struct bpf_reg_state if (reg->type == CONST_PTR_TO_DYNPTR) return reg->ref_obj_id; - - spi = get_spi(reg->off); + spi = dynptr_get_spi(env, reg); + if (WARN_ON_ONCE(spi < 0)) + return U32_MAX; return state->stack[spi].spilled_ptr.ref_obj_id; } @@ -6479,7 +6518,9 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, * PTR_TO_STACK. */ if (reg->type == PTR_TO_STACK) { - spi = get_spi(reg->off); + 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) { verbose(env, "arg %d is an unacquired reference\n", regno); diff --git a/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c b/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c index a9229260a6ce..72800b1e8395 100644 --- a/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c +++ b/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c @@ -18,7 +18,7 @@ static struct { const char *expected_verifier_err_msg; int expected_runtime_err; } kfunc_dynptr_tests[] = { - {"not_valid_dynptr", "Expected an initialized dynptr as arg #1", 0}, + {"not_valid_dynptr", "cannot pass in dynptr at an offset=-8", 0}, {"not_ptr_to_stack", "arg#0 expected pointer to stack or dynptr_ptr", 0}, {"dynptr_data_null", NULL, -EBADMSG}, }; diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c index 78debc1b3820..32df3647b794 100644 --- a/tools/testing/selftests/bpf/progs/dynptr_fail.c +++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c @@ -382,7 +382,7 @@ int invalid_helper1(void *ctx) /* A dynptr can't be passed into a helper function at a non-zero offset */ SEC("?raw_tp") -__failure __msg("Expected an initialized dynptr as arg #3") +__failure __msg("cannot pass in dynptr at an offset=-8") int invalid_helper2(void *ctx) { struct bpf_dynptr ptr; @@ -444,7 +444,7 @@ int invalid_write2(void *ctx) * non-const offset */ SEC("?raw_tp") -__failure __msg("Expected an initialized dynptr as arg #1") +__failure __msg("arg 1 is an unacquired reference") int invalid_write3(void *ctx) { struct bpf_dynptr ptr; @@ -584,7 +584,7 @@ int invalid_read4(void *ctx) /* Initializing a dynptr on an offset should fail */ SEC("?raw_tp") -__failure __msg("invalid write to stack") +__failure __msg("cannot pass in dynptr at an offset=0") int invalid_offset(void *ctx) { struct bpf_dynptr ptr;
Currently, the dynptr function is not checking the variable offset part of PTR_TO_STACK that it needs to check. The fixed offset is considered when computing the stack pointer index, but if the variable offset was not a constant (such that it could not be accumulated in reg->off), we will end up a discrepency where runtime pointer does not point to the actual stack slot we mark as STACK_DYNPTR. It is impossible to precisely track dynptr state when variable offset is not constant, hence, just like bpf_timer, kptr, bpf_spin_lock, etc. simply reject the case where reg->var_off is not constant. Then, consider both reg->off and reg->var_off.value when computing the stack pointer index. A new helper dynptr_get_spi is introduced to hide over these details since the dynptr needs to be located in multiple places outside the process_dynptr_func checks, hence once we know it's a PTR_TO_STACK, we need to enforce these checks in all places. Note that it is disallowed for unprivileged users to have a non-constant var_off, so this problem should only be possible to trigger from programs having CAP_PERFMON. However, its effects can vary. Without the fix, it is possible to replace the contents of the dynptr arbitrarily by making verifier mark different stack slots than actual location and then doing writes to the actual stack address of dynptr at runtime. Fixes: 97e03f521050 ("bpf: Add verifier support for dynptrs") Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> --- kernel/bpf/verifier.c | 83 ++++++++++++++----- .../bpf/prog_tests/kfunc_dynptr_param.c | 2 +- .../testing/selftests/bpf/progs/dynptr_fail.c | 6 +- 3 files changed, 66 insertions(+), 25 deletions(-)