Skip to contentSkip to conversation

bpf: fix divides by zero

PATCH Superseded Eric Dumazet 2 participants 5 messages Jan 13, 2018 – Jan 14, 2018

Subsystems (4)
Networking Network drivers BPF BPF core
lore
Share & tools
lore

Eric Dumazet

author
#0 lore raw

PATCH 1/1 bpf: fix divides by zero

From: Eric Dumazet <[email protected]>

Divides by zero are not nice, lets avoid them if possible.

Also do_div() seems not needed when dealing with 32bit operands, but this seems a minor detail.

Fixes bd4cf0ed331a ("net: filter: rework/optimize internal BPF interpreter's instruction set")
Signed-off-by Eric Dumazet <[email protected]>
Reported-by syzbot <[email protected]>

---
 kernel/bpf/core.c |    4 ++--

 1 file changed, 2 insertions(+), 2 deletions(-)
kernel/bpf/core.c +0 -0

index 51ec2dda7f08c6c90af084589bb6d80662c77d12..7949e8b8f94e9cc196e0449214493 ccce61b0903 100644

kernel/bpf/core.c +0 -0
@@ -956,7 +956,7 @@ static unsigned int ___bpf_prog_run(u64 *regs,

const struct bpf_insn *insn,
  DST = tmp;
  CONT;
  ALU_MOD_X:
- if (unlikely(SRC == 0))
+ if (unlikely((u32)SRC == 0))
  return 0;
  tmp = (u32) DST;
  DST = do_div(tmp, (u32) SRC);
@@ -975,7 +975,7 @@ static unsigned int ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn,
  DST = div64_u64(DST, SRC);
  CONT;
  ALU_DIV_X:
- if (unlikely(SRC == 0))
+ if (unlikely((u32)SRC == 0))
  return 0;
  tmp = (u32) DST;
  do_div(tmp, (u32) SRC);

Acked-by on v1 1/1
#2 lore raw
In reply to Eric Dumazet
Quoted 25 lines · Eric Dumazet

From: Eric Dumazet <[email protected]>

Divides by zero are not nice, lets avoid them if possible.

Also do_div() seems not needed when dealing with 32bit operands, but this seems a minor detail.

Fixes bd4cf0ed331a ("net: filter: rework/optimize internal BPF interpreter's instruction set")
Signed-off-by Eric Dumazet <[email protected]>
Reported-by syzbot <[email protected]>

---
 kernel/bpf/core.c |    4 ++--

 1 file changed, 2 insertions(+), 2 deletions(-)
kernel/bpf/core.c +0 -0

index 51ec2dda7f08c6c90af084589bb6d80662c77d12..7949e8b8f94e9cc196e0449214493 ccce61b0903 100644

kernel/bpf/core.c +0 -0
@@ -956,7 +956,7 @@ static unsigned int ___bpf_prog_run(u64 *regs,
 const struct bpf_insn *insn,
  		DST = tmp;
  		CONT;

  ALU_MOD_X:

kernel/bpf/core.c

Quoted 2 lines
kernel/bpf/core.c +1 -1
-		if (unlikely(SRC == 0))
+		if (unlikely((u32)SRC == 0))

wow.

Acked-by Alexei Starovoitov <[email protected]>

we likely need to fix all JITs as well.
At least x64, arm64, sparc have the same bug.

Long term it's probably better to move all such checks out of JITs and interpreter into the verifier and patch div/mod with additional 'if src == 0'. This way we can do any type of error reporting and/or aborting execution.

author
#3 lore raw

PATCH v2 1/1 bpf: fix divides by zero

In reply to Eric Dumazet

From: Eric Dumazet <[email protected]>

Divides by zero are not nice, lets avoid them if possible.

Also do_div() seems not needed when dealing with 32bit operands, but this seems a minor detail.

Fixes bd4cf0ed331a ("net: filter: rework/optimize internal BPF interpreter's instruction set")
Signed-off-by Eric Dumazet <[email protected]>
Reported-by syzbot <[email protected]>

---
v2: kernel patches 101 : do not mangle patch :/

 kernel/bpf/core.c |    4 ++--

 1 file changed, 2 insertions(+), 2 deletions(-)
kernel/bpf/core.c +2 -2
index 51ec2dda7f08c6c90af084589bb6d80662c77d12..7949e8b8f94e9cc196e0449214493ccce61b0903 100644
@@ -956,7 +956,7 @@ static unsigned int ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn,
 		DST = tmp;
 		CONT;
 	ALU_MOD_X:
-		if (unlikely(SRC == 0))
+		if (unlikely((u32)SRC == 0))
 			return 0;
 		tmp = (u32) DST;
 		DST = do_div(tmp, (u32) SRC);

@@ -975,7 +975,7 @@ static unsigned int ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn,
 		DST = div64_u64(DST, SRC);
 		CONT;
 	ALU_DIV_X:
-		if (unlikely(SRC == 0))
+		if (unlikely((u32)SRC == 0))
 			return 0;
 		tmp = (u32) DST;
 		do_div(tmp, (u32) SRC);