* [PATCH bpf] bpf: fix 32-bit divide by zero
@ 2018-01-13 2:59 Alexei Starovoitov
2018-01-13 16:45 ` Alexei Starovoitov
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Alexei Starovoitov @ 2018-01-13 2:59 UTC (permalink / raw)
To: davem; +Cc: daniel, edumazet, netdev, kernel-team
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
Fixes: 622582786c9e ("net: filter: x86: internal BPF JIT")
Fixes: 7a12b5031c6b ("sparc64: Add eBPF JIT.")
Reported-by: syzbot+48340bb518e88849e2e3@syzkaller.appspotmail.com
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
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(+)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 20eb04fd155e..b7448347e6b6 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -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;
diff --git a/net/core/filter.c b/net/core/filter.c
index d339ef170df6..1c0eb436671f 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -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;
--
2.9.5
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH bpf] bpf: fix 32-bit divide by zero
2018-01-13 2:59 [PATCH bpf] bpf: fix 32-bit divide by zero Alexei Starovoitov
@ 2018-01-13 16:45 ` Alexei Starovoitov
2018-01-14 22:06 ` Daniel Borkmann
2018-01-18 22:30 ` Eric Dumazet
2 siblings, 0 replies; 6+ messages in thread
From: Alexei Starovoitov @ 2018-01-13 16:45 UTC (permalink / raw)
To: Alexei Starovoitov; +Cc: davem, daniel, edumazet, netdev, kernel-team
On Fri, Jan 12, 2018 at 06:59:52PM -0800, Alexei Starovoitov wrote:
> 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
>
> Fixes: 622582786c9e ("net: filter: x86: internal BPF JIT")
> Fixes: 7a12b5031c6b ("sparc64: Add eBPF JIT.")
> Reported-by: syzbot+48340bb518e88849e2e3@syzkaller.appspotmail.com
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---
> 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
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 20eb04fd155e..b7448347e6b6 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -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.
> diff --git a/net/core/filter.c b/net/core/filter.c
> index d339ef170df6..1c0eb436671f 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH bpf] bpf: fix 32-bit divide by zero
2018-01-13 2:59 [PATCH bpf] bpf: fix 32-bit divide by zero Alexei Starovoitov
2018-01-13 16:45 ` Alexei Starovoitov
@ 2018-01-14 22:06 ` Daniel Borkmann
2018-01-18 22:30 ` Eric Dumazet
2 siblings, 0 replies; 6+ messages in thread
From: Daniel Borkmann @ 2018-01-14 22:06 UTC (permalink / raw)
To: Alexei Starovoitov, davem; +Cc: edumazet, netdev, kernel-team
On 01/13/2018 03:59 AM, Alexei Starovoitov wrote:
> 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
>
> Fixes: 622582786c9e ("net: filter: x86: internal BPF JIT")
> Fixes: 7a12b5031c6b ("sparc64: Add eBPF JIT.")
> Reported-by: syzbot+48340bb518e88849e2e3@syzkaller.appspotmail.com
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Applied to bpf as well, thanks Alexei!
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH bpf] bpf: fix 32-bit divide by zero
2018-01-13 2:59 [PATCH bpf] bpf: fix 32-bit divide by zero Alexei Starovoitov
2018-01-13 16:45 ` Alexei Starovoitov
2018-01-14 22:06 ` Daniel Borkmann
@ 2018-01-18 22:30 ` Eric Dumazet
2018-01-18 22:40 ` Alexei Starovoitov
2 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2018-01-18 22:30 UTC (permalink / raw)
To: Alexei Starovoitov, davem; +Cc: daniel, edumazet, netdev, kernel-team
On Fri, 2018-01-12 at 18:59 -0800, Alexei Starovoitov wrote:
> 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 :
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 87f214fbe66ec163d24b12b6defc7edab612ecc9..91e4ab69573e09f793eb1c1e29d1b5ffad1d5dc7 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -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);
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH bpf] bpf: fix 32-bit divide by zero
2018-01-18 22:30 ` Eric Dumazet
@ 2018-01-18 22:40 ` Alexei Starovoitov
2018-01-18 23:33 ` Daniel Borkmann
0 siblings, 1 reply; 6+ messages in thread
From: Alexei Starovoitov @ 2018-01-18 22:40 UTC (permalink / raw)
To: Eric Dumazet, Alexei Starovoitov, davem
Cc: daniel, edumazet, netdev, kernel-team
On 1/18/18 2:30 PM, Eric Dumazet wrote:
> On Fri, 2018-01-12 at 18:59 -0800, Alexei Starovoitov wrote:
>> 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 ?
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf] bpf: fix 32-bit divide by zero
2018-01-18 22:40 ` Alexei Starovoitov
@ 2018-01-18 23:33 ` Daniel Borkmann
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Borkmann @ 2018-01-18 23:33 UTC (permalink / raw)
To: Alexei Starovoitov, Eric Dumazet, Alexei Starovoitov, davem
Cc: edumazet, netdev, kernel-team
On 01/18/2018 11:40 PM, Alexei Starovoitov wrote:
> On 1/18/18 2:30 PM, Eric Dumazet wrote:
>> On Fri, 2018-01-12 at 18:59 -0800, Alexei Starovoitov wrote:
>>> 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 ?
>
> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-01-18 23:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-01-13 2:59 [PATCH bpf] bpf: fix 32-bit divide by zero Alexei Starovoitov
2018-01-13 16:45 ` Alexei Starovoitov
2018-01-14 22:06 ` Daniel Borkmann
2018-01-18 22:30 ` Eric Dumazet
2018-01-18 22:40 ` Alexei Starovoitov
2018-01-18 23:33 ` 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).