]> git.proxmox.com Git - mirror_ubuntu-kernels.git/commitdiff
bpf, arm64: fix out of bounds access in tail call
authorDaniel Borkmann <daniel@iogearbox.net>
Fri, 23 Feb 2018 00:03:43 +0000 (01:03 +0100)
committerAlexei Starovoitov <ast@kernel.org>
Fri, 23 Feb 2018 00:06:28 +0000 (16:06 -0800)
I recently noticed a crash on arm64 when feeding a bogus index
into BPF tail call helper. The crash would not occur when the
interpreter is used, but only in case of JIT. Output looks as
follows:

  [  347.007486] Unable to handle kernel paging request at virtual address fffb850e96492510
  [...]
  [  347.043065] [fffb850e96492510] address between user and kernel address ranges
  [  347.050205] Internal error: Oops: 96000004 [#1] SMP
  [...]
  [  347.190829] x13: 0000000000000000 x12: 0000000000000000
  [  347.196128] x11: fffc047ebe782800 x10: ffff808fd7d0fd10
  [  347.201427] x9 : 0000000000000000 x8 : 0000000000000000
  [  347.206726] x7 : 0000000000000000 x6 : 001c991738000000
  [  347.212025] x5 : 0000000000000018 x4 : 000000000000ba5a
  [  347.217325] x3 : 00000000000329c4 x2 : ffff808fd7cf0500
  [  347.222625] x1 : ffff808fd7d0fc00 x0 : ffff808fd7cf0500
  [  347.227926] Process test_verifier (pid: 4548, stack limit = 0x000000007467fa61)
  [  347.235221] Call trace:
  [  347.237656]  0xffff000002f3a4fc
  [  347.240784]  bpf_test_run+0x78/0xf8
  [  347.244260]  bpf_prog_test_run_skb+0x148/0x230
  [  347.248694]  SyS_bpf+0x77c/0x1110
  [  347.251999]  el0_svc_naked+0x30/0x34
  [  347.255564] Code: 9100075a d280220a 8b0a002a d37df04b (f86b694b)
  [...]

In this case the index used in BPF r3 is the same as in r1
at the time of the call, meaning we fed a pointer as index;
here, it had the value 0xffff808fd7cf0500 which sits in x2.

While I found tail calls to be working in general (also for
hitting the error cases), I noticed the following in the code
emission:

  # bpftool p d j i 988
  [...]
  38:   ldr     w10, [x1,x10]
  3c:   cmp     w2, w10
  40:   b.ge    0x000000000000007c              <-- signed cmp
  44:   mov     x10, #0x20                      // #32
  48:   cmp     x26, x10
  4c:   b.gt    0x000000000000007c
  50:   add     x26, x26, #0x1
  54:   mov     x10, #0x110                     // #272
  58:   add     x10, x1, x10
  5c:   lsl     x11, x2, #3
  60:   ldr     x11, [x10,x11]                  <-- faulting insn (f86b694b)
  64:   cbz     x11, 0x000000000000007c
  [...]

Meaning, the tests passed because commit ddb55992b04d ("arm64:
bpf: implement bpf_tail_call() helper") was using signed compares
instead of unsigned which as a result had the test wrongly passing.

Change this but also the tail call count test both into unsigned
and cap the index as u32. Latter we did as well in 90caccdd8cc0
("bpf: fix bpf_tail_call() x64 JIT") and is needed in addition here,
too. Tested on HiSilicon Hi1616.

Result after patch:

  # bpftool p d j i 268
  [...]
  38: ldr w10, [x1,x10]
  3c: add w2, w2, #0x0
  40: cmp w2, w10
  44: b.cs 0x0000000000000080
  48: mov x10, #0x20                   // #32
  4c: cmp x26, x10
  50: b.hi 0x0000000000000080
  54: add x26, x26, #0x1
  58: mov x10, #0x110                  // #272
  5c: add x10, x1, x10
  60: lsl x11, x2, #3
  64: ldr x11, [x10,x11]
  68: cbz x11, 0x0000000000000080
  [...]

Fixes: ddb55992b04d ("arm64: bpf: implement bpf_tail_call() helper")
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
arch/arm64/net/bpf_jit_comp.c
tools/testing/selftests/bpf/test_verifier.c

index 1d4f1da7c58f8d51371947523e91649915e4320d..a93350451e8ece754a2f565764d47505e48b85ef 100644 (file)
@@ -250,8 +250,9 @@ static int emit_bpf_tail_call(struct jit_ctx *ctx)
        off = offsetof(struct bpf_array, map.max_entries);
        emit_a64_mov_i64(tmp, off, ctx);
        emit(A64_LDR32(tmp, r2, tmp), ctx);
+       emit(A64_MOV(0, r3, r3), ctx);
        emit(A64_CMP(0, r3, tmp), ctx);
-       emit(A64_B_(A64_COND_GE, jmp_offset), ctx);
+       emit(A64_B_(A64_COND_CS, jmp_offset), ctx);
 
        /* if (tail_call_cnt > MAX_TAIL_CALL_CNT)
         *     goto out;
@@ -259,7 +260,7 @@ static int emit_bpf_tail_call(struct jit_ctx *ctx)
         */
        emit_a64_mov_i64(tmp, MAX_TAIL_CALL_CNT, ctx);
        emit(A64_CMP(1, tcc, tmp), ctx);
-       emit(A64_B_(A64_COND_GT, jmp_offset), ctx);
+       emit(A64_B_(A64_COND_HI, jmp_offset), ctx);
        emit(A64_ADD_I(1, tcc, tcc, 1), ctx);
 
        /* prog = array->ptrs[index];
index c0f16e93f9bd14b3ef98b323c6830ca14645509b..c73592fa3d4174815de60cbc8ecb411c95cffd40 100644 (file)
@@ -2586,6 +2586,32 @@ static struct bpf_test tests[] = {
                .result_unpriv = REJECT,
                .result = ACCEPT,
        },
+       {
+               "runtime/jit: pass negative index to tail_call",
+               .insns = {
+                       BPF_MOV64_IMM(BPF_REG_3, -1),
+                       BPF_LD_MAP_FD(BPF_REG_2, 0),
+                       BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
+                                    BPF_FUNC_tail_call),
+                       BPF_MOV64_IMM(BPF_REG_0, 0),
+                       BPF_EXIT_INSN(),
+               },
+               .fixup_prog = { 1 },
+               .result = ACCEPT,
+       },
+       {
+               "runtime/jit: pass > 32bit index to tail_call",
+               .insns = {
+                       BPF_LD_IMM64(BPF_REG_3, 0x100000000ULL),
+                       BPF_LD_MAP_FD(BPF_REG_2, 0),
+                       BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
+                                    BPF_FUNC_tail_call),
+                       BPF_MOV64_IMM(BPF_REG_0, 0),
+                       BPF_EXIT_INSN(),
+               },
+               .fixup_prog = { 2 },
+               .result = ACCEPT,
+       },
        {
                "stack pointer arithmetic",
                .insns = {