bpf: fix 32-bit divide by zero
Subsystems (7)
Alexei Starovoitov
maintainerPATCH 1/1 bpf: fix 32-bit divide by zero
due to some JITs doing if (src_reg == 0) check in 64-bit mode for div/mod opreations mask upper 32-bits of src register before doing the check
---
arm64 jit seems to be ok
haven't analyzed other JITs
It works around the interpreter bug too, but I think the interpreter worth fixing anyway.
--- kernel/bpf/verifier.c | 18 ++++++++++++++++++ net/core/filter.c | 4 ++++ 2 files changed, 22 insertions(+)
kernel/bpf/verifier.c +18 -0
@@ -4445,6 +4445,24 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)int i, cnt, delta =0 ;for (i =0 ; i < insn_cnt; i++, insn++) { +if (insn->code == (BPF_ALU |BPF_MOD |BPF_X ) || + insn->code == (BPF_ALU |BPF_DIV |BPF_X )) { +/* due to JIT bugs clear upper 32-bits of src register +* before div/mod operation +*/ + insn_buf[0 ] =BPF_MOV32_REG (insn->src_reg, insn->src_reg); + insn_buf[1 ] = *insn; + cnt =2 ; + new_prog =bpf_patch_insn_data (env, i + delta, insn_buf, cnt); +if (!new_prog) +return -ENOMEM ; + + delta += cnt -1 ; + env->prog = prog = new_prog; + insn = new_prog->insnsi + i + delta; +continue ; + } +if (insn->code != (BPF_JMP |BPF_CALL ))continue ;
net/core/filter.c +4 -0
@@ -458,6 +458,10 @@ static int bpf_convert_filter(struct sock_filter *prog, int len, convert_bpf_extensions(fp, &insn))break ; +if (fp->code == (BPF_ALU |BPF_DIV |BPF_X ) || + fp->code == (BPF_ALU |BPF_MOD |BPF_X )) + *insn++ =BPF_MOV32_REG (BPF_REG_X ,BPF_REG_X ); + *insn =BPF_RAW_INSN (fp->code,BPF_REG_A ,BPF_REG_X ,0 , fp->k);break ;
Signature · 1 line
2.9.5
Quoted 11 lines · Alexei Starovoitov
due to some JITs doing if (
src_reg== 0) check in 64-bit mode for div/mod opreations mask upper 32-bits of src register before doing the checkFixes 622582786c9e ("net: filter: x86: internal BPF JIT")Fixes 7a12b5031c6b ("sparc64: Add eBPF JIT.")Reported-by [email protected]Signed-off-by Alexei Starovoitov <[email protected]>---
arm64 jit seems to be ok
haven't analyzed other JITs
s390 looks ok
mips64 looks buggy
arm32 ebpf jit doesn't have if src == 0 check
powerpc looks ok
Quoted 5 lines
kernel/bpf/verifier.c +0
-0
kernel/bpf/verifier.c static int fixup_bpf_calls(struct bpf_verifier_env *env) · line 4454
Quoted 10 lines
kernel/bpf/verifier.c +7
-0@@ -4445,6 +4445,24 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)int i, cnt, delta =0 ;for (i =0 ; i < insn_cnt; i++, insn++) { +if (insn->code == (BPF_ALU |BPF_MOD |BPF_X ) || + insn->code == (BPF_ALU |BPF_DIV |BPF_X )) { +/* due to JIT bugs clear upper 32-bits of src register +* before div/mod operation +*/ + insn_buf[0 ] =BPF_MOV32_REG (insn->src_reg, insn->src_reg); + insn_buf[1 ] = *insn;
long term such mask allows us to insert
'if (src_reg == 0) goto error'
by the verifier and remove corresponding branches from JITs. Without mask such comparison is not possible, because eBPF doesn't have 32-bit compare and jump instructions.
Furthermore the verifier tracks values in the registers and in many cases knows that src_reg cannot be zero, so insertion of 'if (src_reg == 0)' safety check can be conditional.
net/core/filter.c static int bpf_convert_filter(struct sock_filter *prog, int len, · line 465
Quoted 13 lines
net/core/filter.c +4
-0@@ -458,6 +458,10 @@ static int bpf_convert_filter(struct sock_filter *prog, int len, convert_bpf_extensions(fp, &insn))break ; +if (fp->code == (BPF_ALU |BPF_DIV |BPF_X ) || + fp->code == (BPF_ALU |BPF_MOD |BPF_X )) + *insn++ =BPF_MOV32_REG (BPF_REG_X ,BPF_REG_X ); + *insn =BPF_RAW_INSN (fp->code,BPF_REG_A ,BPF_REG_X ,0 , fp->k);
this hunk is not strictly necessary, since in classic->extended conversion all operations are 32-bit and BPF_REG_X will have upper 32-bit cleared before div/mod, so buggy JITs will be fine, but div/mod are slow anyway and extra bpf_mov32_reg won't hurt performance, so I prefer to keep this hunk to have less things to worry about.
Quoted 8 lines · Alexei Starovoitov
due to some JITs doing if (
src_reg== 0) check in 64-bit mode for div/mod opreations mask upper 32-bits of src register before doing the checkFixes 622582786c9e ("net: filter: x86: internal BPF JIT")Fixes 7a12b5031c6b ("sparc64: Add eBPF JIT.")Reported-by [email protected]Signed-off-by Alexei Starovoitov <[email protected]>
Applied to bpf as well, thanks Alexei!
Quoted 4 lines · Alexei Starovoitov
due to some JITs doing if (
src_reg== 0) check in 64-bit mode for div/mod opreations mask upper 32-bits of src register before doing the check
Is the plan to fix JIT, and if they can all be fixed, revert this patch ?
x86 patch would be something like :
arch/x86/net/bpf_jit_comp.c +7 -2
@@ -548,8 +548,13 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,if (BPF_SRC (insn->code) ==BPF_X ) {/* if (src_reg == 0) return 0 */ -/* cmp r11, 0 */ -EMIT4 (0x49 ,0x83 ,0xFB ,0x00 ); +if (BPF_CLASS (insn->code) ==BPF_ALU64 ) { +/* cmp r11, 0 */ +EMIT4 (0x49 ,0x83 ,0xFB ,0x00 ); + }else { +/* cmp r11d, 0 */ +EMIT4 (0x41 ,0x83 ,0xFB ,0x00 ); + }/* jne .+9 (skip over pop, pop, xor and jmp) */ EMIT2 (X86_JNE ,1 +1 +2 +5 );
Quoted 8 lines · Eric Dumazet
Quoted 4 lines · Alexei Starovoitov
due to some JITs doing if (
src_reg== 0) check in 64-bit mode for div/mod opreations mask upper 32-bits of src register before doing the checkIs the plan to fix JIT, and if they can all be fixed, revert this patch ?
No need to fix JITs.
I'd rather remove 'cmp rX, 0' from JITs and let verifier emit it. It's more generic and gives us flexibility to decide what to do with divide by zero and other exceptions.
Daniel Borkmann
maintainerQuoted 6 lines · Alexei Starovoitov
Quoted 5 lines · Eric Dumazet
Quoted 3 lines · Alexei Starovoitov
due to some JITs doing if (
src_reg== 0) check in 64-bit mode for div/mod opreations mask upper 32-bits of src register before doing the check
Quoted 7 lines
Quoted 2 lines
Is the plan to fix JIT, and if they can all be fixed, revert this patch ?
No need to fix JITs.
I'd rather remove 'cmp rX, 0' from JITs and let verifier emit it. It's more generic and gives us flexibility to decide what to do with divide by zero and other exceptions.
Yeah, working on this, but patch most likely for the next window. The 'return 0' is really suboptimal as exception code for some program types, so I'd like to have struct bpf_verifier_ops to define such exception return code to be used, so that the verifier can apply this via bpf insns directly.