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.

Sign up for GitHub

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

Fix behavior around jumps with immediate values > 0x7fffffff in the interpreter #447

Merged
merged 5 commits into from
May 8, 2024
Merged

Fix behavior around jumps with immediate values > 0x7fffffff in the interpreter #447

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f5810b1
Add test for sign extension in unsigned jump
May 7, 2024
93e06df
Fix interpreter to match jit behavior
May 7, 2024
0dcf196
Make sign extension explicit
May 7, 2024
0854ecd
PR feedback
May 7, 2024
8b003aa
Merge branch 'main' into unsigned_jump_test
Alan-Jowett May 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion external/bpf_conformance
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these be a separate commit?

Submodule bpf_conformance updated 8 files
+1 −1 .github/workflows/Build.yml
+1 −1 .github/workflows/CICD.yml
+4 −4 .github/workflows/codeql-analysis.yml
+1 −1 .github/workflows/dependency-review.yml
+2 −2 .github/workflows/scorecards.yml
+1 −1 Docker/Linux/Dockerfile
+3 −3 src/bpf_test_parser.cc
+25 −0 tests/j-signed-imm.data
33 changes: 22 additions & 11 deletions vm/ubpf_vm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,17 @@ i32(uint64_t x)
return x;
}

/**
* @brief Sign extend immediate value to a signed 64-bit value.
*
* @param[in] immediate The signed 32-bit immediate value to sign extend.
* @return The sign extended 64-bit value.
*/
static int64_t sign_extend_immediate(int32_t immediate) {
return (int64_t)immediate;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am currently working through whether or not this is guaranteed to give the proper behavior according to the C standard. In newer versions of the C standard, twos complement representation of signed integers is specified. In older versions, it is not. Therefore, we will (I think) need to be very careful about this. See, e.g., https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2218.htm.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Further research:

  1. GCC represents integers as twos complement: https://gcc.gnu.org/onlinedocs/gcc-5.3.0/gcc/Integers-implementation.html#Integers-implementation
  2. MSVC represents integers as twos complement: https://learn.microsoft.com/en-us/cpp/c-language/range-of-integer-values?view=msvc-170

There does not appear to be a place where Clang (llvm) defines its implementation-specific behavior. See llvm/llvm-project#11644 for the status of such a document.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the yet-to-be standardized version of C, twos complement is specified as the representation to use for signed integers. See https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3220.pdf


}

#define IS_ALIGNED(x, a) (((uintptr_t)(x) & ((a)-1)) == 0)

inline static uint64_t
Expand Down Expand Up @@ -685,7 +696,7 @@ ubpf_exec(const struct ubpf_vm* vm, void* mem, size_t mem_len, uint64_t* bpf_ret
pc += inst.offset;
break;
case EBPF_OP_JEQ_IMM:
if (reg[inst.dst] == inst.imm) {
if (reg[inst.dst] == (uint64_t)sign_extend_immediate(inst.imm)) {
hawkinsw marked this conversation as resolved.
Show resolved Hide resolved
pc += inst.offset;
}
break;
Expand All @@ -705,7 +716,7 @@ ubpf_exec(const struct ubpf_vm* vm, void* mem, size_t mem_len, uint64_t* bpf_ret
}
break;
case EBPF_OP_JGT_IMM:
if (reg[inst.dst] > u32(inst.imm)) {
if (reg[inst.dst] > (uint64_t)sign_extend_immediate(inst.imm)) {
pc += inst.offset;
}
break;
Expand All @@ -725,7 +736,7 @@ ubpf_exec(const struct ubpf_vm* vm, void* mem, size_t mem_len, uint64_t* bpf_ret
}
break;
case EBPF_OP_JGE_IMM:
if (reg[inst.dst] >= u32(inst.imm)) {
if (reg[inst.dst] >= (uint64_t)sign_extend_immediate(inst.imm)) {
pc += inst.offset;
}
break;
Expand All @@ -745,7 +756,7 @@ ubpf_exec(const struct ubpf_vm* vm, void* mem, size_t mem_len, uint64_t* bpf_ret
}
break;
case EBPF_OP_JLT_IMM:
if (reg[inst.dst] < u32(inst.imm)) {
if (reg[inst.dst] < (uint64_t)sign_extend_immediate(inst.imm)) {
pc += inst.offset;
}
break;
Expand All @@ -765,7 +776,7 @@ ubpf_exec(const struct ubpf_vm* vm, void* mem, size_t mem_len, uint64_t* bpf_ret
}
break;
case EBPF_OP_JLE_IMM:
if (reg[inst.dst] <= u32(inst.imm)) {
if (reg[inst.dst] <= (uint64_t)sign_extend_immediate(inst.imm)) {
pc += inst.offset;
}
break;
Expand All @@ -785,7 +796,7 @@ ubpf_exec(const struct ubpf_vm* vm, void* mem, size_t mem_len, uint64_t* bpf_ret
}
break;
case EBPF_OP_JSET_IMM:
if (reg[inst.dst] & inst.imm) {
if (reg[inst.dst] & (uint64_t)sign_extend_immediate(inst.imm)) {
pc += inst.offset;
}
break;
Expand All @@ -805,7 +816,7 @@ ubpf_exec(const struct ubpf_vm* vm, void* mem, size_t mem_len, uint64_t* bpf_ret
}
break;
case EBPF_OP_JNE_IMM:
if (reg[inst.dst] != inst.imm) {
if (reg[inst.dst] != (uint64_t)sign_extend_immediate(inst.imm)) {
pc += inst.offset;
}
break;
Expand All @@ -825,7 +836,7 @@ ubpf_exec(const struct ubpf_vm* vm, void* mem, size_t mem_len, uint64_t* bpf_ret
}
break;
case EBPF_OP_JSGT_IMM:
if ((int64_t)reg[inst.dst] > inst.imm) {
if ((int64_t)reg[inst.dst] > sign_extend_immediate(inst.imm)) {
pc += inst.offset;
}
break;
Expand All @@ -845,7 +856,7 @@ ubpf_exec(const struct ubpf_vm* vm, void* mem, size_t mem_len, uint64_t* bpf_ret
}
break;
case EBPF_OP_JSGE_IMM:
if ((int64_t)reg[inst.dst] >= inst.imm) {
if ((int64_t)reg[inst.dst] >= sign_extend_immediate(inst.imm)) {
pc += inst.offset;
}
break;
Expand All @@ -865,7 +876,7 @@ ubpf_exec(const struct ubpf_vm* vm, void* mem, size_t mem_len, uint64_t* bpf_ret
}
break;
case EBPF_OP_JSLT_IMM:
if ((int64_t)reg[inst.dst] < inst.imm) {
if ((int64_t)reg[inst.dst] < sign_extend_immediate(inst.imm)) {
pc += inst.offset;
}
break;
Expand All @@ -885,7 +896,7 @@ ubpf_exec(const struct ubpf_vm* vm, void* mem, size_t mem_len, uint64_t* bpf_ret
}
break;
case EBPF_OP_JSLE_IMM:
if ((int64_t)reg[inst.dst] <= inst.imm) {
if ((int64_t)reg[inst.dst] <= sign_extend_immediate(inst.imm)) {
pc += inst.offset;
}
break;
Expand Down
Loading