Hi Daniel,
Thanks for the patch. Yes, it solves my issue. Then there is another
related due to BPF_AND
LBB0_12:
; if (ebpf_packetEnd < ebpf_packetStart + BYTES(ebpf_packetOffsetInBits + 64)) {
from 52 to 80: R0=imm1,min_value=1,max_value=1 R1=pkt(id=0,off=0,r=34)
R2=pkt_end R3=inv R4=imm272 R5=inv56,min_value=17,max_value=17
R6=pkt(id=0,off=26,r=34) R10=fp
80: (07) r4 += 71
81: (18) r5 = 0xfffffff8
83: (5f) r4 &= r5
84: (77) r4 >>= 3
85: (0f) r1 += r4
cannot add integer value with 3 upper zero bits to ptr_to_packet
I have no idea why this time the compiler wants to do "and" then
"right shift", instead of directly shit right 3 bit. R5's type is
UNKNOWN_VALUE, So in addition to your patch, I add
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -1489,11 +1489,46 @@ static int evaluate_reg_imm_alu(struct
bpf_verifier_env *env,
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_AND && BPF_SRC(insn->code) == BPF_K)
+ dst_reg->imm &= insn->imm;
+ else if (opcode == BPF_AND && BPF_SRC(insn->code) == BPF_X &&
+ src_reg->type == CONST_IMM)
+ dst_reg->imm &= src_reg->imm;
+
+ else if (opcode == BPF_AND && BPF_SRC(insn->code) == BPF_X &&
+ src_reg->type == UNKNOWN_VALUE)
+ {
+ dst_reg->min_value &= src_reg->min_value;
+ dst_reg->max_value &= src_reg->max_value;
+ }
Then it passes verifier.
William
On Tue, Jan 17, 2017 at 3:16 PM, Daniel Borkmann <dan...@iogearbox.net> wrote:
> 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 <dan...@iogearbox.net>
> ---
> 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
iovisor-dev@lists.iovisor.org
https://lists.iovisor.org/mailman/listinfo/iovisor-dev
