On Wed, Jan 18, 2023 at 6:14 PM Kumar Kartikeya Dwivedi
<memxor@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> kernel/bpf/verifier.c | 83 +++++++++++++++----
> .../bpf/prog_tests/kfunc_dynptr_param.c | 2 +-
> .../testing/selftests/bpf/progs/dynptr_fail.c | 4 +-
> 3 files changed, 68 insertions(+), 21 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 89de5bc46f27..eeb6f1b2bd60 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");
nit: "at a constant offset" instead of "at the constant offset"?
> + 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", off);
> + return -EINVAL;
> + }
> +
> + spi = __get_spi(off);
> + if (spi < 1) {
> + verbose(env, "cannot pass in dynptr at an offset=%d\n", off);
> + return -EINVAL;
> + }
I still 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 remove every place we call is_spi_bounds_valid().
WDYT?
> + 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;
> @@ -844,7 +871,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 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;
>
> @@ -860,14 +891,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;
> @@ -896,7 +928,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 (spi < 0)
> + return false;
> return state->stack[spi].spilled_ptr.dynptr.type == dynptr_type;
> }
> }
> @@ -2429,7 +2463,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 (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.
> @@ -5993,12 +6029,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.
> *
> @@ -6404,15 +6442,16 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env,
> }
> }
>
> -static u32 dynptr_ref_obj_id(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> +static int dynptr_ref_obj_id(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 reg->ref_obj_id;
> -
> - spi = get_spi(reg->off);
> + spi = dynptr_get_spi(env, reg);
> + if (spi < 0)
> + return spi;
> return state->stack[spi].spilled_ptr.ref_obj_id;
> }
>
> @@ -6486,7 +6525,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);
> @@ -7976,13 +8017,19 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
> for (i = 0; i < MAX_BPF_FUNC_REG_ARGS; i++) {
> if (arg_type_is_dynptr(fn->arg_type[i])) {
> struct bpf_reg_state *reg = ®s[BPF_REG_1 + i];
> + int ref_obj_id;
>
> if (meta.ref_obj_id) {
> verbose(env, "verifier internal error: meta.ref_obj_id already set\n");
> return -EFAULT;
> }
>
> - meta.ref_obj_id = dynptr_ref_obj_id(env, reg);
> + ref_obj_id = dynptr_ref_obj_id(env, reg);
> + if (err < 0) {
> + verbose(env, "verifier internal error: failed to obtain dynptr ref_obj_id\n");
> + return err;
> + }
> + meta.ref_obj_id = ref_obj_id;
> break;
> }
> }
> 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..02d57b95cf6e 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;
> @@ -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.1
>
