Message ID | 20190209062554.142612-1-kafai@fb.com |
---|---|
State | Accepted |
Delegated to: | BPF Maintainers |
Headers | show |
Series | [bpf] bpf: Fix narrow load on a bpf_sock returned from sk_lookup() | expand |
On Fri, 8 Feb 2019 at 22:27, Martin KaFai Lau <kafai@fb.com> wrote: > > By adding this test to test_verifier: > { > "reference tracking: access sk->src_ip4 (narrow load)", > .insns = { > BPF_SK_LOOKUP, > BPF_MOV64_REG(BPF_REG_6, BPF_REG_0), > BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 3), > BPF_LDX_MEM(BPF_H, BPF_REG_2, BPF_REG_0, offsetof(struct bpf_sock, src_ip4) + 2), > BPF_MOV64_REG(BPF_REG_1, BPF_REG_6), > BPF_EMIT_CALL(BPF_FUNC_sk_release), > BPF_EXIT_INSN(), > }, > .prog_type = BPF_PROG_TYPE_SCHED_CLS, > .result = ACCEPT, > }, > > The above test loads 2 bytes from sk->src_ip4 where > sk is obtained by bpf_sk_lookup_tcp(). > > It hits an internal verifier error from convert_ctx_accesses(): > [root@arch-fb-vm1 bpf]# ./test_verifier 665 665 > Failed to load prog 'Invalid argument'! > 0: (b7) r2 = 0 > 1: (63) *(u32 *)(r10 -8) = r2 > 2: (7b) *(u64 *)(r10 -16) = r2 > 3: (7b) *(u64 *)(r10 -24) = r2 > 4: (7b) *(u64 *)(r10 -32) = r2 > 5: (7b) *(u64 *)(r10 -40) = r2 > 6: (7b) *(u64 *)(r10 -48) = r2 > 7: (bf) r2 = r10 > 8: (07) r2 += -48 > 9: (b7) r3 = 36 > 10: (b7) r4 = 0 > 11: (b7) r5 = 0 > 12: (85) call bpf_sk_lookup_tcp#84 > 13: (bf) r6 = r0 > 14: (15) if r0 == 0x0 goto pc+3 > R0=sock(id=1,off=0,imm=0) R6=sock(id=1,off=0,imm=0) R10=fp0,call_-1 fp-8=????0000 fp-16=0000mmmm fp-24=mmmmmmmm fp-32=mmmmmmmm fp-40=mmmmmmmm fp-48=mmmmmmmm refs=1 > 15: (69) r2 = *(u16 *)(r0 +26) > 16: (bf) r1 = r6 > 17: (85) call bpf_sk_release#86 > 18: (95) exit > > from 14 to 18: safe > processed 20 insns (limit 131072), stack depth 48 > bpf verifier is misconfigured > Summary: 0 PASSED, 0 SKIPPED, 1 FAILED > > The bpf_sock_is_valid_access() is expecting src_ip4 can be narrowly > loaded (meaning load any 1 or 2 bytes of the src_ip4) by > marking info->ctx_field_size. However, this marked > ctx_field_size is not used. This patch fixes it. > > Due to the recent refactoring in test_verifier, > this new test will be added to the bpf-next branch > (together with the bpf_tcp_sock patchset) > to avoid merge conflict. > > Fixes: c64b7983288e ("bpf: Add PTR_TO_SOCKET verifier type") > Cc: Joe Stringer <joe@wand.net.nz> > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > --- Nice find, thanks. Acked-by: Joe Stringer <joe@wand.net.nz>
On Fri, Feb 08, 2019 at 10:25:54PM -0800, Martin KaFai Lau wrote: > By adding this test to test_verifier: > { > "reference tracking: access sk->src_ip4 (narrow load)", > .insns = { > BPF_SK_LOOKUP, > BPF_MOV64_REG(BPF_REG_6, BPF_REG_0), > BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 3), > BPF_LDX_MEM(BPF_H, BPF_REG_2, BPF_REG_0, offsetof(struct bpf_sock, src_ip4) + 2), > BPF_MOV64_REG(BPF_REG_1, BPF_REG_6), > BPF_EMIT_CALL(BPF_FUNC_sk_release), > BPF_EXIT_INSN(), > }, > .prog_type = BPF_PROG_TYPE_SCHED_CLS, > .result = ACCEPT, > }, > > The above test loads 2 bytes from sk->src_ip4 where > sk is obtained by bpf_sk_lookup_tcp(). > > It hits an internal verifier error from convert_ctx_accesses(): > [root@arch-fb-vm1 bpf]# ./test_verifier 665 665 > Failed to load prog 'Invalid argument'! > 0: (b7) r2 = 0 > 1: (63) *(u32 *)(r10 -8) = r2 > 2: (7b) *(u64 *)(r10 -16) = r2 > 3: (7b) *(u64 *)(r10 -24) = r2 > 4: (7b) *(u64 *)(r10 -32) = r2 > 5: (7b) *(u64 *)(r10 -40) = r2 > 6: (7b) *(u64 *)(r10 -48) = r2 > 7: (bf) r2 = r10 > 8: (07) r2 += -48 > 9: (b7) r3 = 36 > 10: (b7) r4 = 0 > 11: (b7) r5 = 0 > 12: (85) call bpf_sk_lookup_tcp#84 > 13: (bf) r6 = r0 > 14: (15) if r0 == 0x0 goto pc+3 > R0=sock(id=1,off=0,imm=0) R6=sock(id=1,off=0,imm=0) R10=fp0,call_-1 fp-8=????0000 fp-16=0000mmmm fp-24=mmmmmmmm fp-32=mmmmmmmm fp-40=mmmmmmmm fp-48=mmmmmmmm refs=1 > 15: (69) r2 = *(u16 *)(r0 +26) > 16: (bf) r1 = r6 > 17: (85) call bpf_sk_release#86 > 18: (95) exit > > from 14 to 18: safe > processed 20 insns (limit 131072), stack depth 48 > bpf verifier is misconfigured > Summary: 0 PASSED, 0 SKIPPED, 1 FAILED > > The bpf_sock_is_valid_access() is expecting src_ip4 can be narrowly > loaded (meaning load any 1 or 2 bytes of the src_ip4) by > marking info->ctx_field_size. However, this marked > ctx_field_size is not used. This patch fixes it. > > Due to the recent refactoring in test_verifier, > this new test will be added to the bpf-next branch > (together with the bpf_tcp_sock patchset) > to avoid merge conflict. > > Fixes: c64b7983288e ("bpf: Add PTR_TO_SOCKET verifier type") > Cc: Joe Stringer <joe@wand.net.nz> > Signed-off-by: Martin KaFai Lau <kafai@fb.com> Applied to bpf tree. Martin, if your is_fullsock work depends on it, I can apply the fix to bpf-next as well. Just let me know.
On Sat, Feb 09, 2019 at 08:02:43PM -0800, Alexei Starovoitov wrote: > On Fri, Feb 08, 2019 at 10:25:54PM -0800, Martin KaFai Lau wrote: > > By adding this test to test_verifier: > > { > > "reference tracking: access sk->src_ip4 (narrow load)", > > .insns = { > > BPF_SK_LOOKUP, > > BPF_MOV64_REG(BPF_REG_6, BPF_REG_0), > > BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 3), > > BPF_LDX_MEM(BPF_H, BPF_REG_2, BPF_REG_0, offsetof(struct bpf_sock, src_ip4) + 2), > > BPF_MOV64_REG(BPF_REG_1, BPF_REG_6), > > BPF_EMIT_CALL(BPF_FUNC_sk_release), > > BPF_EXIT_INSN(), > > }, > > .prog_type = BPF_PROG_TYPE_SCHED_CLS, > > .result = ACCEPT, > > }, > > > > The above test loads 2 bytes from sk->src_ip4 where > > sk is obtained by bpf_sk_lookup_tcp(). > > > > It hits an internal verifier error from convert_ctx_accesses(): > > [root@arch-fb-vm1 bpf]# ./test_verifier 665 665 > > Failed to load prog 'Invalid argument'! > > 0: (b7) r2 = 0 > > 1: (63) *(u32 *)(r10 -8) = r2 > > 2: (7b) *(u64 *)(r10 -16) = r2 > > 3: (7b) *(u64 *)(r10 -24) = r2 > > 4: (7b) *(u64 *)(r10 -32) = r2 > > 5: (7b) *(u64 *)(r10 -40) = r2 > > 6: (7b) *(u64 *)(r10 -48) = r2 > > 7: (bf) r2 = r10 > > 8: (07) r2 += -48 > > 9: (b7) r3 = 36 > > 10: (b7) r4 = 0 > > 11: (b7) r5 = 0 > > 12: (85) call bpf_sk_lookup_tcp#84 > > 13: (bf) r6 = r0 > > 14: (15) if r0 == 0x0 goto pc+3 > > R0=sock(id=1,off=0,imm=0) R6=sock(id=1,off=0,imm=0) R10=fp0,call_-1 fp-8=????0000 fp-16=0000mmmm fp-24=mmmmmmmm fp-32=mmmmmmmm fp-40=mmmmmmmm fp-48=mmmmmmmm refs=1 > > 15: (69) r2 = *(u16 *)(r0 +26) > > 16: (bf) r1 = r6 > > 17: (85) call bpf_sk_release#86 > > 18: (95) exit > > > > from 14 to 18: safe > > processed 20 insns (limit 131072), stack depth 48 > > bpf verifier is misconfigured > > Summary: 0 PASSED, 0 SKIPPED, 1 FAILED > > > > The bpf_sock_is_valid_access() is expecting src_ip4 can be narrowly > > loaded (meaning load any 1 or 2 bytes of the src_ip4) by > > marking info->ctx_field_size. However, this marked > > ctx_field_size is not used. This patch fixes it. > > > > Due to the recent refactoring in test_verifier, > > this new test will be added to the bpf-next branch > > (together with the bpf_tcp_sock patchset) > > to avoid merge conflict. > > > > Fixes: c64b7983288e ("bpf: Add PTR_TO_SOCKET verifier type") > > Cc: Joe Stringer <joe@wand.net.nz> > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > > Applied to bpf tree. Thanks! > > Martin, if your is_fullsock work depends on it, I can apply the fix > to bpf-next as well. Just let me know. Yes, the is_fullsock work depends on it. I should have mentioned it in this commit log.
On Sun, Feb 10, 2019 at 07:15:17AM +0000, Martin Lau wrote: > > > Fixes: c64b7983288e ("bpf: Add PTR_TO_SOCKET verifier type") > > > Cc: Joe Stringer <joe@wand.net.nz> > > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > > > > Applied to bpf tree. > Thanks! > > > > > Martin, if your is_fullsock work depends on it, I can apply the fix > > to bpf-next as well. Just let me know. > Yes, the is_fullsock work depends on it. > I should have mentioned it in this commit log. Ok. I've pushed it to bpf-next as well. Last time we discusses this scenario at netconf and agreed that git should do the right thing, since commit is the same. I think this is a case where I think it makes sense to give it a shot. If we get issues during pulls/merges it will be a lesson to avoid such things in the future, but if we don't try it we won't know. So applied.
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 56674a7c3778..8f295b790297 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -1617,12 +1617,13 @@ static int check_flow_keys_access(struct bpf_verifier_env *env, int off, return 0; } -static int check_sock_access(struct bpf_verifier_env *env, u32 regno, int off, - int size, enum bpf_access_type t) +static int check_sock_access(struct bpf_verifier_env *env, int insn_idx, + u32 regno, int off, int size, + enum bpf_access_type t) { struct bpf_reg_state *regs = cur_regs(env); struct bpf_reg_state *reg = ®s[regno]; - struct bpf_insn_access_aux info; + struct bpf_insn_access_aux info = {}; if (reg->smin_value < 0) { verbose(env, "R%d min value is negative, either use unsigned index or do a if (index >=0) check.\n", @@ -1636,6 +1637,8 @@ static int check_sock_access(struct bpf_verifier_env *env, u32 regno, int off, return -EACCES; } + env->insn_aux_data[insn_idx].ctx_field_size = info.ctx_field_size; + return 0; } @@ -2032,7 +2035,7 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn verbose(env, "cannot write into socket\n"); return -EACCES; } - err = check_sock_access(env, regno, off, size, t); + err = check_sock_access(env, insn_idx, regno, off, size, t); if (!err && value_regno >= 0) mark_reg_unknown(env, regs, value_regno); } else {
By adding this test to test_verifier: { "reference tracking: access sk->src_ip4 (narrow load)", .insns = { BPF_SK_LOOKUP, BPF_MOV64_REG(BPF_REG_6, BPF_REG_0), BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 3), BPF_LDX_MEM(BPF_H, BPF_REG_2, BPF_REG_0, offsetof(struct bpf_sock, src_ip4) + 2), BPF_MOV64_REG(BPF_REG_1, BPF_REG_6), BPF_EMIT_CALL(BPF_FUNC_sk_release), BPF_EXIT_INSN(), }, .prog_type = BPF_PROG_TYPE_SCHED_CLS, .result = ACCEPT, }, The above test loads 2 bytes from sk->src_ip4 where sk is obtained by bpf_sk_lookup_tcp(). It hits an internal verifier error from convert_ctx_accesses(): [root@arch-fb-vm1 bpf]# ./test_verifier 665 665 Failed to load prog 'Invalid argument'! 0: (b7) r2 = 0 1: (63) *(u32 *)(r10 -8) = r2 2: (7b) *(u64 *)(r10 -16) = r2 3: (7b) *(u64 *)(r10 -24) = r2 4: (7b) *(u64 *)(r10 -32) = r2 5: (7b) *(u64 *)(r10 -40) = r2 6: (7b) *(u64 *)(r10 -48) = r2 7: (bf) r2 = r10 8: (07) r2 += -48 9: (b7) r3 = 36 10: (b7) r4 = 0 11: (b7) r5 = 0 12: (85) call bpf_sk_lookup_tcp#84 13: (bf) r6 = r0 14: (15) if r0 == 0x0 goto pc+3 R0=sock(id=1,off=0,imm=0) R6=sock(id=1,off=0,imm=0) R10=fp0,call_-1 fp-8=????0000 fp-16=0000mmmm fp-24=mmmmmmmm fp-32=mmmmmmmm fp-40=mmmmmmmm fp-48=mmmmmmmm refs=1 15: (69) r2 = *(u16 *)(r0 +26) 16: (bf) r1 = r6 17: (85) call bpf_sk_release#86 18: (95) exit from 14 to 18: safe processed 20 insns (limit 131072), stack depth 48 bpf verifier is misconfigured Summary: 0 PASSED, 0 SKIPPED, 1 FAILED The bpf_sock_is_valid_access() is expecting src_ip4 can be narrowly loaded (meaning load any 1 or 2 bytes of the src_ip4) by marking info->ctx_field_size. However, this marked ctx_field_size is not used. This patch fixes it. Due to the recent refactoring in test_verifier, this new test will be added to the bpf-next branch (together with the bpf_tcp_sock patchset) to avoid merge conflict. Fixes: c64b7983288e ("bpf: Add PTR_TO_SOCKET verifier type") Cc: Joe Stringer <joe@wand.net.nz> Signed-off-by: Martin KaFai Lau <kafai@fb.com> --- kernel/bpf/verifier.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)