From d2a33c28db73951a60d781db860d64c80447fe22 Mon Sep 17 00:00:00 2001 From: Will Hawkins Date: Wed, 26 Jun 2024 14:36:29 -0400 Subject: [PATCH] Rationalize parameters to helper functions (#515) The external helper functions should get 6 parameters no matter whether the eBPF program is interpeted or JIT'd. The 6 parameter, the one in addition to the 5 specified by the eBPF standard, is the context parameter. Add tests to make sure that the 6th parameter is properly passed to the external helper functions. Signed-off-by: Will Hawkins --- ...st_default_dispatcher_helper_context.input | 1 + ..._test_default_dispatcher_helper_context.md | 4 + ..._test_default_dispatcher_helper_context.cc | 81 +++++++++++++++++++ vm/inc/ubpf.h | 14 +++- vm/ubpf_int.h | 33 ++++++-- vm/ubpf_jit.c | 7 +- vm/ubpf_jit_arm64.c | 37 ++++++--- vm/ubpf_jit_x86_64.c | 9 ++- vm/ubpf_jit_x86_64.h | 4 +- vm/ubpf_vm.c | 26 +++--- 10 files changed, 174 insertions(+), 42 deletions(-) create mode 100644 custom_tests/data/ubpf_test_default_dispatcher_helper_context.input create mode 100644 custom_tests/descrs/ubpf_test_default_dispatcher_helper_context.md create mode 100644 custom_tests/srcs/ubpf_test_default_dispatcher_helper_context.cc diff --git a/custom_tests/data/ubpf_test_default_dispatcher_helper_context.input b/custom_tests/data/ubpf_test_default_dispatcher_helper_context.input new file mode 100644 index 000000000..cd45e5965 --- /dev/null +++ b/custom_tests/data/ubpf_test_default_dispatcher_helper_context.input @@ -0,0 +1 @@ +85 00 00 00 01 00 00 00 95 00 00 00 00 00 00 00 diff --git a/custom_tests/descrs/ubpf_test_default_dispatcher_helper_context.md b/custom_tests/descrs/ubpf_test_default_dispatcher_helper_context.md new file mode 100644 index 000000000..968c8de6e --- /dev/null +++ b/custom_tests/descrs/ubpf_test_default_dispatcher_helper_context.md @@ -0,0 +1,4 @@ +##Test Description + + This custom test program tests whether JIT'd and interpreted eBPF programs properly pass the context to external + helper functions. diff --git a/custom_tests/srcs/ubpf_test_default_dispatcher_helper_context.cc b/custom_tests/srcs/ubpf_test_default_dispatcher_helper_context.cc new file mode 100644 index 000000000..382b8789b --- /dev/null +++ b/custom_tests/srcs/ubpf_test_default_dispatcher_helper_context.cc @@ -0,0 +1,81 @@ +// Copyright (c) Will Hawkins +// SPDX-License-Identifier: Apache-2.0 + +#include +#include +#include +#include +#include + +extern "C" +{ +#include "ubpf.h" +} + +#include "ubpf_custom_test_support.h" + +uint64_t +simple_helper(uint64_t p0, uint64_t p1, uint64_t p2, uint64_t p3, uint64_t p4, void* cookie) +{ + UNREFERENCED_PARAMETER(p0); + UNREFERENCED_PARAMETER(p1); + UNREFERENCED_PARAMETER(p2); + UNREFERENCED_PARAMETER(p3); + UNREFERENCED_PARAMETER(p4); + uint64_t* ccookie = (uint64_t*)cookie; + return *ccookie; +} + +int +main(int argc, char** argv) +{ + std::string program_string{}; + std::string error{}; + ubpf_jit_fn jit_fn; + uint64_t memory{0x123456789}; + + // The test program invokes an external function at index 1 (which is invoked via the + // default external helper dispatcher). The result of that external function is given as the + // result of the eBPF's program execution. Therefore, ... + if (!get_program_string(argc, argv, program_string, error)) { + std::cerr << error << std::endl; + return 1; + } + + std::unique_ptr vm(ubpf_create(), ubpf_destroy); + if (!ubpf_setup_custom_test( + vm, + program_string, + [](ubpf_vm_up& vm, std::string& error) { + if (ubpf_register(vm.get(), 1, "simple helper", as_external_function_t((void*)simple_helper)) < 0) { + error = "Failed to register external helper function at index 1."; + return false; + } + return true; + }, + jit_fn, + error)) { + std::cerr << "Problem setting up custom test: " << error << std::endl; + return 1; + } + + [[maybe_unused]] auto result = jit_fn(&memory, sizeof(uint64_t)); + + // ... because of the semantics of external helper functions, the result of the eBPF + // program execution should point to the same place to which &memory points. + if (result != memory) { + std::cerr << "result and memory are not equal (JIT version).\n"; + return 1; + } + + if (ubpf_exec(vm.get(), &memory, sizeof(uint64_t), &result)) { + std::cerr << "There was an error interpreting the test program.\n"; + return 1; + } + + if (result != memory) { + std::cerr << "result and memory are not equal (interpreter version).\n"; + return 1; + } + return 0; +} diff --git a/vm/inc/ubpf.h b/vm/inc/ubpf.h index 554a25d80..760e4fceb 100644 --- a/vm/inc/ubpf.h +++ b/vm/inc/ubpf.h @@ -130,14 +130,20 @@ extern "C" ubpf_set_error_print(struct ubpf_vm* vm, int (*error_printf)(FILE* stream, const char* format, ...)); /** - * @brief The type of an external function. + * @brief The type of an external helper function. + * + * Note: There is an implicit void *-typed 6th parameter that users can access if they choose. That + * sixth parameter's value is the value of the pointer given by the user of \ref ubpf_exec, \ref ubpf_exec_ex, and + * the function generated by \ref ubpf_translate as the first argument. */ typedef uint64_t (*external_function_t)(uint64_t p0, uint64_t p1, uint64_t p2, uint64_t p3, uint64_t p4); /** - * @brief Cast an external function to external_function_t - * Some external functions may not use all the parameters and, therefore, - * not match the external_function_t typedef. Use this for a conversion. + * @brief Cast an external function to \ref external_function_t + * + * Some external functions may not use all the parameters (or may use the implicit + * 6th parameter) and, therefore, not match the \ref external_function_t typedef. + * Use this for a conversion. * * @param[in] f The function to cast to match the signature of an * external function. diff --git a/vm/ubpf_int.h b/vm/ubpf_int.h index 6c27ffcd0..8ec933e58 100644 --- a/vm/ubpf_int.h +++ b/vm/ubpf_int.h @@ -29,7 +29,8 @@ #define UNUSED_PARAMETER(x) ((void)x) struct ebpf_inst; -typedef uint64_t (*ext_func)(uint64_t arg0, uint64_t arg1, uint64_t arg2, uint64_t arg3, uint64_t arg4); +typedef uint64_t (*extended_external_helper_t)( + uint64_t arg0, uint64_t arg1, uint64_t arg2, uint64_t arg3, uint64_t arg4, void* cookie); typedef enum { @@ -63,7 +64,7 @@ struct ubpf_vm size_t jitter_buffer_size; struct ubpf_jit_result jitted_result; - ext_func* ext_funcs; + extended_external_helper_t* ext_funcs; bool* int_funcs; const char** ext_func_names; @@ -85,7 +86,12 @@ struct ubpf_vm size_t size, uint32_t offset); bool (*jit_update_helper)( - struct ubpf_vm* vm, ext_func new_helper, unsigned int idx, uint8_t* buffer, size_t size, uint32_t offset); + struct ubpf_vm* vm, + extended_external_helper_t new_helper, + unsigned int idx, + uint8_t* buffer, + size_t size, + uint32_t offset); int unwind_stack_extension_index; uint64_t pointer_secret; ubpf_data_relocation data_relocation_function; @@ -115,7 +121,12 @@ ubpf_jit_update_dispatcher_arm64( struct ubpf_vm* vm, external_function_dispatcher_t new_dispatcher, uint8_t* buffer, size_t size, uint32_t offset); bool ubpf_jit_update_helper_arm64( - struct ubpf_vm* vm, ext_func new_helper, unsigned int idx, uint8_t* buffer, size_t size, uint32_t offset); + struct ubpf_vm* vm, + extended_external_helper_t new_helper, + unsigned int idx, + uint8_t* buffer, + size_t size, + uint32_t offset); // x86_64 struct ubpf_jit_result @@ -125,7 +136,12 @@ ubpf_jit_update_dispatcher_x86_64( struct ubpf_vm* vm, external_function_dispatcher_t new_dispatcher, uint8_t* buffer, size_t size, uint32_t offset); bool ubpf_jit_update_helper_x86_64( - struct ubpf_vm* vm, ext_func new_helper, unsigned int idx, uint8_t* buffer, size_t size, uint32_t offset); + struct ubpf_vm* vm, + extended_external_helper_t new_helper, + unsigned int idx, + uint8_t* buffer, + size_t size, + uint32_t offset); // uhm, hello? struct ubpf_jit_result @@ -135,7 +151,12 @@ ubpf_jit_update_dispatcher_null( struct ubpf_vm* vm, external_function_dispatcher_t new_dispatcher, uint8_t* buffer, size_t size, uint32_t offset); bool ubpf_jit_update_helper_null( - struct ubpf_vm* vm, ext_func new_helper, unsigned int idx, uint8_t* buffer, size_t size, uint32_t offset); + struct ubpf_vm* vm, + extended_external_helper_t new_helper, + unsigned int idx, + uint8_t* buffer, + size_t size, + uint32_t offset); char* ubpf_error(const char* fmt, ...); diff --git a/vm/ubpf_jit.c b/vm/ubpf_jit.c index 0b9f42396..5e405d1e1 100644 --- a/vm/ubpf_jit.c +++ b/vm/ubpf_jit.c @@ -77,7 +77,12 @@ ubpf_jit_update_dispatcher_null( bool ubpf_jit_update_helper_null( - struct ubpf_vm* vm, ext_func new_helper, unsigned int idx, uint8_t* buffer, size_t size, uint32_t offset) + struct ubpf_vm* vm, + extended_external_helper_t new_helper, + unsigned int idx, + uint8_t* buffer, + size_t size, + uint32_t offset) { UNUSED_PARAMETER(vm); UNUSED_PARAMETER(new_helper); diff --git a/vm/ubpf_jit_arm64.c b/vm/ubpf_jit_arm64.c index 2df17cbee..b9943f32c 100644 --- a/vm/ubpf_jit_arm64.c +++ b/vm/ubpf_jit_arm64.c @@ -603,10 +603,10 @@ emit_dispatched_external_helper_call(struct jit_state* state, struct ubpf_vm* vm // Check whether temp_register is empty. emit_addsub_immediate(state, true, AS_SUBS, temp_register, temp_register, 0); - // Jump if we are ready to roll (because we are using an external dispatcher). - uint32_t jump_source = emit_conditionalbranch_immediate(state, COND_NE, 0); + // Jump to the call if we are ready to roll (because we are using an external dispatcher). + uint32_t external_dispatcher_jump_source = emit_conditionalbranch_immediate(state, COND_NE, 0); - // We are not ready to roll. So, load the helper function address by index. + // We are not ready to roll. In other words, we are going to load the helper function address by index. emit_movewide_immediate(state, true, R5, idx); emit_movewide_immediate(state, true, R6, 3); emit_dataprocessing_twosource(state, true, DP2_LSLV, R5, R5, R6); @@ -616,17 +616,31 @@ emit_dispatched_external_helper_call(struct jit_state* state, struct ubpf_vm* vm emit_addsub_register(state, true, AS_ADD, temp_register, temp_register, R5); emit_loadstore_immediate(state, LS_LDRX, temp_register, temp_register, 0); - // And now we, too, are ready to roll. + // Add the implicit 6th parameter (the context) + emit_logical_register(state, true, LOG_ORR, R5, RZ, VOLATILE_CTXT); - // Both paths meet here where we ... - emit_jump_target(state, jump_source); + // And now we, too, are ready to roll. So, let's jump around the code that sets up the additional + // parameters for the external dispatcher. We will end up at the call site where both paths + // will rendezvous. + uint32_t no_dispatcher_jump_source = emit_unconditionalbranch_immediate(state, UBR_B, 0); - // ... set up the final two parameters. + // Mark the landing spot for the jump around the code that sets up a call to a helper function + // when no external dispatcher is present. + emit_jump_target(state, external_dispatcher_jump_source); + + // ... set up the final two arguments for the external dispatcher. + + // The index of the helper to be invoked. emit_movewide_immediate(state, true, R5, idx); + + // The context. // Use a sneaky way to copy the context register into the R6 register (as the final parameter). emit_logical_register(state, true, LOG_ORR, R6, RZ, VOLATILE_CTXT); - // Now, all that's left is to call! + // Mark the landing spot for the jump around the external-dispatcher-argument-setup code. + emit_jump_target(state, no_dispatcher_jump_source); + + // Both paths meet here -- all that's left is to call! emit_unconditionalbranch_register(state, BR_BLR, temp_register); /* On exit need to move result from r0 to whichever register we've mapped EBPF r0 to. */ @@ -1459,7 +1473,12 @@ ubpf_jit_update_dispatcher_arm64( bool ubpf_jit_update_helper_arm64( - struct ubpf_vm* vm, ext_func new_helper, unsigned int idx, uint8_t* buffer, size_t size, uint32_t offset) + struct ubpf_vm* vm, + extended_external_helper_t new_helper, + unsigned int idx, + uint8_t* buffer, + size_t size, + uint32_t offset) { UNUSED_PARAMETER(vm); uint64_t jit_upper_bound = (uint64_t)buffer + size; diff --git a/vm/ubpf_jit_x86_64.c b/vm/ubpf_jit_x86_64.c index b6f35ec9b..be20d6cc6 100644 --- a/vm/ubpf_jit_x86_64.c +++ b/vm/ubpf_jit_x86_64.c @@ -149,7 +149,7 @@ emit_local_call(struct ubpf_vm* vm, struct jit_state* state, uint32_t target_pc) emit_pop(state, map_register(BPF_REG_7)); emit_pop(state, map_register(BPF_REG_6)); - // Because the top of the host tack holds the stack usage of the currently-executing + // Because the top of the host stack holds the stack usage of the currently-executing // function, we adjust the eBPF base pointer back up by that value! // add r15, [rsp] emit1(state, 0x4c); @@ -1170,7 +1170,12 @@ ubpf_jit_update_dispatcher_x86_64( bool ubpf_jit_update_helper_x86_64( - struct ubpf_vm* vm, ext_func new_helper, unsigned int idx, uint8_t* buffer, size_t size, uint32_t offset) + struct ubpf_vm* vm, + extended_external_helper_t new_helper, + unsigned int idx, + uint8_t* buffer, + size_t size, + uint32_t offset) { UNUSED_PARAMETER(vm); uint64_t jit_upper_bound = (uint64_t)buffer + size; diff --git a/vm/ubpf_jit_x86_64.h b/vm/ubpf_jit_x86_64.h index b0ae1c42d..33dcdecf4 100644 --- a/vm/ubpf_jit_x86_64.h +++ b/vm/ubpf_jit_x86_64.h @@ -554,9 +554,7 @@ emit_dispatched_external_helper_call(struct jit_state* state, unsigned int idx) #endif // jmp call_label - emit1(state, 0xe9); - uint32_t skip_external_dispatcher_source = state->offset; - emit_4byte_offset_placeholder(state); + uint32_t skip_external_dispatcher_source = emit_jmp(state, 0); // External dispatcher: diff --git a/vm/ubpf_vm.c b/vm/ubpf_vm.c index 2836ea342..b68324c33 100644 --- a/vm/ubpf_vm.c +++ b/vm/ubpf_vm.c @@ -73,19 +73,6 @@ ubpf_set_error_print(struct ubpf_vm* vm, int (*error_printf)(FILE* stream, const vm->error_printf = fprintf; } -static uint64_t -ubpf_default_external_dispatcher( - uint64_t arg1, - uint64_t arg2, - uint64_t arg3, - uint64_t arg4, - uint64_t arg5, - unsigned int index, - external_function_t* external_fns) -{ - return external_fns[index](arg1, arg2, arg3, arg4, arg5); -} - struct ubpf_vm* ubpf_create(void) { @@ -158,7 +145,7 @@ ubpf_register(struct ubpf_vm* vm, unsigned int idx, const char* name, external_f return -1; } - vm->ext_funcs[idx] = (ext_func)fn; + vm->ext_funcs[idx] = (extended_external_helper_t)fn; vm->ext_func_names[idx] = name; int success = 0; @@ -170,7 +157,12 @@ ubpf_register(struct ubpf_vm* vm, unsigned int idx, const char* name, external_f // Now, update! if (!vm->jit_update_helper( - vm, fn, idx, (uint8_t*)vm->jitted, vm->jitted_size, vm->jitted_result.external_helper_offset)) { + vm, + (extended_external_helper_t)fn, + idx, + (uint8_t*)vm->jitted, + vm->jitted_size, + vm->jitted_result.external_helper_offset)) { // Can't immediately stop here because we have unprotected memory! success = -1; } @@ -1229,8 +1221,8 @@ ubpf_exec_ex( reg[0] = vm->dispatcher(reg[1], reg[2], reg[3], reg[4], reg[5], inst.imm, external_dispatcher_cookie); } else { - reg[0] = ubpf_default_external_dispatcher( - reg[1], reg[2], reg[3], reg[4], reg[5], inst.imm, vm->ext_funcs); + reg[0] = + vm->ext_funcs[inst.imm](reg[1], reg[2], reg[3], reg[4], reg[5], external_dispatcher_cookie); } if (inst.imm == vm->unwind_stack_extension_index && reg[0] == 0) { *bpf_return_value = reg[0];