Re: [PATCH bpf] bpf, arm64: fix out of bounds access in tail call

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

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 <daniel@iogearbox.net>

ouch. nice catch!
Tested on arm64 hw and applied to bpf tree, Thanks Daniel!