* SOCKET_FILTER regression - eBPF can't subtract when attached from unprivileged user @ 2019-03-01 1:06 Marek Majkowski 2019-03-01 11:39 ` Arthur Fabre 0 siblings, 1 reply; 6+ messages in thread From: Marek Majkowski @ 2019-03-01 1:06 UTC (permalink / raw) To: ast, Daniel Borkmann, netdev Howdy, After some dramatic debugging, I think I managed to isolate a problem that looks like a funny eBPF runtime regression. It seems to be introduced somewhere after 4.14. The eBPF in question is running on network sockets with SO_ATTACH_BPF. The BPF_PROG_TYPE_SOCKET_FILTER code: uint64_t a = bpf_ktime_get_ns(); uint64_t b = bpf_ktime_get_ns(); uint64_t delta = b - a; if ((int64_t)delta > 0) { Depending on a context, the "delta" variable is set to a wrong value. The compiled bytecode seems fine: Disassembly of section socket1: bpf_prog1: ; { 0: 85 00 00 00 05 00 00 00 call 5 ; uint64_t a = bpf_ktime_get_ns(); 1: bf 07 00 00 00 00 00 00 r7 = r0 ; uint64_t b = bpf_ktime_get_ns(); 2: 85 00 00 00 05 00 00 00 call 5 3: bf 06 00 00 00 00 00 00 r6 = r0 ; uint64_t delta = b - a; 4: bf 68 00 00 00 00 00 00 r8 = r6 5: 1f 78 00 00 00 00 00 00 r8 -= r7 ; if ((int64_t)delta > 0) { 6: b7 01 00 00 01 00 00 00 r1 = 1 7: 6d 81 0a 00 00 00 00 00 if r1 s> r8 goto +10 XXX The code runs fine from root. From unprivileged user though, the value of "delta" is a wrapped negative. Both "a" and "b" are fine in both root and non-root cases. Technically bpf_ktime_get_ns() can go backwards, but this isn't the case here. It does seem like the problem is with the behaiviour of ... the subtraction operation when running from SO_ATTACH_BPF executed by non-root? Code: https://gist.github.com/majek/d0bb75a8c62cc35bec2b342054084aab git clone https://gist.github.com/majek/d0bb75a8c62cc35bec2b342054084aab cd d0bb75a8c62cc35bec2b342054084aab make Then: $ sudo ./ebpf-bug 0 -> 0 0x0000000000000000 1 -> 12585651690481 0x00000b72534c5bf1 2 -> 12585651690697 0x00000b72534c5cc9 3 -> 216 0x00000000000000d8 $ ./ebpf-bug 0 -> 1 0x0000000000000001 1 -> 12581437028489 0x00000b715815b889 2 -> 12581437028689 0x00000b715815b951 3 -> 18446731492272523127 0xfffff48ea7ea4777 "1" shows "a" "2" shows "b" "3" shows "detla" As you see "delta" is off the scale for unprivileged user run. I don't see any reason why root vs non-root should make any difference for this code. For completeness, this was tested with jit disabled: $ cat /proc/version Linux version 5.0.0-rc6+ (marek@mrbreeze) (gcc version 7.3.0) $ sudo sysctl -a|grep -i jit net.core.bpf_jit_enable = 0 net.core.bpf_jit_harden = 0 The same test on 4.14 seems fine: $ cat /proc/version Linux version 4.14.83-cloudflare-2018.11.4 (gcc version 8.2.0 (GCC)) $ sudo ./ebpf-bug 0 -> 0 0x0000000000000000 1 -> 7435203111991321 0x001a6a472052b819 2 -> 7435203111991429 0x001a6a472052b885 3 -> 108 0x000000000000006c $ ./ebpf-bug 0 -> 0 0x0000000000000000 1 -> 7435205114618775 0x001a6a4797b06397 2 -> 7435205114618883 0x001a6a4797b06403 3 -> 108 0x000000000000006c Cheers, Marek ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: SOCKET_FILTER regression - eBPF can't subtract when attached from unprivileged user 2019-03-01 1:06 SOCKET_FILTER regression - eBPF can't subtract when attached from unprivileged user Marek Majkowski @ 2019-03-01 11:39 ` Arthur Fabre 2019-03-01 12:51 ` Daniel Borkmann 2019-03-01 14:04 ` Daniel Borkmann 0 siblings, 2 replies; 6+ messages in thread From: Arthur Fabre @ 2019-03-01 11:39 UTC (permalink / raw) To: marek; +Cc: ast, daniel, netdev, afabre I can reproduce this on 4.19.0-3-amd64 both with, and without the JIT enabled. Dumping the "root" and "non-root" programs with bpftool, the subtraction instructions differ: "non-root": 0: (85) call bpf_ktime_get_ns#74944 1: (bf) r7 = r0 2: (85) call bpf_ktime_get_ns#74944 3: (bf) r6 = r0 4: (bf) r8 = r6 5: (b4) w11 = -1 6: (1f) r11 -= r8 7: (4f) r11 |= r8 8: (87) r11 = -r11 9: (c7) r11 s>>= 63 10: (5f) r8 &= r11 "root": 0: (85) call bpf_ktime_get_ns#74944 1: (bf) r7 = r0 2: (85) call bpf_ktime_get_ns#74944 3: (bf) r6 = r0 4: (bf) r8 = r6 The remainder of the instructions are for writing the results in the map, and the instructions are identical. I believe the extra instructions come from "fixup_bpf_calls" in the verifier: if (isneg) *patch++ = BPF_ALU64_IMM(BPF_MUL, off_reg, -1); *patch++ = BPF_MOV32_IMM(BPF_REG_AX, aux->alu_limit - 1); *patch++ = BPF_ALU64_REG(BPF_SUB, BPF_REG_AX, off_reg); *patch++ = BPF_ALU64_REG(BPF_OR, BPF_REG_AX, off_reg); *patch++ = BPF_ALU64_IMM(BPF_NEG, BPF_REG_AX, 0); *patch++ = BPF_ALU64_IMM(BPF_ARSH, BPF_REG_AX, 63); if (issrc) { *patch++ = BPF_ALU64_REG(BPF_AND, BPF_REG_AX, off_reg); insn->src_reg = BPF_REG_AX; } else { *patch++ = BPF_ALU64_REG(BPF_AND, off_reg, BPF_REG_AX); } This was introduced by "bpf: prevent out of bounds speculation on pointer arithmetic" (https://patchwork.ozlabs.org/patch/1039606/). I don't yet understand what's going on. Cheers, Arthur ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: SOCKET_FILTER regression - eBPF can't subtract when attached from unprivileged user 2019-03-01 11:39 ` Arthur Fabre @ 2019-03-01 12:51 ` Daniel Borkmann 2019-03-01 14:04 ` Daniel Borkmann 1 sibling, 0 replies; 6+ messages in thread From: Daniel Borkmann @ 2019-03-01 12:51 UTC (permalink / raw) To: Arthur Fabre, marek; +Cc: ast, netdev On 03/01/2019 12:39 PM, Arthur Fabre wrote: > I can reproduce this on 4.19.0-3-amd64 both with, and without the JIT enabled. > > Dumping the "root" and "non-root" programs with bpftool, > the subtraction instructions differ: > > "non-root": > 0: (85) call bpf_ktime_get_ns#74944 > 1: (bf) r7 = r0 > 2: (85) call bpf_ktime_get_ns#74944 > 3: (bf) r6 = r0 > 4: (bf) r8 = r6 > 5: (b4) w11 = -1 > 6: (1f) r11 -= r8 > 7: (4f) r11 |= r8 > 8: (87) r11 = -r11 > 9: (c7) r11 s>>= 63 > 10: (5f) r8 &= r11 > > "root": > 0: (85) call bpf_ktime_get_ns#74944 > 1: (bf) r7 = r0 > 2: (85) call bpf_ktime_get_ns#74944 > 3: (bf) r6 = r0 > 4: (bf) r8 = r6 > > The remainder of the instructions are for writing the results in the map, > and the instructions are identical. > > I believe the extra instructions come from "fixup_bpf_calls" in the verifier: > > if (isneg) > *patch++ = BPF_ALU64_IMM(BPF_MUL, off_reg, -1); > *patch++ = BPF_MOV32_IMM(BPF_REG_AX, aux->alu_limit - 1); > *patch++ = BPF_ALU64_REG(BPF_SUB, BPF_REG_AX, off_reg); > *patch++ = BPF_ALU64_REG(BPF_OR, BPF_REG_AX, off_reg); > *patch++ = BPF_ALU64_IMM(BPF_NEG, BPF_REG_AX, 0); > *patch++ = BPF_ALU64_IMM(BPF_ARSH, BPF_REG_AX, 63); > if (issrc) { > *patch++ = BPF_ALU64_REG(BPF_AND, BPF_REG_AX, > off_reg); > insn->src_reg = BPF_REG_AX; > } else { > *patch++ = BPF_ALU64_REG(BPF_AND, off_reg, > BPF_REG_AX); > } > > This was introduced by "bpf: prevent out of bounds speculation on pointer arithmetic" > (https://patchwork.ozlabs.org/patch/1039606/). > I don't yet understand what's going on. Hmm, thanks for the report, I'll take a look right away! There's no map involved here it seems, so there shouldn't be such fixup. Cheers, Daniel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: SOCKET_FILTER regression - eBPF can't subtract when attached from unprivileged user 2019-03-01 11:39 ` Arthur Fabre 2019-03-01 12:51 ` Daniel Borkmann @ 2019-03-01 14:04 ` Daniel Borkmann 2019-03-01 14:10 ` Marek Majkowski 1 sibling, 1 reply; 6+ messages in thread From: Daniel Borkmann @ 2019-03-01 14:04 UTC (permalink / raw) To: Arthur Fabre, marek; +Cc: ast, netdev On 03/01/2019 12:39 PM, Arthur Fabre wrote: > I can reproduce this on 4.19.0-3-amd64 both with, and without the JIT enabled. > > Dumping the "root" and "non-root" programs with bpftool, > the subtraction instructions differ: > > "non-root": > 0: (85) call bpf_ktime_get_ns#74944 > 1: (bf) r7 = r0 > 2: (85) call bpf_ktime_get_ns#74944 > 3: (bf) r6 = r0 > 4: (bf) r8 = r6 > 5: (b4) w11 = -1 > 6: (1f) r11 -= r8 > 7: (4f) r11 |= r8 > 8: (87) r11 = -r11 > 9: (c7) r11 s>>= 63 > 10: (5f) r8 &= r11 > > "root": > 0: (85) call bpf_ktime_get_ns#74944 > 1: (bf) r7 = r0 > 2: (85) call bpf_ktime_get_ns#74944 > 3: (bf) r6 = r0 > 4: (bf) r8 = r6 > > The remainder of the instructions are for writing the results in the map, > and the instructions are identical. > > I believe the extra instructions come from "fixup_bpf_calls" in the verifier: > > if (isneg) > *patch++ = BPF_ALU64_IMM(BPF_MUL, off_reg, -1); > *patch++ = BPF_MOV32_IMM(BPF_REG_AX, aux->alu_limit - 1); > *patch++ = BPF_ALU64_REG(BPF_SUB, BPF_REG_AX, off_reg); > *patch++ = BPF_ALU64_REG(BPF_OR, BPF_REG_AX, off_reg); > *patch++ = BPF_ALU64_IMM(BPF_NEG, BPF_REG_AX, 0); > *patch++ = BPF_ALU64_IMM(BPF_ARSH, BPF_REG_AX, 63); > if (issrc) { > *patch++ = BPF_ALU64_REG(BPF_AND, BPF_REG_AX, > off_reg); > insn->src_reg = BPF_REG_AX; > } else { > *patch++ = BPF_ALU64_REG(BPF_AND, off_reg, > BPF_REG_AX); > } > > This was introduced by "bpf: prevent out of bounds speculation on pointer arithmetic" > (https://patchwork.ozlabs.org/patch/1039606/). > I don't yet understand what's going on. Ok, sigh, fix is this, sorry about the braino: diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index cdd2cb01f789..5b3cd384df1d 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -7629,7 +7629,8 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env) u32 off_reg; aux = &env->insn_aux_data[i + delta]; - if (!aux->alu_state) + if (!aux->alu_state || + aux->alu_state == BPF_ALU_NON_POINTER) continue; isneg = aux->alu_state & BPF_ALU_NEG_VALUE; And this also makes the test work again: foo@test:/root/d0bb75a8c62cc35bec2b342054084aab-7cc37a3a93c8b4028e977f3131feaf7f8705e6a7$ ./ebpf-bug 0 -> 0 0x0000000000000000 1 -> 54645145816 0x0000000cb91ac0d8 2 -> 54645145860 0x0000000cb91ac104 3 -> 44 0x000000000000002c foo@test:/root/d0bb75a8c62cc35bec2b342054084aab-7cc37a3a93c8b4028e977f3131feaf7f8705e6a7$ exit root@test:~/d0bb75a8c62cc35bec2b342054084aab-7cc37a3a93c8b4028e977f3131feaf7f8705e6a7# ./ebpf-bug 0 -> 0 0x0000000000000000 1 -> 57984017624 0x0000000d801de4d8 2 -> 57984017673 0x0000000d801de509 3 -> 49 0x0000000000000031 I'll cook it as proper patch in a bit along with a test case. Thanks for reporting! Daniel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: SOCKET_FILTER regression - eBPF can't subtract when attached from unprivileged user 2019-03-01 14:04 ` Daniel Borkmann @ 2019-03-01 14:10 ` Marek Majkowski 2019-03-01 14:22 ` Daniel Borkmann 0 siblings, 1 reply; 6+ messages in thread From: Marek Majkowski @ 2019-03-01 14:10 UTC (permalink / raw) To: Daniel Borkmann; +Cc: Arthur Fabre, ast, netdev Great, appreciated. One more thing (since upgrading kernels takes time) do you think I can amend eBPF on my side to avoid triggering this? Naive stuff like this doesn't work sadly: uint64_t delta = b + ~a + 1; I tried couple more variants with uint32_t types, but to no avail. Ideas? Marek On Fri, Mar 1, 2019 at 3:04 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 03/01/2019 12:39 PM, Arthur Fabre wrote: > > I can reproduce this on 4.19.0-3-amd64 both with, and without the JIT enabled. > > > > Dumping the "root" and "non-root" programs with bpftool, > > the subtraction instructions differ: > > > > "non-root": > > 0: (85) call bpf_ktime_get_ns#74944 > > 1: (bf) r7 = r0 > > 2: (85) call bpf_ktime_get_ns#74944 > > 3: (bf) r6 = r0 > > 4: (bf) r8 = r6 > > 5: (b4) w11 = -1 > > 6: (1f) r11 -= r8 > > 7: (4f) r11 |= r8 > > 8: (87) r11 = -r11 > > 9: (c7) r11 s>>= 63 > > 10: (5f) r8 &= r11 > > > > "root": > > 0: (85) call bpf_ktime_get_ns#74944 > > 1: (bf) r7 = r0 > > 2: (85) call bpf_ktime_get_ns#74944 > > 3: (bf) r6 = r0 > > 4: (bf) r8 = r6 > > > > The remainder of the instructions are for writing the results in the map, > > and the instructions are identical. > > > > I believe the extra instructions come from "fixup_bpf_calls" in the verifier: > > > > if (isneg) > > *patch++ = BPF_ALU64_IMM(BPF_MUL, off_reg, -1); > > *patch++ = BPF_MOV32_IMM(BPF_REG_AX, aux->alu_limit - 1); > > *patch++ = BPF_ALU64_REG(BPF_SUB, BPF_REG_AX, off_reg); > > *patch++ = BPF_ALU64_REG(BPF_OR, BPF_REG_AX, off_reg); > > *patch++ = BPF_ALU64_IMM(BPF_NEG, BPF_REG_AX, 0); > > *patch++ = BPF_ALU64_IMM(BPF_ARSH, BPF_REG_AX, 63); > > if (issrc) { > > *patch++ = BPF_ALU64_REG(BPF_AND, BPF_REG_AX, > > off_reg); > > insn->src_reg = BPF_REG_AX; > > } else { > > *patch++ = BPF_ALU64_REG(BPF_AND, off_reg, > > BPF_REG_AX); > > } > > > > This was introduced by "bpf: prevent out of bounds speculation on pointer arithmetic" > > (https://patchwork.ozlabs.org/patch/1039606/). > > I don't yet understand what's going on. > > Ok, sigh, fix is this, sorry about the braino: > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index cdd2cb01f789..5b3cd384df1d 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -7629,7 +7629,8 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env) > u32 off_reg; > > aux = &env->insn_aux_data[i + delta]; > - if (!aux->alu_state) > + if (!aux->alu_state || > + aux->alu_state == BPF_ALU_NON_POINTER) > continue; > > isneg = aux->alu_state & BPF_ALU_NEG_VALUE; > > And this also makes the test work again: > > foo@test:/root/d0bb75a8c62cc35bec2b342054084aab-7cc37a3a93c8b4028e977f3131feaf7f8705e6a7$ ./ebpf-bug > 0 -> 0 0x0000000000000000 > 1 -> 54645145816 0x0000000cb91ac0d8 > 2 -> 54645145860 0x0000000cb91ac104 > 3 -> 44 0x000000000000002c > foo@test:/root/d0bb75a8c62cc35bec2b342054084aab-7cc37a3a93c8b4028e977f3131feaf7f8705e6a7$ exit > root@test:~/d0bb75a8c62cc35bec2b342054084aab-7cc37a3a93c8b4028e977f3131feaf7f8705e6a7# ./ebpf-bug > 0 -> 0 0x0000000000000000 > 1 -> 57984017624 0x0000000d801de4d8 > 2 -> 57984017673 0x0000000d801de509 > 3 -> 49 0x0000000000000031 > > I'll cook it as proper patch in a bit along with a test case. > > Thanks for reporting! > Daniel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: SOCKET_FILTER regression - eBPF can't subtract when attached from unprivileged user 2019-03-01 14:10 ` Marek Majkowski @ 2019-03-01 14:22 ` Daniel Borkmann 0 siblings, 0 replies; 6+ messages in thread From: Daniel Borkmann @ 2019-03-01 14:22 UTC (permalink / raw) To: Marek Majkowski; +Cc: Arthur Fabre, ast, netdev On 03/01/2019 03:10 PM, Marek Majkowski wrote: > Great, appreciated. > > One more thing (since upgrading kernels takes time) do you think I can > amend eBPF on my side to avoid triggering this? Naive stuff like this > doesn't work sadly: > > uint64_t delta = b + ~a + 1; > > I tried couple more variants with uint32_t types, but to no avail. Ideas? For 32bit based add/sub this would definitely not be triggered, but only latest LLVM supports alu32 emission. Since you guys are using inline asm already, perhaps worth a shot. Thanks, Daniel > Marek > > On Fri, Mar 1, 2019 at 3:04 PM Daniel Borkmann <daniel@iogearbox.net> wrote: >> >> On 03/01/2019 12:39 PM, Arthur Fabre wrote: >>> I can reproduce this on 4.19.0-3-amd64 both with, and without the JIT enabled. >>> >>> Dumping the "root" and "non-root" programs with bpftool, >>> the subtraction instructions differ: >>> >>> "non-root": >>> 0: (85) call bpf_ktime_get_ns#74944 >>> 1: (bf) r7 = r0 >>> 2: (85) call bpf_ktime_get_ns#74944 >>> 3: (bf) r6 = r0 >>> 4: (bf) r8 = r6 >>> 5: (b4) w11 = -1 >>> 6: (1f) r11 -= r8 >>> 7: (4f) r11 |= r8 >>> 8: (87) r11 = -r11 >>> 9: (c7) r11 s>>= 63 >>> 10: (5f) r8 &= r11 >>> >>> "root": >>> 0: (85) call bpf_ktime_get_ns#74944 >>> 1: (bf) r7 = r0 >>> 2: (85) call bpf_ktime_get_ns#74944 >>> 3: (bf) r6 = r0 >>> 4: (bf) r8 = r6 >>> >>> The remainder of the instructions are for writing the results in the map, >>> and the instructions are identical. >>> >>> I believe the extra instructions come from "fixup_bpf_calls" in the verifier: >>> >>> if (isneg) >>> *patch++ = BPF_ALU64_IMM(BPF_MUL, off_reg, -1); >>> *patch++ = BPF_MOV32_IMM(BPF_REG_AX, aux->alu_limit - 1); >>> *patch++ = BPF_ALU64_REG(BPF_SUB, BPF_REG_AX, off_reg); >>> *patch++ = BPF_ALU64_REG(BPF_OR, BPF_REG_AX, off_reg); >>> *patch++ = BPF_ALU64_IMM(BPF_NEG, BPF_REG_AX, 0); >>> *patch++ = BPF_ALU64_IMM(BPF_ARSH, BPF_REG_AX, 63); >>> if (issrc) { >>> *patch++ = BPF_ALU64_REG(BPF_AND, BPF_REG_AX, >>> off_reg); >>> insn->src_reg = BPF_REG_AX; >>> } else { >>> *patch++ = BPF_ALU64_REG(BPF_AND, off_reg, >>> BPF_REG_AX); >>> } >>> >>> This was introduced by "bpf: prevent out of bounds speculation on pointer arithmetic" >>> (https://patchwork.ozlabs.org/patch/1039606/). >>> I don't yet understand what's going on. >> >> Ok, sigh, fix is this, sorry about the braino: >> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >> index cdd2cb01f789..5b3cd384df1d 100644 >> --- a/kernel/bpf/verifier.c >> +++ b/kernel/bpf/verifier.c >> @@ -7629,7 +7629,8 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env) >> u32 off_reg; >> >> aux = &env->insn_aux_data[i + delta]; >> - if (!aux->alu_state) >> + if (!aux->alu_state || >> + aux->alu_state == BPF_ALU_NON_POINTER) >> continue; >> >> isneg = aux->alu_state & BPF_ALU_NEG_VALUE; >> >> And this also makes the test work again: >> >> foo@test:/root/d0bb75a8c62cc35bec2b342054084aab-7cc37a3a93c8b4028e977f3131feaf7f8705e6a7$ ./ebpf-bug >> 0 -> 0 0x0000000000000000 >> 1 -> 54645145816 0x0000000cb91ac0d8 >> 2 -> 54645145860 0x0000000cb91ac104 >> 3 -> 44 0x000000000000002c >> foo@test:/root/d0bb75a8c62cc35bec2b342054084aab-7cc37a3a93c8b4028e977f3131feaf7f8705e6a7$ exit >> root@test:~/d0bb75a8c62cc35bec2b342054084aab-7cc37a3a93c8b4028e977f3131feaf7f8705e6a7# ./ebpf-bug >> 0 -> 0 0x0000000000000000 >> 1 -> 57984017624 0x0000000d801de4d8 >> 2 -> 57984017673 0x0000000d801de509 >> 3 -> 49 0x0000000000000031 >> >> I'll cook it as proper patch in a bit along with a test case. >> >> Thanks for reporting! >> Daniel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-03-01 14:22 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-01 1:06 SOCKET_FILTER regression - eBPF can't subtract when attached from unprivileged user Marek Majkowski 2019-03-01 11:39 ` Arthur Fabre 2019-03-01 12:51 ` Daniel Borkmann 2019-03-01 14:04 ` Daniel Borkmann 2019-03-01 14:10 ` Marek Majkowski 2019-03-01 14:22 ` Daniel Borkmann
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).