On 14 May 2018 at 19:37, Alexei Starovoitov <alexei.starovoi...@gmail.com> wrote: > On Wed, May 09, 2018 at 02:07:02PM -0700, Joe Stringer wrote: >> Teach the verifier a little bit about a new type of pointer, a >> PTR_TO_SOCKET. This pointer type is accessed from BPF through the >> 'struct bpf_sock' structure. >> >> Signed-off-by: Joe Stringer <j...@wand.net.nz> >> --- >> include/linux/bpf.h | 19 +++++++++- >> include/linux/bpf_verifier.h | 2 ++ >> kernel/bpf/verifier.c | 86 >> ++++++++++++++++++++++++++++++++++++++------ >> net/core/filter.c | 30 +++++++++------- >> 4 files changed, 114 insertions(+), 23 deletions(-) > > Ack for patches 1-3. In this one few nits: > >> @@ -1723,6 +1752,16 @@ static int check_mem_access(struct bpf_verifier_env >> *env, int insn_idx, u32 regn >> err = check_packet_access(env, regno, off, size, false); >> if (!err && t == BPF_READ && value_regno >= 0) >> mark_reg_unknown(env, regs, value_regno); >> + >> + } else if (reg->type == PTR_TO_SOCKET) { >> + if (t == BPF_WRITE) { >> + verbose(env, "cannot write into socket\n"); >> + return -EACCES; >> + } >> + err = check_sock_access(env, regno, off, size, t); >> + if (!err && t == BPF_READ && value_regno >= 0) > > t == BPF_READ check is unnecessary. > >> @@ -5785,7 +5845,13 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr >> *attr) >> >> if (ret == 0) >> /* program is valid, convert *(u32*)(ctx + off) accesses */ >> - ret = convert_ctx_accesses(env); >> + ret = convert_ctx_accesses(env, env->ops->convert_ctx_access, >> + PTR_TO_CTX); >> + >> + if (ret == 0) >> + /* Convert *(u32*)(sock_ops + off) accesses */ >> + ret = convert_ctx_accesses(env, bpf_sock_convert_ctx_access, >> + PTR_TO_SOCKET); > > Overall looks great. > Only this part is missing for PTR_TO_SOCKET: > } else if (dst_reg_type != *prev_dst_type && > (dst_reg_type == PTR_TO_CTX || > *prev_dst_type == PTR_TO_CTX)) { > verbose(env, "same insn cannot be used with different > pointers\n"); > return -EINVAL; > similar logic has to be added. > Otherwise the following will be accepted: > > R1 = sock_ptr > goto X; > ... > R1 = some_other_valid_ptr; > goto X; > ... > > R2 = *(u32 *)(R1 + 0); > this will be rewritten for first branch, > but it's wrong for second. >
Thanks for the review, will address these comments.