[PATCH bpf 0/2] BPF fix and test case

netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH bpf 0/2] BPF fix and test case
@ 2018-07-19 16:18 Daniel Borkmann
  2018-07-19 16:18 ` [PATCH bpf 1/2] bpf, ppc64: fix unexpected r0=0 exit path inside bpf_xadd Daniel Borkmann
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Daniel Borkmann @ 2018-07-19 16:18 UTC (permalink / raw)
  To: alexei.starovoitov; +Cc: naveen.n.rao, sandipan, netdev, Daniel Borkmann

This set adds a ppc64 JIT fix for xadd as well as a missing test
case for verifying whether xadd messes with src/dst reg. Thanks!

Daniel Borkmann (2):
  bpf, ppc64: fix unexpected r0=0 exit path inside bpf_xadd
  bpf: test case to check whether src/dst regs got mangled by xadd

 arch/powerpc/net/bpf_jit_comp64.c           | 29 ++++-----------------
 tools/testing/selftests/bpf/test_verifier.c | 40 +++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 24 deletions(-)

-- 
2.9.5

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

* [PATCH bpf 1/2] bpf, ppc64: fix unexpected r0=0 exit path inside bpf_xadd
  2018-07-19 16:18 [PATCH bpf 0/2] BPF fix and test case Daniel Borkmann
@ 2018-07-19 16:18 ` Daniel Borkmann
  2018-07-19 16:18 ` [PATCH bpf 2/2] bpf: test case to check whether src/dst regs got mangled by xadd Daniel Borkmann
  2018-07-19 23:12 ` [PATCH bpf 0/2] BPF fix and test case Alexei Starovoitov
  2 siblings, 0 replies; 4+ messages in thread
From: Daniel Borkmann @ 2018-07-19 16:18 UTC (permalink / raw)
  To: alexei.starovoitov; +Cc: naveen.n.rao, sandipan, netdev, Daniel Borkmann

