Netdev Archive on lore.kernel.org
 help / Atom feed
* [PATCH bpf] bpf: Fix narrow load on a bpf_sock returned from sk_lookup()
@ 2019-02-09  6:25 Martin KaFai Lau
  2019-02-10  1:47 ` Joe Stringer
  2019-02-10  4:02 ` Alexei Starovoitov
  0 siblings, 2 replies; 5+ messages in thread
From: Martin KaFai Lau @ 2019-02-09  6:25 UTC (permalink / raw)
  To: netdev; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, Joe Stringer

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(-)

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 = &regs[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 {
-- 
2.17.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH bpf] bpf: Fix narrow load on a bpf_sock returned from sk_lookup()
  2019-02-09  6:25 [PATCH bpf] bpf: Fix narrow load on a bpf_sock returned from sk_lookup() Martin KaFai Lau
@ 2019-02-10  1:47 ` Joe Stringer
  2019-02-10  4:02 ` Alexei Starovoitov
  1 sibling, 0 replies; 5+ messages in thread
From: Joe Stringer @ 2019-02-10  1:47 UTC (permalink / raw)
  To: Martin KaFai Lau
  Cc: netdev, Alexei Starovoitov, Daniel Borkmann, kernel-team, Joe Stringer

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>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH bpf] bpf: Fix narrow load on a bpf_sock returned from sk_lookup()
  2019-02-09  6:25 [PATCH bpf] bpf: Fix narrow load on a bpf_sock returned from sk_lookup() Martin KaFai Lau
  2019-02-10  1:47 ` Joe Stringer
@ 2019-02-10  4:02 ` Alexei Starovoitov
  2019-02-10  7:15   ` Martin Lau
  1 sibling, 1 reply; 5+ messages in thread
From: Alexei Starovoitov @ 2019-02-10  4:02 UTC (permalink / raw)
  To: Martin KaFai Lau
  Cc: netdev, Alexei Starovoitov, Daniel Borkmann, kernel-team, Joe Stringer

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.


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH bpf] bpf: Fix narrow load on a bpf_sock returned from sk_lookup()
  2019-02-10  4:02 ` Alexei Starovoitov
@ 2019-02-10  7:15   ` Martin Lau
  2019-02-11  3:54     ` Alexei Starovoitov
  0 siblings, 1 reply; 5+ messages in thread
From: Martin Lau @ 2019-02-10  7:15 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: netdev, Alexei Starovoitov, Daniel Borkmann, Kernel Team, Joe Stringer

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.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH bpf] bpf: Fix narrow load on a bpf_sock returned from sk_lookup()
  2019-02-10  7:15   ` Martin Lau
@ 2019-02-11  3:54     ` Alexei Starovoitov
  0 siblings, 0 replies; 5+ messages in thread
From: Alexei Starovoitov @ 2019-02-11  3:54 UTC (permalink / raw)
  To: Martin Lau
  Cc: netdev, Alexei Starovoitov, Daniel Borkmann, Kernel Team, Joe Stringer

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.


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-09  6:25 [PATCH bpf] bpf: Fix narrow load on a bpf_sock returned from sk_lookup() Martin KaFai Lau
2019-02-10  1:47 ` Joe Stringer
2019-02-10  4:02 ` Alexei Starovoitov
2019-02-10  7:15   ` Martin Lau
2019-02-11  3:54     ` Alexei Starovoitov

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org netdev@archiver.kernel.org
	public-inbox-index netdev


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox