* [PATCH bpf-next v3 0/2] bpf: permits pointers on stack for helper calls
@ 2020-12-11 3:41 Yonghong Song
2020-12-11 3:41 ` [PATCH bpf-next v3 1/2] " Yonghong Song
2020-12-11 3:41 ` [PATCH bpf-next v3 2/2] selftests/bpf: add a test for ptr_to_map_value on stack for helper access Yonghong Song
0 siblings, 2 replies; 5+ messages in thread
From: Yonghong Song @ 2020-12-11 3:41 UTC (permalink / raw)
To: bpf; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team
This patch permits pointers on stack for helper calls if permission is
granted. Patch #1 described the detailed usecase and Patch #2
added a test.
Changelog:
v2 -> v3:
- do not permit spilled reg state NOT_INIT on stack. (Daniel)
v1 -> v2:
- fix a verifier test failure due to verifier change.
Yonghong Song (2):
bpf: permits pointers on stack for helper calls
selftests/bpf: add a test for ptr_to_map_value on stack for helper
access
kernel/bpf/verifier.c | 4 +++-
tools/testing/selftests/bpf/progs/bpf_iter_task.c | 3 ++-
tools/testing/selftests/bpf/verifier/unpriv.c | 5 +++--
3 files changed, 8 insertions(+), 4 deletions(-)
--
2.24.1
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH bpf-next v3 1/2] bpf: permits pointers on stack for helper calls 2020-12-11 3:41 [PATCH bpf-next v3 0/2] bpf: permits pointers on stack for helper calls Yonghong Song @ 2020-12-11 3:41 ` Yonghong Song 2020-12-14 21:26 ` Daniel Borkmann 2020-12-11 3:41 ` [PATCH bpf-next v3 2/2] selftests/bpf: add a test for ptr_to_map_value on stack for helper access Yonghong Song 1 sibling, 1 reply; 5+ messages in thread From: Yonghong Song @ 2020-12-11 3:41 UTC (permalink / raw) To: bpf; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, Song Liu Currently, when checking stack memory accessed by helper calls, for spills, only PTR_TO_BTF_ID and SCALAR_VALUE are allowed. Song discovered an issue where the below bpf program int dump_task(struct bpf_iter__task *ctx) { struct seq_file *seq = ctx->meta->seq; static char[] info = "abc"; BPF_SEQ_PRINTF(seq, "%s\n", info); return 0; } may cause a verifier failure. The verifier output looks like: ; struct seq_file *seq = ctx->meta->seq; 1: (79) r1 = *(u64 *)(r1 +0) ; BPF_SEQ_PRINTF(seq, "%s\n", info); 2: (18) r2 = 0xffff9054400f6000 4: (7b) *(u64 *)(r10 -8) = r2 5: (bf) r4 = r10 ; 6: (07) r4 += -8 ; BPF_SEQ_PRINTF(seq, "%s\n", info); 7: (18) r2 = 0xffff9054400fe000 9: (b4) w3 = 4 10: (b4) w5 = 8 11: (85) call bpf_seq_printf#126 R1_w=ptr_seq_file(id=0,off=0,imm=0) R2_w=map_value(id=0,off=0,ks=4,vs=4,imm=0) R3_w=inv4 R4_w=fp-8 R5_w=inv8 R10=fp0 fp-8_w=map_value last_idx 11 first_idx 0 regs=8 stack=0 before 10: (b4) w5 = 8 regs=8 stack=0 before 9: (b4) w3 = 4 invalid indirect read from stack off -8+0 size 8 Basically, the verifier complains the map_value pointer at "fp-8" location. To fix the issue, if env->allow_ptr_leaks is true, let us also permit pointers on the stack to be accessible by the helper. Suggested-by: Alexei Starovoitov <ast@kernel.org> Reported-by: Song Liu <songliubraving@fb.com> Signed-off-by: Yonghong Song <yhs@fb.com> --- kernel/bpf/verifier.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 93def76cf32b..eebb2d3e16bf 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -3769,7 +3769,9 @@ static int check_stack_boundary(struct bpf_verifier_env *env, int regno, goto mark; if (state->stack[spi].slot_type[0] == STACK_SPILL && - state->stack[spi].spilled_ptr.type == SCALAR_VALUE) { + (state->stack[spi].spilled_ptr.type == SCALAR_VALUE || + (state->stack[spi].spilled_ptr.type != NOT_INIT && + env->allow_ptr_leaks))) { __mark_reg_unknown(env, &state->stack[spi].spilled_ptr); for (j = 0; j < BPF_REG_SIZE; j++) state->stack[spi].slot_type[j] = STACK_MISC; -- 2.24.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next v3 1/2] bpf: permits pointers on stack for helper calls 2020-12-11 3:41 ` [PATCH bpf-next v3 1/2] " Yonghong Song @ 2020-12-14 21:26 ` Daniel Borkmann 2020-12-15 3:57 ` Yonghong Song 0 siblings, 1 reply; 5+ messages in thread From: Daniel Borkmann @ 2020-12-14 21:26 UTC (permalink / raw) To: Yonghong Song, bpf; +Cc: Alexei Starovoitov, kernel-team, Song Liu On 12/11/20 4:41 AM, Yonghong Song wrote: > Currently, when checking stack memory accessed by helper calls, > for spills, only PTR_TO_BTF_ID and SCALAR_VALUE are > allowed. > > Song discovered an issue where the below bpf program > int dump_task(struct bpf_iter__task *ctx) > { > struct seq_file *seq = ctx->meta->seq; > static char[] info = "abc"; > BPF_SEQ_PRINTF(seq, "%s\n", info); > return 0; > } > may cause a verifier failure. > > The verifier output looks like: > ; struct seq_file *seq = ctx->meta->seq; > 1: (79) r1 = *(u64 *)(r1 +0) > ; BPF_SEQ_PRINTF(seq, "%s\n", info); > 2: (18) r2 = 0xffff9054400f6000 > 4: (7b) *(u64 *)(r10 -8) = r2 > 5: (bf) r4 = r10 > ; > 6: (07) r4 += -8 > ; BPF_SEQ_PRINTF(seq, "%s\n", info); > 7: (18) r2 = 0xffff9054400fe000 > 9: (b4) w3 = 4 > 10: (b4) w5 = 8 > 11: (85) call bpf_seq_printf#126 > R1_w=ptr_seq_file(id=0,off=0,imm=0) R2_w=map_value(id=0,off=0,ks=4,vs=4,imm=0) > R3_w=inv4 R4_w=fp-8 R5_w=inv8 R10=fp0 fp-8_w=map_value > last_idx 11 first_idx 0 > regs=8 stack=0 before 10: (b4) w5 = 8 > regs=8 stack=0 before 9: (b4) w3 = 4 > invalid indirect read from stack off -8+0 size 8 > > Basically, the verifier complains the map_value pointer at "fp-8" location. > To fix the issue, if env->allow_ptr_leaks is true, let us also permit > pointers on the stack to be accessible by the helper. > > Suggested-by: Alexei Starovoitov <ast@kernel.org> > Reported-by: Song Liu <songliubraving@fb.com> > Signed-off-by: Yonghong Song <yhs@fb.com> > --- > kernel/bpf/verifier.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 93def76cf32b..eebb2d3e16bf 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -3769,7 +3769,9 @@ static int check_stack_boundary(struct bpf_verifier_env *env, int regno, > goto mark; > > if (state->stack[spi].slot_type[0] == STACK_SPILL && > - state->stack[spi].spilled_ptr.type == SCALAR_VALUE) { > + (state->stack[spi].spilled_ptr.type == SCALAR_VALUE || > + (state->stack[spi].spilled_ptr.type != NOT_INIT && Thinking more on this, your v2 was actually correct since in such case stype would have been STACK_MISC or STACK_ZERO and we would have jumped to goto mark here instead, so the above is not reachable under NOT_INIT. Anyway, I took the v2 in, thanks! > + env->allow_ptr_leaks))) { > __mark_reg_unknown(env, &state->stack[spi].spilled_ptr); > for (j = 0; j < BPF_REG_SIZE; j++) > state->stack[spi].slot_type[j] = STACK_MISC; > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next v3 1/2] bpf: permits pointers on stack for helper calls 2020-12-14 21:26 ` Daniel Borkmann @ 2020-12-15 3:57 ` Yonghong Song 0 siblings, 0 replies; 5+ messages in thread From: Yonghong Song @ 2020-12-15 3:57 UTC (permalink / raw) To: Daniel Borkmann, bpf; +Cc: Alexei Starovoitov, kernel-team, Song Liu On 12/14/20 1:26 PM, Daniel Borkmann wrote: > On 12/11/20 4:41 AM, Yonghong Song wrote: >> Currently, when checking stack memory accessed by helper calls, >> for spills, only PTR_TO_BTF_ID and SCALAR_VALUE are >> allowed. >> >> Song discovered an issue where the below bpf program >> int dump_task(struct bpf_iter__task *ctx) >> { >> struct seq_file *seq = ctx->meta->seq; >> static char[] info = "abc"; >> BPF_SEQ_PRINTF(seq, "%s\n", info); >> return 0; >> } >> may cause a verifier failure. >> >> The verifier output looks like: >> ; struct seq_file *seq = ctx->meta->seq; >> 1: (79) r1 = *(u64 *)(r1 +0) >> ; BPF_SEQ_PRINTF(seq, "%s\n", info); >> 2: (18) r2 = 0xffff9054400f6000 >> 4: (7b) *(u64 *)(r10 -8) = r2 >> 5: (bf) r4 = r10 >> ; >> 6: (07) r4 += -8 >> ; BPF_SEQ_PRINTF(seq, "%s\n", info); >> 7: (18) r2 = 0xffff9054400fe000 >> 9: (b4) w3 = 4 >> 10: (b4) w5 = 8 >> 11: (85) call bpf_seq_printf#126 >> R1_w=ptr_seq_file(id=0,off=0,imm=0) >> R2_w=map_value(id=0,off=0,ks=4,vs=4,imm=0) >> R3_w=inv4 R4_w=fp-8 R5_w=inv8 R10=fp0 fp-8_w=map_value >> last_idx 11 first_idx 0 >> regs=8 stack=0 before 10: (b4) w5 = 8 >> regs=8 stack=0 before 9: (b4) w3 = 4 >> invalid indirect read from stack off -8+0 size 8 >> >> Basically, the verifier complains the map_value pointer at "fp-8" >> location. >> To fix the issue, if env->allow_ptr_leaks is true, let us also permit >> pointers on the stack to be accessible by the helper. >> >> Suggested-by: Alexei Starovoitov <ast@kernel.org> >> Reported-by: Song Liu <songliubraving@fb.com> >> Signed-off-by: Yonghong Song <yhs@fb.com> >> --- >> kernel/bpf/verifier.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >> index 93def76cf32b..eebb2d3e16bf 100644 >> --- a/kernel/bpf/verifier.c >> +++ b/kernel/bpf/verifier.c >> @@ -3769,7 +3769,9 @@ static int check_stack_boundary(struct >> bpf_verifier_env *env, int regno, >> goto mark; >> if (state->stack[spi].slot_type[0] == STACK_SPILL && >> - state->stack[spi].spilled_ptr.type == SCALAR_VALUE) { >> + (state->stack[spi].spilled_ptr.type == SCALAR_VALUE || >> + (state->stack[spi].spilled_ptr.type != NOT_INIT && > > Thinking more on this, your v2 was actually correct since in such case > stype > would have been STACK_MISC or STACK_ZERO and we would have jumped to > goto mark > here instead, so the above is not reachable under NOT_INIT. Anyway, I > took the > v2 in, thanks! Thanks! I missed this (NOT_INIT is associated with STACK_MISC/STACK_ZERO spill type) too. > >> + env->allow_ptr_leaks))) { >> __mark_reg_unknown(env, &state->stack[spi].spilled_ptr); >> for (j = 0; j < BPF_REG_SIZE; j++) >> state->stack[spi].slot_type[j] = STACK_MISC; >> > ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH bpf-next v3 2/2] selftests/bpf: add a test for ptr_to_map_value on stack for helper access 2020-12-11 3:41 [PATCH bpf-next v3 0/2] bpf: permits pointers on stack for helper calls Yonghong Song 2020-12-11 3:41 ` [PATCH bpf-next v3 1/2] " Yonghong Song @ 2020-12-11 3:41 ` Yonghong Song 1 sibling, 0 replies; 5+ messages in thread From: Yonghong Song @ 2020-12-11 3:41 UTC (permalink / raw) To: bpf; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, Song Liu Change bpf_iter_task.c such that pointer to map_value may appear on the stack for bpf_seq_printf() to access. Without previous verifier patch, the bpf_iter test will fail. Acked-by: Song Liu <songliubraving@fb.com> Signed-off-by: Yonghong Song <yhs@fb.com> --- tools/testing/selftests/bpf/progs/bpf_iter_task.c | 3 ++- tools/testing/selftests/bpf/verifier/unpriv.c | 5 +++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_task.c b/tools/testing/selftests/bpf/progs/bpf_iter_task.c index 4983087852a0..b7f32c160f4e 100644 --- a/tools/testing/selftests/bpf/progs/bpf_iter_task.c +++ b/tools/testing/selftests/bpf/progs/bpf_iter_task.c @@ -11,9 +11,10 @@ int dump_task(struct bpf_iter__task *ctx) { struct seq_file *seq = ctx->meta->seq; struct task_struct *task = ctx->task; + static char info[] = " === END ==="; if (task == (void *)0) { - BPF_SEQ_PRINTF(seq, " === END ===\n"); + BPF_SEQ_PRINTF(seq, "%s\n", info); return 0; } diff --git a/tools/testing/selftests/bpf/verifier/unpriv.c b/tools/testing/selftests/bpf/verifier/unpriv.c index 91bb77c24a2e..a3fe0fbaed41 100644 --- a/tools/testing/selftests/bpf/verifier/unpriv.c +++ b/tools/testing/selftests/bpf/verifier/unpriv.c @@ -108,8 +108,9 @@ BPF_EXIT_INSN(), }, .fixup_map_hash_8b = { 3 }, - .errstr = "invalid indirect read from stack off -8+0 size 8", - .result = REJECT, + .errstr_unpriv = "invalid indirect read from stack off -8+0 size 8", + .result_unpriv = REJECT, + .result = ACCEPT, }, { "unpriv: mangle pointer on stack 1", -- 2.24.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-12-15 3:58 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-11 3:41 [PATCH bpf-next v3 0/2] bpf: permits pointers on stack for helper calls Yonghong Song 2020-12-11 3:41 ` [PATCH bpf-next v3 1/2] " Yonghong Song 2020-12-14 21:26 ` Daniel Borkmann 2020-12-15 3:57 ` Yonghong Song 2020-12-11 3:41 ` [PATCH bpf-next v3 2/2] selftests/bpf: add a test for ptr_to_map_value on stack for helper access Yonghong Song
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.