Message ID | 20180607154003.5368-1-daniel@iogearbox.net |
---|---|
State | Accepted, archived |
Delegated to: | BPF Maintainers |
Headers | show |
Series | [bpf] bpf: reject passing modified ctx to helper functions | expand |
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, ®_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 >
On 07/06/18 16:40, Daniel Borkmann 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> Acked-by: Edward Cree <ecree@solarflare.com>
On Thu, Jun 07, 2018 at 05:40:03PM +0200, Daniel Borkmann 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> Applied, Thanks
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)