Hi William, On 01/12/2017 07:32 PM, William Tu via iovisor-dev wrote:
Hi, I encounter the following BPF verifier error:from 28 to 30: R0=imm1,min_value=1,max_value=1 R1=pkt(id=0,off=0,r=22) R2=pkt_end R3=imm144,min_value=144,max_value=144 R4=imm0,min_value=0,max_value=0 R5=inv48,min_value=2054,max_value=2054 R10=fp 30: (bf) r5 = r3 31: (07) r5 += 23 32: (77) r5 >>= 3 33: (bf) r6 = r1 34: (0f) r6 += r5 cannot add integer value with 0 upper zero bits to ptr_to_packet --- Shouldn't the r5 be an imm value? since it comes from r3=144 and just doing +=23 and shift right 3 bits? --- the C code is doing data and data_end checking: ebpf_filter: ; int ebpf_filter(struct xdp_md* skb){ 0: r2 = *(u32 *)(r1 + 4) ; void *data = (void *)(long)skb->data; 1: r1 = *(u32 *)(r1 + 0) ; u32 ebpf_zero = 0; 2: r3 = 0 3: *(u32 *)(r10 - 4) = r3 4: r0 = 1 ; if (data_end < data + BYTES(ebpf_packetOffsetInBits + 48)) { 5: r3 = r1 6: r3 += 6 7: if r3 > r2 goto 1 8: goto 1 ... LBB0_1: ; if (data_end < data + BYTES(ebpf_packetOffsetInBits + 16)) { 30: r5 = r3 31: r5 += 23 32: r5 >>= 3 33: r6 = r1 34: r6 += r5 35: if r6 > r2 goto 65509 ; if (data_end < data + BYTES(ebpf_packetOffsetInBits + 16)) { ... In which I was checking the packet's boundary. BYTE() is a simple macro doing #define BYTES(w) ((w + 7) >> 3)
Can you try out the below patch? evaluate_reg_imm_alu() is invoked on ALU ops where the dst reg is CONST_IMM and the src is either a constant from the insn or a reg with type CONST_IMM. Currently we only simulate BPF_ADD / BPF_OR and if we encounter something else (like in your case shifts), then we mark the reg from imm to unknown, and as soon as you try to add that to the pkt pointer, then check_packet_ptr_add() will bark due to src_reg->imm being 0 from prior mark_reg_unknown_value(). Given we don't care about overflows or negative values on tracking pure imm ops, we might as well let evaluate_reg_imm_alu() simulate more ops than just these two. Does that fix it for you? Signed-off-by: Daniel Borkmann <[email protected]> --- kernel/bpf/verifier.c | 24 ++++++++++++++++++++++-- tools/testing/selftests/bpf/test_verifier.c | 24 ++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index d60e12c..2ac50ae 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -1567,19 +1567,39 @@ static int evaluate_reg_imm_alu(struct bpf_verifier_env *env, struct bpf_reg_state *src_reg = ®s[insn->src_reg]; u8 opcode = BPF_OP(insn->code); - /* dst_reg->type == CONST_IMM here, simulate execution of 'add'/'or' - * insn. Don't care about overflow or negative values, just add them + /* dst_reg->type == CONST_IMM here, simulate execution of 'add'/'or'/... + * insn. Don't care about overflow or negative values, just add them. */ if (opcode == BPF_ADD && BPF_SRC(insn->code) == BPF_K) dst_reg->imm += insn->imm; else if (opcode == BPF_ADD && BPF_SRC(insn->code) == BPF_X && src_reg->type == CONST_IMM) dst_reg->imm += src_reg->imm; + else if (opcode == BPF_SUB && BPF_SRC(insn->code) == BPF_K) + dst_reg->imm -= insn->imm; + else if (opcode == BPF_SUB && BPF_SRC(insn->code) == BPF_X && + src_reg->type == CONST_IMM) + dst_reg->imm -= src_reg->imm; + else if (opcode == BPF_MUL && BPF_SRC(insn->code) == BPF_K) + dst_reg->imm *= insn->imm; + else if (opcode == BPF_MUL && BPF_SRC(insn->code) == BPF_X && + src_reg->type == CONST_IMM) + dst_reg->imm *= src_reg->imm; else if (opcode == BPF_OR && BPF_SRC(insn->code) == BPF_K) dst_reg->imm |= insn->imm; else if (opcode == BPF_OR && BPF_SRC(insn->code) == BPF_X && src_reg->type == CONST_IMM) dst_reg->imm |= src_reg->imm; + else if (opcode == BPF_RSH && BPF_SRC(insn->code) == BPF_K) + dst_reg->imm >>= insn->imm; + else if (opcode == BPF_RSH && BPF_SRC(insn->code) == BPF_X && + src_reg->type == CONST_IMM) + dst_reg->imm >>= src_reg->imm; + else if (opcode == BPF_LSH && BPF_SRC(insn->code) == BPF_K) + dst_reg->imm <<= insn->imm; + else if (opcode == BPF_LSH && BPF_SRC(insn->code) == BPF_X && + src_reg->type == CONST_IMM) + dst_reg->imm <<= src_reg->imm; else mark_reg_unknown_value(regs, insn->dst_reg); return 0; diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c index 1aa7324..d60bd07 100644 --- a/tools/testing/selftests/bpf/test_verifier.c +++ b/tools/testing/selftests/bpf/test_verifier.c @@ -2326,6 +2326,30 @@ static struct bpf_test tests[] = { .prog_type = BPF_PROG_TYPE_SCHED_CLS, }, { + "direct packet access: test11 (shift, good access)", + .insns = { + BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1, + offsetof(struct __sk_buff, data)), + BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_1, + offsetof(struct __sk_buff, data_end)), + BPF_MOV64_REG(BPF_REG_0, BPF_REG_2), + BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, 22), + BPF_JMP_REG(BPF_JGT, BPF_REG_0, BPF_REG_3, 8), + BPF_MOV64_IMM(BPF_REG_3, 144), + BPF_MOV64_REG(BPF_REG_5, BPF_REG_3), + BPF_ALU64_IMM(BPF_ADD, BPF_REG_5, 23), + BPF_ALU64_IMM(BPF_RSH, BPF_REG_5, 3), + BPF_MOV64_REG(BPF_REG_6, BPF_REG_2), + BPF_ALU64_REG(BPF_ADD, BPF_REG_6, BPF_REG_5), + BPF_MOV64_IMM(BPF_REG_0, 1), + BPF_EXIT_INSN(), + BPF_MOV64_IMM(BPF_REG_0, 0), + BPF_EXIT_INSN(), + }, + .result = ACCEPT, + .prog_type = BPF_PROG_TYPE_SCHED_CLS, + }, + { "helper access to packet: test1, valid packet_ptr range", .insns = { BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1, -- 2.5.5 _______________________________________________ iovisor-dev mailing list [email protected] https://lists.iovisor.org/mailman/listinfo/iovisor-dev