None of the JITs is allowed to implement exit paths from the BPF
insn mappings other than BPF_JMP | BPF_EXIT. In the BPF core code
we have a couple of rewrites in eBPF (e.g. LD_ABS / LD_IND) and
in eBPF to cBPF translation to retain old existing behavior where
exceptions may occur; they are also tightly controlled by the
verifier where it disallows some of the features such as BPF to
BPF calls when legacy LD_ABS / LD_IND ops are present in the BPF
program. During recent review of all BPF_XADD JIT implementations
I noticed that the ppc64 one is buggy in that it contains two
jumps to exit paths. This is problematic as this can bypass verifier
expectations e.g. pointed out in commit f6b1b3bf0d5f ("bpf: fix
subprog verifier bypass by div/mod by 0 exception"). The first
exit path is obsoleted by the fix in ca36960211eb ("bpf: allow xadd
only on aligned memory") anyway, and for the second one we need to
do a fetch, add and store loop if the reservation from lwarx/ldarx
was lost in the meantime.

Fixes: 156d0e290e96 ("powerpc/ebpf/jit: Implement JIT compiler for extended BPF")
Reviewed-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Reviewed-by: Sandipan Das <sandipan@linux.vnet.ibm.com>
Tested-by: Sandipan Das <sandipan@linux.vnet.ibm.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
 arch/powerpc/net/bpf_jit_comp64.c | 29 +++++------------------------
 1 file changed, 5 insertions(+), 24 deletions(-)

diff --git a/arch/powerpc/net/bpf_jit_comp64.c b/arch/powerpc/net/bpf_jit_comp64.c
index 380cbf9..c0a9bcd 100644
--- a/arch/powerpc/net/bpf_jit_comp64.c
+++ b/arch/powerpc/net/bpf_jit_comp64.c
@@ -286,6 +286,7 @@ static int bpf_jit_build_body(struct bpf_prog *fp, u32 *image,
 		u64 imm64;
 		u8 *func;
 		u32 true_cond;
+		u32 tmp_idx;
 
 		/*
 		 * addrs[] maps a BPF bytecode address into a real offset from
@@ -637,11 +638,7 @@ static int bpf_jit_build_body(struct bpf_prog *fp, u32 *image,
 		case BPF_STX | BPF_XADD | BPF_W:
 			/* Get EA into TMP_REG_1 */
 			PPC_ADDI(b2p[TMP_REG_1], dst_reg, off);
-			/* error if EA is not word-aligned */
-			PPC_ANDI(b2p[TMP_REG_2], b2p[TMP_REG_1], 0x03);
-			PPC_BCC_SHORT(COND_EQ, (ctx->idx * 4) + 12);
-			PPC_LI(b2p[BPF_REG_0], 0);
-			PPC_JMP(exit_addr);
+			tmp_idx = ctx->idx * 4;
 			/* load value from memory into TMP_REG_2 */
 			PPC_BPF_LWARX(b2p[TMP_REG_2], 0, b2p[TMP_REG_1], 0);
 			/* add value from src_reg into this */
@@ -649,32 +646,16 @@ static int bpf_jit_build_body(struct bpf_prog *fp, u32 *image,
 			/* store result back */
 			PPC_BPF_STWCX(b2p[TMP_REG_2], 0, b2p[TMP_REG_1]);
 			/* we're done if this succeeded */
-			PPC_BCC_SHORT(COND_EQ, (ctx->idx * 4) + (7*4));
-			/* otherwise, let's try once more */
-			PPC_BPF_LWARX(b2p[TMP_REG_2], 0, b2p[TMP_REG_1], 0);
-			PPC_ADD(b2p[TMP_REG_2], b2p[TMP_REG_2], src_reg);
-			PPC_BPF_STWCX(b2p[TMP_REG_2], 0, b2p[TMP_REG_1]);
-			/* exit if the store was not successful */
-			PPC_LI(b2p[BPF_REG_0], 0);
-			PPC_BCC(COND_NE, exit_addr);
+			PPC_BCC_SHORT(COND_NE, tmp_idx);
 			break;
 		/* *(u64 *)(dst + off) += src */
 		case BPF_STX | BPF_XADD | BPF_DW:
 			PPC_ADDI(b2p[TMP_REG_1], dst_reg, off);
-			/* error if EA is not doubleword-aligned */
-			PPC_ANDI(b2p[TMP_REG_2], b2p[TMP_REG_1], 0x07);
-			PPC_BCC_SHORT(COND_EQ, (ctx->idx * 4) + (3*4));
-			PPC_LI(b2p[BPF_REG_0], 0);
-			PPC_JMP(exit_addr);
-			PPC_BPF_LDARX(b2p[TMP_REG_2], 0, b2p[TMP_REG_1], 0);
-			PPC_ADD(b2p[TMP_REG_2], b2p[TMP_REG_2], src_reg);
-			PPC_BPF_STDCX(b2p[TMP_REG_2], 0, b2p[TMP_REG_1]);
-			PPC_BCC_SHORT(COND_EQ, (ctx->idx * 4) + (7*4));
+			tmp_idx = ctx->idx * 4;
 			PPC_BPF_LDARX(b2p[TMP_REG_2], 0, b2p[TMP_REG_1], 0);
 			PPC_ADD(b2p[TMP_REG_2], b2p[TMP_REG_2], src_reg);
 			PPC_BPF_STDCX(b2p[TMP_REG_2], 0, b2p[TMP_REG_1]);
-			PPC_LI(b2p[BPF_REG_0], 0);
-			PPC_BCC(COND_NE, exit_addr);
+			PPC_BCC_SHORT(COND_NE, tmp_idx);
 			break;
 
 		/*
-- 
2.9.5

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

* [PATCH bpf 2/2] bpf: test case to check whether src/dst regs got mangled by xadd
  2018-07-19 16:18 [PATCH bpf 0/2] BPF fix and test case Daniel Borkmann
  2018-07-19 16:18 ` [PATCH bpf 1/2] bpf, ppc64: fix unexpected r0=0 exit path inside bpf_xadd Daniel Borkmann
@ 2018-07-19 16:18 ` Daniel Borkmann
  2018-07-19 23:12 ` [PATCH bpf 0/2] BPF fix and test case Alexei Starovoitov
  2 siblings, 0 replies; 4+ messages in thread
From: Daniel Borkmann @ 2018-07-19 16:18 UTC (permalink / raw)
  To: alexei.starovoitov; +Cc: naveen.n.rao, sandipan, netdev, Daniel Borkmann

We currently do not have such a test case in test_verifier selftests
but it's important to test under bpf_jit_enable=1 to make sure JIT
implementations do not mistakenly mess with src/dst reg for xadd/{w,dw}.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
 tools/testing/selftests/bpf/test_verifier.c | 40 +++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index f5f7bcc..41106d9 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -12005,6 +12005,46 @@ static struct bpf_test tests[] = {
 		.prog_type = BPF_PROG_TYPE_XDP,
 	},
 	{
+		"xadd/w check whether src/dst got mangled, 1",
+		.insns = {
+			BPF_MOV64_IMM(BPF_REG_0, 1),
+			BPF_MOV64_REG(BPF_REG_6, BPF_REG_0),
+			BPF_MOV64_REG(BPF_REG_7, BPF_REG_10),
+			BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_0, -8),
+			BPF_STX_XADD(BPF_DW, BPF_REG_10, BPF_REG_0, -8),
+			BPF_STX_XADD(BPF_DW, BPF_REG_10, BPF_REG_0, -8),
+			BPF_JMP_REG(BPF_JNE, BPF_REG_6, BPF_REG_0, 3),
+			BPF_JMP_REG(BPF_JNE, BPF_REG_7, BPF_REG_10, 2),
+			BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_10, -8),
+			BPF_EXIT_INSN(),
+			BPF_MOV64_IMM(BPF_REG_0, 42),
+			BPF_EXIT_INSN(),
+		},
+		.result = ACCEPT,
+		.prog_type = BPF_PROG_TYPE_SCHED_CLS,
+		.retval = 3,
+	},
+	{
+		"xadd/w check whether src/dst got mangled, 2",
+		.insns = {
+			BPF_MOV64_IMM(BPF_REG_0, 1),
+			BPF_MOV64_REG(BPF_REG_6, BPF_REG_0),
+			BPF_MOV64_REG(BPF_REG_7, BPF_REG_10),
+			BPF_STX_MEM(BPF_W, BPF_REG_10, BPF_REG_0, -8),
+			BPF_STX_XADD(BPF_W, BPF_REG_10, BPF_REG_0, -8),
+			BPF_STX_XADD(BPF_W, BPF_REG_10, BPF_REG_0, -8),
+			BPF_JMP_REG(BPF_JNE, BPF_REG_6, BPF_REG_0, 3),
+			BPF_JMP_REG(BPF_JNE, BPF_REG_7, BPF_REG_10, 2),
+			BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_10, -8),
+			BPF_EXIT_INSN(),
+			BPF_MOV64_IMM(BPF_REG_0, 42),
+			BPF_EXIT_INSN(),
+		},
+		.result = ACCEPT,
+		.prog_type = BPF_PROG_TYPE_SCHED_CLS,
+		.retval = 3,
+	},
+	{
 		"bpf_get_stack return R0 within range",
 		.insns = {
 			BPF_MOV64_REG(BPF_REG_6, BPF_REG_1),
-- 
2.9.5

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

* Re: [PATCH bpf 0/2] BPF fix and test case
  2018-07-19 16:18 [PATCH bpf 0/2] BPF fix and test case Daniel Borkmann
  2018-07-19 16:18 ` [PATCH bpf 1/2] bpf, ppc64: fix unexpected r0=0 exit path inside bpf_xadd Daniel Borkmann
  2018-07-19 16:18 ` [PATCH bpf 2/2] bpf: test case to check whether src/dst regs got mangled by xadd Daniel Borkmann
@ 2018-07-19 23:12 ` Alexei Starovoitov
  2 siblings, 0 replies; 4+ messages in thread
From: Alexei Starovoitov @ 2018-07-19 23:12 UTC (permalink / raw)
  To: Daniel Borkmann; +Cc: naveen.n.rao, sandipan, netdev

On Thu, Jul 19, 2018 at 06:18:34PM +0200, Daniel Borkmann wrote:
> This set adds a ppc64 JIT fix for xadd as well as a missing test
> case for verifying whether xadd messes with src/dst reg. Thanks!

Applied, Thanks

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

end of thread, other threads:[~2018-07-19 23:58 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-07-19 16:18 [PATCH bpf 0/2] BPF fix and test case Daniel Borkmann
2018-07-19 16:18 ` [PATCH bpf 1/2] bpf, ppc64: fix unexpected r0=0 exit path inside bpf_xadd Daniel Borkmann
2018-07-19 16:18 ` [PATCH bpf 2/2] bpf: test case to check whether src/dst regs got mangled by xadd Daniel Borkmann
2018-07-19 23:12 ` [PATCH bpf 0/2] BPF fix and test case Alexei Starovoitov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).