Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attempt to divide by zero in DIV32 of interpreter #96

Closed
pcy190 opened this issue Jan 3, 2024 · 0 comments · Fixed by #97
Closed

Attempt to divide by zero in DIV32 of interpreter #96

pcy190 opened this issue Jan 3, 2024 · 0 comments · Fixed by #97
Labels

Comments

@pcy190
Copy link
Contributor

pcy190 commented Jan 3, 2024

Details

In the implementation of DIV32, it checks whether the 64-bit source value is zero. However, during the division computation, the source is casted to 32-bit, which could still be zero, e.g., 0xffff000000000000. According to the kernel ebpf standard, we should check the division by zero by checking the nullness of the 32-bit source value.

https://github.com/torvalds/linux/blob/610a9b8f49fbcf1100716370d3b5f6f884a2835a/Documentation/bpf/standardization/instruction-set.rst?plain=1#L246-L251

If BPF program execution would result in division by zero, the destination register is instead set to zero.

The current implementation of DIV32_IMM and DIV32_REG lacks the check to the lower 32 bits of source value.

rbpf/src/interpreter.rs

Lines 217 to 220 in 4812c52

ebpf::DIV32_IMM if insn.imm == 0 => reg[_dst] = 0,
ebpf::DIV32_IMM => reg[_dst] = (reg[_dst] as u32 / insn.imm as u32) as u64,
ebpf::DIV32_REG if reg[_src] == 0 => reg[_dst] = 0,
ebpf::DIV32_REG => reg[_dst] = (reg[_dst] as u32 / reg[_src] as u32) as u64,

The following PoC program would cause the attempt to calculate the remainder with a divisor of zero panic in the interpreter:

lddw r1, 0x100000000
div32 r0, r1
exit

Result:

attempt to divide by zero
thread '' panicked at 'attempt to divide by zero', src/interpreter.rs:222:45
stack backtrace:
...
   3: rbpf::interpreter::execute_program
             at ./src/interpreter.rs:222:45
   4: rbpf::EbpfVmMbuff::execute_program
             at ./src/lib.rs:300:9
   5: rbpf::EbpfVmRaw::execute_program
             at ./src/lib.rs:1221:9

The expected result is that the program executed with r0 set to zero, without panic/error.

Suggestion

Check nullness of 32-bit casted source value before division

qmonnet added a commit to pcy190/rbpf that referenced this issue Jan 4, 2024
We had an issue in the interpreter, because for 32-bit div and mod we
would check that the whole (64-bit long) divisor was not 0, but then
we'd only use the last 32 bits of this divisor to perform the arithmetic
operation. We fixed the bug, but let's add a non-regression test to
catch this in the future.

Link: qmonnet#95
Link: qmonnet#96
Signed-off-by: Quentin Monnet <quentin@isovalent.com>
@qmonnet qmonnet added the bug label Jan 4, 2024
pcy190 pushed a commit to pcy190/rbpf that referenced this issue Jan 5, 2024
We had an issue in the interpreter, because for 32-bit div and mod we
would check that the whole (64-bit long) divisor was not 0, but then
we'd only use the last 32 bits of this divisor to perform the arithmetic
operation. We fixed the bug, but let's add a non-regression test to
catch this in the future.

Link: qmonnet#95
Link: qmonnet#96
Signed-off-by: Quentin Monnet <quentin@isovalent.com>
Signed-off-by: Richard Smith <ret2happy@126.com>
qmonnet added a commit that referenced this issue Jan 5, 2024
We had an issue in the interpreter, because for 32-bit div and mod we
would check that the whole (64-bit long) divisor was not 0, but then
we'd only use the last 32 bits of this divisor to perform the arithmetic
operation. We fixed the bug, but let's add a non-regression test to
catch this in the future.

Link: #95
Link: #96
Signed-off-by: Quentin Monnet <quentin@isovalent.com>
Signed-off-by: Richard Smith <ret2happy@126.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants