Merge pull request #116 from Alan-Jowett/issue115

uBPF behavior around division and modulus is non-conformant
iovisor · Sep 29, 2022 · b3ea3a0 · b3ea3a0
2 parents af0194f + 99a8ce8
commit b3ea3a0
Show file tree

Hide file tree

Showing 15 changed files with 137 additions and 85 deletions.
diff --git a/tests/div-by-zero-imm.data b/tests/div-by-zero-imm.data
@@ -0,0 +1,6 @@
+-- asm
+mov32 r0, 1
+div32 r0, 0
+exit
+-- result
+0x0
diff --git a/tests/div-by-zero-reg.data b/tests/div-by-zero-reg.data
@@ -0,0 +1,7 @@
+-- asm
+mov32 r0, 1
+mov32 r1, 0
+div32 r0, r1
+exit
+-- result
+0x0
diff --git a/tests/div64-by-zero-imm.data b/tests/div64-by-zero-imm.data
@@ -0,0 +1,6 @@
+-- asm
+mov32 r0, 1
+div r0, 0
+exit
+-- result
+0x0
diff --git a/tests/div64-by-zero-reg.data b/tests/div64-by-zero-reg.data
@@ -0,0 +1,7 @@
+-- asm
+mov32 r0, 1
+mov32 r1, 0
+div r0, r1
+exit
+-- result
+0x0
diff --git a/tests/err-div-by-zero-imm.data b/tests/err-div-by-zero-imm.data
diff --git a/tests/err-div-by-zero-reg.data b/tests/err-div-by-zero-reg.data
diff --git a/tests/err-div64-by-zero-reg.data b/tests/err-div64-by-zero-reg.data
diff --git a/tests/err-mod-by-zero-reg.data b/tests/err-mod-by-zero-reg.data
diff --git a/tests/err-mod64-by-zero-reg.data b/tests/err-mod64-by-zero-reg.data
diff --git a/tests/mod-by-zero-imm.data b/tests/mod-by-zero-imm.data
@@ -0,0 +1,6 @@
+-- asm
+mov32 r0, 1
+mod32 r0, 0
+exit
+-- result
+0x1
diff --git a/tests/mod-by-zero-reg.data b/tests/mod-by-zero-reg.data
@@ -0,0 +1,7 @@
+-- asm
+mov32 r0, 1
+mov32 r1, 0
+mod32 r0, r1
+exit
+-- result
+0x1
diff --git a/tests/mod64-by-zero-imm.data b/tests/mod64-by-zero-imm.data
@@ -0,0 +1,6 @@
+-- asm
+mov32 r0, 1
+mod r0, 0
+exit
+-- result
+0x1
diff --git a/tests/mod64-by-zero-reg.data b/tests/mod64-by-zero-reg.data
@@ -0,0 +1,7 @@
+-- asm
+mov32 r0, 1
+mov32 r1, 0
+mod r0, r1
+exit
+-- result
+0x1
diff --git a/vm/ubpf_jit_x86_64.c b/vm/ubpf_jit_x86_64.c
@@ -35,7 +35,7 @@
 #define TARGET_PC_EXIT -1
 #define TARGET_PC_DIV_BY_ZERO -2
 
-static void muldivmod(struct jit_state *state, uint16_t pc, uint8_t opcode, int src, int dst, int32_t imm);
+static void muldivmod(struct jit_state *state, uint8_t opcode, int src, int dst, int32_t imm);
 
 #define REGISTER_MAP_SIZE 11
 
@@ -174,7 +174,7 @@ translate(struct ubpf_vm *vm, struct jit_state *state, char **errmsg)
         case EBPF_OP_DIV_REG:
         case EBPF_OP_MOD_IMM:
         case EBPF_OP_MOD_REG:
-            muldivmod(state, i, inst.opcode, src, dst, inst.imm);
+            muldivmod(state, inst.opcode, src, dst, inst.imm);
             break;
         case EBPF_OP_OR_IMM:
             emit_alu32_imm32(state, 0x81, 1, dst, inst.imm);
@@ -261,7 +261,7 @@ translate(struct ubpf_vm *vm, struct jit_state *state, char **errmsg)
         case EBPF_OP_DIV64_REG:
         case EBPF_OP_MOD64_IMM:
         case EBPF_OP_MOD64_REG:
-            muldivmod(state, i, inst.opcode, src, dst, inst.imm);
+            muldivmod(state, inst.opcode, src, dst, inst.imm);
             break;
         case EBPF_OP_OR64_IMM:
             emit_alu64_imm32(state, 0x81, 1, dst, inst.imm);
@@ -515,42 +515,74 @@ translate(struct ubpf_vm *vm, struct jit_state *state, char **errmsg)
 }
 
 static void
