On Thu, Jun 7, 2018 at 8:40 AM, Daniel Borkmann <dan...@iogearbox.net> wrote:
> As commit 28e33f9d78ee ("bpf: disallow arithmetic operations on
> context pointer") already describes, f1174f77b50c ("bpf/verifier:
> rework value tracking") removed the specific white-listed cases
> we had previously where we would allow for pointer arithmetic in
> order to further generalize it, and allow e.g. context access via
> modified registers. While the dereferencing of modified context
> pointers had been forbidden through 28e33f9d78ee, syzkaller did
> recently manage to trigger several KASAN splats for slab out of
> bounds access and use after frees by simply passing a modified
> context pointer to a helper function which would then do the bad
> access since verifier allowed it in adjust_ptr_min_max_vals().
>
> Rejecting arithmetic on ctx pointer in adjust_ptr_min_max_vals()
> generally could break existing programs as there's a valid use
> case in tracing in combination with passing the ctx to helpers as
> bpf_probe_read(), where the register then becomes unknown at
> verification time due to adding a non-constant offset to it. An
> access sequence may look like the following:
>
> offset = args->filename; /* field __data_loc filename */
> bpf_probe_read(&dst, len, (char *)args + offset); // args is ctx
>
> There are two options: i) we could special case the ctx and as
> soon as we add a constant or bounded offset to it (hence ctx type
> wouldn't change) we could turn the ctx into an unknown scalar, or
> ii) we generalize the sanity test for ctx member access into a
> small helper and assert it on the ctx register that was passed
> as a function argument. Fwiw, latter is more obvious and less
> complex at the same time, and one case that may potentially be
> legitimate in future for ctx member access at least would be for
> ctx to carry a const offset. Therefore, fix follows approach
> from ii) and adds test cases to BPF kselftests.
>
> Fixes: f1174f77b50c ("bpf/verifier: rework value tracking")
> Reported-by: syzbot+3d0b2441dbb717516...@syzkaller.appspotmail.com
> Reported-by: syzbot+c8504affd4fdd0c1b...@syzkaller.appspotmail.com
> Reported-by: syzbot+e5190cb881d8660fb...@syzkaller.appspotmail.com
> Reported-by: syzbot+efae31b384d5badbd...@syzkaller.appspotmail.com
> Signed-off-by: Daniel Borkmann <dan...@iogearbox.net>
> Acked-by: Alexei Starovoitov <a...@kernel.org>
Tested with bcc for the above ctx + unknown_offset usage for bpf_probe_read.
No breakage.
Acked-by: Yonghong Song <y...@fb.com>
> ---
> kernel/bpf/verifier.c | 48 +++++++++++++++---------
> tools/testing/selftests/bpf/test_verifier.c | 58
> ++++++++++++++++++++++++++++-
> 2 files changed, 88 insertions(+), 18 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index d6403b5..cced0c1 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -1617,6 +1617,30 @@ static int get_callee_stack_depth(struct
> bpf_verifier_env *env,
> }
> #endif
>
> +static int check_ctx_reg(struct bpf_verifier_env *env,
> + const struct bpf_reg_state *reg, int regno)
> +{
> + /* Access to ctx or passing it to a helper is only allowed in
> + * its original, unmodified form.
> + */
> +
> + if (reg->off) {
> + verbose(env, "dereference of modified ctx ptr R%d off=%d
> disallowed\n",
> + regno, reg->off);
> + return -EACCES;
> + }
> +
> + if (!tnum_is_const(reg->var_off) || reg->var_off.value) {
> + char tn_buf[48];
> +
> + tnum_strn(tn_buf, sizeof(tn_buf), reg->var_off);
> + verbose(env, "variable ctx access var_off=%s disallowed\n",
> tn_buf);
> + return -EACCES;
> + }
> +
> + return 0;
> +}
> +
> /* truncate register to smaller size (in bytes)
> * must be called with size < BPF_REG_SIZE
> */
> @@ -1686,24 +1710,11 @@ static int check_mem_access(struct bpf_verifier_env
> *env, int insn_idx, u32 regn
> verbose(env, "R%d leaks addr into ctx\n",
> value_regno);
> return -EACCES;
> }
> - /* ctx accesses must be at a fixed offset, so that we can
> - * determine what type of data were returned.
> - */
> - if (reg->off) {
> - verbose(env,
> - "dereference of modified ctx ptr R%d
> off=%d+%d, ctx+const is allowed, ctx+const+const is not\n",
> - regno, reg->off, off - reg->off);
> - return -EACCES;
> - }
> - if (!tnum_is_const(reg->var_off) || reg->var_off.value) {
> - char tn_buf[48];
>
> - tnum_strn(tn_buf, sizeof(tn_buf), reg->var_off);
> - verbose(env,
> - "variable ctx access var_off=%s off=%d
> size=%d",
> - tn_buf, off, size);
> - return -EACCES;
> - }
> + err = check_ctx_reg(env, reg, regno);
> + if (err < 0)
> + return err;
> +
> err = check_ctx_access(env, insn_idx, off, size, t,
> ®_type);
> if (!err && t == BPF_READ && value_regno >= 0) {
> /* ctx access returns either a scalar, or a
> @@ -1984,6 +1995,9 @@ static int check_func_arg(struct bpf_verifier_env *env,
> u32 regno,
> expected_type = PTR_TO_CTX;
> if (type != expected_type)
> goto err_type;
> + err = check_ctx_reg(env, reg, regno);
> + if (err < 0)
> + return err;
> } else if (arg_type_is_mem_ptr(arg_type)) {
> expected_type = PTR_TO_STACK;
> /* One exception here. In case function allows for NULL to be
> diff --git a/tools/testing/selftests/bpf/test_verifier.c
> b/tools/testing/selftests/bpf/test_verifier.c
> index 7cb1d74..2ecd27b 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -8647,7 +8647,7 @@ static struct bpf_test tests[] = {
> offsetof(struct __sk_buff, mark)),
> BPF_EXIT_INSN(),
> },
> - .errstr = "dereference of modified ctx ptr R1 off=68+8,
> ctx+const is allowed, ctx+const+const is not",
> + .errstr = "dereference of modified ctx ptr",
> .result = REJECT,
> .prog_type = BPF_PROG_TYPE_SCHED_CLS,
> },
> @@ -12258,6 +12258,62 @@ static struct bpf_test tests[] = {
> .result = ACCEPT,
> .retval = 5,
> },
> + {
> + "pass unmodified ctx pointer to helper",
> + .insns = {
> + BPF_MOV64_IMM(BPF_REG_2, 0),
> + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
> + BPF_FUNC_csum_update),
> + BPF_MOV64_IMM(BPF_REG_0, 0),
> + BPF_EXIT_INSN(),
> + },
> + .prog_type = BPF_PROG_TYPE_SCHED_CLS,
> + .result = ACCEPT,
> + },
> + {
> + "pass modified ctx pointer to helper, 1",
> + .insns = {
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -612),
> + BPF_MOV64_IMM(BPF_REG_2, 0),
> + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
> + BPF_FUNC_csum_update),
> + BPF_MOV64_IMM(BPF_REG_0, 0),
> + BPF_EXIT_INSN(),
> + },
> + .prog_type = BPF_PROG_TYPE_SCHED_CLS,
> + .result = REJECT,
> + .errstr = "dereference of modified ctx ptr",
> + },
> + {
> + "pass modified ctx pointer to helper, 2",
> + .insns = {
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -612),
> + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
> + BPF_FUNC_get_socket_cookie),
> + BPF_MOV64_IMM(BPF_REG_0, 0),
> + BPF_EXIT_INSN(),
> + },
> + .result_unpriv = REJECT,
> + .result = REJECT,
> + .errstr_unpriv = "dereference of modified ctx ptr",
> + .errstr = "dereference of modified ctx ptr",
> + },
> + {
> + "pass modified ctx pointer to helper, 3",
> + .insns = {
> + BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_1, 0),
> + BPF_ALU64_IMM(BPF_AND, BPF_REG_3, 4),
> + BPF_ALU64_REG(BPF_ADD, BPF_REG_1, BPF_REG_3),
> + BPF_MOV64_IMM(BPF_REG_2, 0),
> + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
> + BPF_FUNC_csum_update),
> + BPF_MOV64_IMM(BPF_REG_0, 0),
> + BPF_EXIT_INSN(),
> + },
> + .prog_type = BPF_PROG_TYPE_SCHED_CLS,
> + .result = REJECT,
> + .errstr = "variable ctx access var_off=(0x0; 0x4)",
> + },
> };
>
> static int probe_filter_length(const struct bpf_insn *fp)
> --
> 2.9.5
>
