| Message ID | 20200504062603.2048735-1-yhs@fb.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: implement bpf iterator for kernel data | expand |
On Sun, May 3, 2020 at 11:28 PM Yonghong Song <yhs@fb.com> wrote: > > This specifically to handle the case like below: > // ptr below is a socket ptr identified by PTR_TO_BTF_ID > u64 param[2] = { ptr, val }; > bpf_seq_printf(seq, fmt, sizeof(fmt), param, sizeof(param)); > > In this case, the 16 bytes stack for "param" contains: > 8 bytes for ptr with spilled PTR_TO_BTF_ID > 8 bytes for val as STACK_MISC > > The current verifier will complain the ptr should not be visible > to the helper. > ... > 16: (7b) *(u64 *)(r10 -64) = r2 > 18: (7b) *(u64 *)(r10 -56) = r1 > 19: (bf) r4 = r10 > ; > 20: (07) r4 += -64 > ; BPF_SEQ_PRINTF(seq, fmt1, (long)s, s->sk_protocol); > 21: (bf) r1 = r6 > 22: (18) r2 = 0xffffa8d00018605a > 24: (b4) w3 = 10 > 25: (b4) w5 = 16 > 26: (85) call bpf_seq_printf#125 > R0=inv(id=0) R1_w=ptr_seq_file(id=0,off=0,imm=0) > R2_w=map_value(id=0,off=90,ks=4,vs=144,imm=0) R3_w=inv10 > R4_w=fp-64 R5_w=inv16 R6=ptr_seq_file(id=0,off=0,imm=0) > R7=ptr_netlink_sock(id=0,off=0,imm=0) R10=fp0 fp-56_w=mmmmmmmm > fp-64_w=ptr_ > last_idx 26 first_idx 13 > regs=8 stack=0 before 25: (b4) w5 = 16 > regs=8 stack=0 before 24: (b4) w3 = 10 > invalid indirect read from stack off -64+0 size 16 > > Let us permit this if the program is a tracing/iter program. > > Signed-off-by: Yonghong Song <yhs@fb.com> > --- LGTM, but I wonder why enabling this only for iterator programs? Acked-by: Andrii Nakryiko <andriin@fb.com> > kernel/bpf/verifier.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 36b2a38a06fe..4884b6fd7bad 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -3494,6 +3494,14 @@ static int check_stack_boundary(struct bpf_verifier_env *env, int regno, > *stype = STACK_MISC; > goto mark; > } > + > + /* pointer value can be visible to tracing/iter program */ > + if (env->prog->type == BPF_PROG_TYPE_TRACING && > + env->prog->expected_attach_type == BPF_TRACE_ITER && What's the problem allowing this for all program types? > + state->stack[spi].slot_type[0] == STACK_SPILL && > + state->stack[spi].spilled_ptr.type == PTR_TO_BTF_ID) > + goto mark; > + > if (state->stack[spi].slot_type[0] == STACK_SPILL && > state->stack[spi].spilled_ptr.type == SCALAR_VALUE) { > __mark_reg_unknown(env, &state->stack[spi].spilled_ptr); > -- > 2.24.1 >
On 5/6/20 10:38 AM, Andrii Nakryiko wrote: > On Sun, May 3, 2020 at 11:28 PM Yonghong Song <yhs@fb.com> wrote: >> >> This specifically to handle the case like below: >> // ptr below is a socket ptr identified by PTR_TO_BTF_ID >> u64 param[2] = { ptr, val }; >> bpf_seq_printf(seq, fmt, sizeof(fmt), param, sizeof(param)); >> >> In this case, the 16 bytes stack for "param" contains: >> 8 bytes for ptr with spilled PTR_TO_BTF_ID >> 8 bytes for val as STACK_MISC >> >> The current verifier will complain the ptr should not be visible >> to the helper. >> ... >> 16: (7b) *(u64 *)(r10 -64) = r2 >> 18: (7b) *(u64 *)(r10 -56) = r1 >> 19: (bf) r4 = r10 >> ; >> 20: (07) r4 += -64 >> ; BPF_SEQ_PRINTF(seq, fmt1, (long)s, s->sk_protocol); >> 21: (bf) r1 = r6 >> 22: (18) r2 = 0xffffa8d00018605a >> 24: (b4) w3 = 10 >> 25: (b4) w5 = 16 >> 26: (85) call bpf_seq_printf#125 >> R0=inv(id=0) R1_w=ptr_seq_file(id=0,off=0,imm=0) >> R2_w=map_value(id=0,off=90,ks=4,vs=144,imm=0) R3_w=inv10 >> R4_w=fp-64 R5_w=inv16 R6=ptr_seq_file(id=0,off=0,imm=0) >> R7=ptr_netlink_sock(id=0,off=0,imm=0) R10=fp0 fp-56_w=mmmmmmmm >> fp-64_w=ptr_ >> last_idx 26 first_idx 13 >> regs=8 stack=0 before 25: (b4) w5 = 16 >> regs=8 stack=0 before 24: (b4) w3 = 10 >> invalid indirect read from stack off -64+0 size 16 >> >> Let us permit this if the program is a tracing/iter program. >> >> Signed-off-by: Yonghong Song <yhs@fb.com> >> --- > > LGTM, but I wonder why enabling this only for iterator programs? > > Acked-by: Andrii Nakryiko <andriin@fb.com> > > >> kernel/bpf/verifier.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >> index 36b2a38a06fe..4884b6fd7bad 100644 >> --- a/kernel/bpf/verifier.c >> +++ b/kernel/bpf/verifier.c >> @@ -3494,6 +3494,14 @@ static int check_stack_boundary(struct bpf_verifier_env *env, int regno, >> *stype = STACK_MISC; >> goto mark; >> } >> + >> + /* pointer value can be visible to tracing/iter program */ >> + if (env->prog->type == BPF_PROG_TYPE_TRACING && >> + env->prog->expected_attach_type == BPF_TRACE_ITER && > > What's the problem allowing this for all program types? Just want to conservative here since we may leak kernel pointers. But probably we are fine since the spill type is PTR_TO_BTF_ID which means tracing/raw_tp related bpf programs which should be okay. Will remove the above additional check, which I added in v2 of the patch. > >> + state->stack[spi].slot_type[0] == STACK_SPILL && >> + state->stack[spi].spilled_ptr.type == PTR_TO_BTF_ID) >> + goto mark; >> + >> if (state->stack[spi].slot_type[0] == STACK_SPILL && >> state->stack[spi].spilled_ptr.type == SCALAR_VALUE) { >> __mark_reg_unknown(env, &state->stack[spi].spilled_ptr); >> -- >> 2.24.1 >>
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 36b2a38a06fe..4884b6fd7bad 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -3494,6 +3494,14 @@ static int check_stack_boundary(struct bpf_verifier_env *env, int regno, *stype = STACK_MISC; goto mark; } + + /* pointer value can be visible to tracing/iter program */ + if (env->prog->type == BPF_PROG_TYPE_TRACING && + env->prog->expected_attach_type == BPF_TRACE_ITER && + state->stack[spi].slot_type[0] == STACK_SPILL && + state->stack[spi].spilled_ptr.type == PTR_TO_BTF_ID) + goto mark; + if (state->stack[spi].slot_type[0] == STACK_SPILL && state->stack[spi].spilled_ptr.type == SCALAR_VALUE) { __mark_reg_unknown(env, &state->stack[spi].spilled_ptr);
This specifically to handle the case like below: // ptr below is a socket ptr identified by PTR_TO_BTF_ID u64 param[2] = { ptr, val }; bpf_seq_printf(seq, fmt, sizeof(fmt), param, sizeof(param)); In this case, the 16 bytes stack for "param" contains: 8 bytes for ptr with spilled PTR_TO_BTF_ID 8 bytes for val as STACK_MISC The current verifier will complain the ptr should not be visible to the helper. ... 16: (7b) *(u64 *)(r10 -64) = r2 18: (7b) *(u64 *)(r10 -56) = r1 19: (bf) r4 = r10 ; 20: (07) r4 += -64 ; BPF_SEQ_PRINTF(seq, fmt1, (long)s, s->sk_protocol); 21: (bf) r1 = r6 22: (18) r2 = 0xffffa8d00018605a 24: (b4) w3 = 10 25: (b4) w5 = 16 26: (85) call bpf_seq_printf#125 R0=inv(id=0) R1_w=ptr_seq_file(id=0,off=0,imm=0) R2_w=map_value(id=0,off=90,ks=4,vs=144,imm=0) R3_w=inv10 R4_w=fp-64 R5_w=inv16 R6=ptr_seq_file(id=0,off=0,imm=0) R7=ptr_netlink_sock(id=0,off=0,imm=0) R10=fp0 fp-56_w=mmmmmmmm fp-64_w=ptr_ last_idx 26 first_idx 13 regs=8 stack=0 before 25: (b4) w5 = 16 regs=8 stack=0 before 24: (b4) w3 = 10 invalid indirect read from stack off -64+0 size 16 Let us permit this if the program is a tracing/iter program. Signed-off-by: Yonghong Song <yhs@fb.com> --- kernel/bpf/verifier.c | 8 ++++++++ 1 file changed, 8 insertions(+)