Re: [PATCH bpf] bpf: reject passing modified ctx to helper functions

[Y Song <ys114321@gmail.com>]

On Thu, Jun 7, 2018 at 8:40 AM, Daniel Borkmann <daniel@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+3d0b2441dbb71751615e@syzkaller.appspotmail.com
> Reported-by: syzbot+c8504affd4fdd0c1b626@syzkaller.appspotmail.com
> Reported-by: syzbot+e5190cb881d8660fb1a3@syzkaller.appspotmail.com
> Reported-by: syzbot+efae31b384d5badbd620@syzkaller.appspotmail.com
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Acked-by: Alexei Starovoitov <ast@kernel.org>

Tested with bcc for the above ctx + unknown_offset usage for bpf_probe_read.
No breakage.

Acked-by: Yonghong Song <yhs@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, &reg_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
>