On Fri, Feb 23, 2018 at 01:03:43AM +0100, Daniel Borkmann wrote:
> 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 <dan...@iogearbox.net>
ouch. nice catch!
Tested on arm64 hw and applied to bpf tree, Thanks Daniel!