-muldivmod(struct jit_state *state, uint16_t pc, uint8_t opcode, int src, int dst, int32_t imm)
+muldivmod(struct jit_state *state, uint8_t opcode, int src, int dst, int32_t imm)
 {
     bool mul = (opcode & EBPF_ALU_OP_MASK) == (EBPF_OP_MUL_IMM & EBPF_ALU_OP_MASK);
     bool div = (opcode & EBPF_ALU_OP_MASK) == (EBPF_OP_DIV_IMM & EBPF_ALU_OP_MASK);
     bool mod = (opcode & EBPF_ALU_OP_MASK) == (EBPF_OP_MOD_IMM & EBPF_ALU_OP_MASK);
     bool is64 = (opcode & EBPF_CLS_MASK) == EBPF_CLS_ALU64;
+    bool reg = (opcode & EBPF_SRC_REG) == EBPF_SRC_REG;
 
-    if (div || mod) {
-        emit_load_imm(state, RCX, pc);
-
-        /* test src,src */
-        if (is64) {
-            emit_alu64(state, 0x85, src, src);
+    // Short circuit for imm == 0.
+    if (!reg && imm == 0) {
+        if (div || mul) {
+            // For division and multiplication, set result to zero.
+            emit_alu32(state, 0x31, dst, dst);
         } else {
-            emit_alu32(state, 0x85, src, src);
+            // For modulo, set result to dividend.
+            emit_mov(state, dst, dst);
         }
-
-        /* jz div_by_zero */
-        emit_jcc(state, 0x84, TARGET_PC_DIV_BY_ZERO);
+        return;
     }
 
     if (dst != RAX) {
         emit_push(state, RAX);
     }
+
     if (dst != RDX) {
         emit_push(state, RDX);
     }
+
+    // Load the divisor into RCX.
     if (imm) {
         emit_load_imm(state, RCX, imm);
     } else {
         emit_mov(state, src, RCX);
     }
 
+    // Load the dividend into RAX.
     emit_mov(state, dst, RAX);
 
+    // BPF has two different semantics for division and modulus. For division
+    // if the divisor is zero, the result is zero.  For modulus, if the divisor
+    // is zero, the result is the dividend. To handle this we set the divisor
+    // to 1 if it is zero and then set the result to zero if the divisor was
+    // zero (for division) or set the result to the dividend if the divisor was
+    // zero (for modulo).
+
     if (div || mod) {
+        // Check if divisor is zero.
+        if (is64) {
+            emit_alu64(state, 0x85, RCX, RCX);
+        } else {
+            emit_alu32(state, 0x85, RCX, RCX);
+        }
+
+        // Save the dividend for the modulo case.
+        if (mod) {
+            emit_push(state, RAX);  // Save dividend.
+        }
+
+        // Save the result of the test.
+        emit1(state, 0x9c); /* pushfq */
+
+        // Set the divisor to 1 if it is zero.
+        emit_load_imm(state, RDX, 1);
+        emit1(state, 0x48);
+        emit1(state, 0x0f);
+        emit1(state, 0x44);
+        emit1(state, 0xca); /* cmove rcx,rdx */
+
         /* xor %edx,%edx */
         emit_alu32(state, 0x31, RDX, RDX);
     }
@@ -559,9 +591,39 @@ muldivmod(struct jit_state *state, uint16_t pc, uint8_t opcode, int src, int dst
         emit_rex(state, 1, 0, 0, 0);
     }
 
-    /* mul %ecx or div %ecx */
+    // Multiply or divide.
     emit_alu32(state, 0xf7, mul ? 4 : 6, RCX);
 
+    // Division operation stores the remainder in RDX and the quotient in RAX.
+    if (div || mod) {
+        // Restore the result of the test.
+        emit1(state, 0x9d); /* popfq */
+
+        // If zero flag is set, then the divisor was zero.
+
+        if (div) {
+            // Set the dividend to zero if the divisor was zero.
+            emit_load_imm(state, RCX, 0);
+
+            // Store 0 in RAX if the divisor was zero.
+            // Use conditional move to avoid a branch.
+            emit1(state, 0x48);
+            emit1(state, 0x0f);
+            emit1(state, 0x44);
+            emit1(state, 0xc1); /* cmove rax,rcx */
+        } else {
+            // Restore dividend to RCX.
+            emit_pop(state, RCX);
+
+            // Store the dividend in RAX if the divisor was zero.
+            // Use conditional move to avoid a branch.
+            emit1(state, 0x48);
+            emit1(state, 0x0f);
+            emit1(state, 0x44);
+            emit1(state, 0xd1); /* cmove rdx,rcx */
+        }
+    }
+
     if (dst != RDX) {
         if (mod) {
             emit_mov(state, RDX, dst);

diff --git a/vm/ubpf_vm.c b/vm/ubpf_vm.c
@@ -238,15 +238,11 @@ ubpf_exec(const struct ubpf_vm *vm, void *mem, size_t mem_len, uint64_t* bpf_ret
             reg[inst.dst] &= UINT32_MAX;
             break;
         case EBPF_OP_DIV_IMM:
-            reg[inst.dst] = u32(reg[inst.dst]) / u32(inst.imm);
+            reg[inst.dst] = u32(inst.imm) ? u32(reg[inst.dst]) / u32(inst.imm) : 0;
             reg[inst.dst] &= UINT32_MAX;
             break;
         case EBPF_OP_DIV_REG:
-            if (reg[inst.src] == 0) {
-                vm->error_printf(stderr, ubpf_string_table[UBPF_STRING_ID_DIVIDE_BY_ZERO], cur_pc);
-                return -1;
-            }
-            reg[inst.dst] = u32(reg[inst.dst]) / u32(reg[inst.src]);
+            reg[inst.dst] = reg[inst.src] ? u32(reg[inst.dst]) / u32(reg[inst.src]) : 0;
             reg[inst.dst] &= UINT32_MAX;
             break;
         case EBPF_OP_OR_IMM:
@@ -286,15 +282,11 @@ ubpf_exec(const struct ubpf_vm *vm, void *mem, size_t mem_len, uint64_t* bpf_ret
             reg[inst.dst] &= UINT32_MAX;
             break;
         case EBPF_OP_MOD_IMM:
-            reg[inst.dst] = u32(reg[inst.dst]) % u32(inst.imm);
+            reg[inst.dst] = u32(inst.imm) ? u32(reg[inst.dst]) % u32(inst.imm) : u32(reg[inst.dst]);
             reg[inst.dst] &= UINT32_MAX;
             break;
         case EBPF_OP_MOD_REG:
-            if (reg[inst.src] == 0) {
-                vm->error_printf(stderr, ubpf_string_table[UBPF_STRING_ID_DIVIDE_BY_ZERO], cur_pc);
-                return -1;
-            }
-            reg[inst.dst] = u32(reg[inst.dst]) % u32(reg[inst.src]);
+            reg[inst.dst] = u32(reg[inst.src]) ? u32(reg[inst.dst]) % u32(reg[inst.src]) : u32(reg[inst.dst]);
             break;
         case EBPF_OP_XOR_IMM:
             reg[inst.dst] ^= inst.imm;
@@ -360,14 +352,10 @@ ubpf_exec(const struct ubpf_vm *vm, void *mem, size_t mem_len, uint64_t* bpf_ret
             reg[inst.dst] *= reg[inst.src];
             break;
         case EBPF_OP_DIV64_IMM:
-            reg[inst.dst] /= inst.imm;
+            reg[inst.dst] = inst.imm ? reg[inst.dst] / inst.imm : 0;
             break;
         case EBPF_OP_DIV64_REG:
-            if (reg[inst.src] == 0) {
-                vm->error_printf(stderr, ubpf_string_table[UBPF_STRING_ID_DIVIDE_BY_ZERO], cur_pc);
-                return -1;
-            }
-            reg[inst.dst] /= reg[inst.src];
+            reg[inst.dst] = reg[inst.src] ? reg[inst.dst] / reg[inst.src] : 0;
             break;
         case EBPF_OP_OR64_IMM:
             reg[inst.dst] |= inst.imm;
@@ -397,14 +385,10 @@ ubpf_exec(const struct ubpf_vm *vm, void *mem, size_t mem_len, uint64_t* bpf_ret
             reg[inst.dst] = -reg[inst.dst];
             break;
         case EBPF_OP_MOD64_IMM:
-            reg[inst.dst] %= inst.imm;
+            reg[inst.dst] = inst.imm ? reg[inst.dst] % inst.imm : reg[inst.dst];
             break;
         case EBPF_OP_MOD64_REG:
-            if (reg[inst.src] == 0) {
-                vm->error_printf(stderr, ubpf_string_table[UBPF_STRING_ID_DIVIDE_BY_ZERO], cur_pc);
-                return -1;
-            }
-            reg[inst.dst] %= reg[inst.src];
+            reg[inst.dst] = reg[inst.src] ? reg[inst.dst] % reg[inst.src] : reg[inst.dst];
             break;
         case EBPF_OP_XOR64_IMM:
             reg[inst.dst] ^= inst.imm;
@@ -778,10 +762,6 @@ validate(const struct ubpf_vm *vm, const struct ebpf_inst *insts, uint32_t num_i
         case EBPF_OP_MOD_IMM:
         case EBPF_OP_DIV64_IMM:
         case EBPF_OP_MOD64_IMM:
-            if (inst.imm == 0) {
-                *errmsg = ubpf_error("division by zero at PC %d", i);
-                return false;
-            }
             break;
 
         default: