From 57139e9e1fca4f01155f7d99bc55cdcc25b0bc04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Thu, 12 Sep 2024 23:02:55 +0200 Subject: [PATCH] Fix - `ExceededMaxInstructions` final PC (#593) * Removes unused immediate. * Moves instruction meter validation first in internal function calls. * Splits emit_validate_and_profile_instruction_count in exit instruction. * Fixes chaotic final PC of ExceededMaxInstructions. * Removes exclusive parameter emit_validate_and_profile_instruction_count. --- src/interpreter.rs | 5 ----- src/jit.rs | 35 +++++++++++++++++++---------------- 2 files changed, 19 insertions(+), 21 deletions(-) diff --git a/src/interpreter.rs b/src/interpreter.rs index 19aec1ab..8f3951e4 100644 --- a/src/interpreter.rs +++ b/src/interpreter.rs @@ -527,11 +527,6 @@ impl<'a, 'b, C: ContextObject> Interpreter<'a, 'b, C> { _ => throw_error!(self, EbpfError::UnsupportedInstruction), } - if config.enable_instruction_meter && self.vm.due_insn_count >= self.vm.previous_instruction_meter { - self.reg[11] += 1; - throw_error!(self, EbpfError::ExceededMaxInstructions); - } - self.reg[11] = next_pc; true } diff --git a/src/jit.rs b/src/jit.rs index 395c3882..e632f508 100644 --- a/src/jit.rs +++ b/src/jit.rs @@ -427,7 +427,7 @@ impl<'a, C: ContextObject> JitCompiler<'a, C> { } ebpf::LD_DW_IMM if self.executable.get_sbpf_version().enable_lddw() => { - self.emit_validate_and_profile_instruction_count(true, false, Some(self.pc + 2)); + self.emit_validate_and_profile_instruction_count(false, Some(self.pc + 2)); self.pc += 1; self.result.pc_section[self.pc] = self.anchors[ANCHOR_CALL_UNSUPPORTED_INSTRUCTION] as usize; ebpf::augment_lddw_unchecked(self.program, &mut insn); @@ -628,7 +628,7 @@ impl<'a, C: ContextObject> JitCompiler<'a, C> { // BPF_JMP class ebpf::JA => { - self.emit_validate_and_profile_instruction_count(false, true, Some(target_pc)); + self.emit_validate_and_profile_instruction_count(true, Some(target_pc)); self.emit_ins(X86Instruction::load_immediate(OperandSize::S64, REGISTER_SCRATCH, target_pc as i64)); let jump_offset = self.relative_to_target_pc(target_pc, 5); self.emit_ins(X86Instruction::jump_immediate(jump_offset)); @@ -667,7 +667,7 @@ impl<'a, C: ContextObject> JitCompiler<'a, C> { if external { if let Some((_function_name, function)) = self.executable.get_loader().get_function_registry().lookup_by_key(insn.imm as u32) { - self.emit_validate_and_profile_instruction_count(true, false, Some(0)); + self.emit_validate_and_profile_instruction_count(false, Some(0)); self.emit_ins(X86Instruction::load_immediate(OperandSize::S64, REGISTER_SCRATCH, function as usize as i64)); self.emit_ins(X86Instruction::call_immediate(self.relative_to_anchor(ANCHOR_EXTERNAL_FUNCTION_CALL, 5))); self.emit_undo_profile_instruction_count(0); @@ -696,6 +696,8 @@ impl<'a, C: ContextObject> JitCompiler<'a, C> { self.emit_internal_call(Value::Register(target_pc)); }, ebpf::EXIT => { + self.emit_validate_instruction_count(true, Some(self.pc)); + let call_depth_access = X86IndirectAccess::Offset(self.slot_in_vm(RuntimeEnvironmentSlot::CallDepth)); self.emit_ins(X86Instruction::load(OperandSize::S64, REGISTER_PTR_TO_VM, REGISTER_MAP[FRAME_PTR_REG], call_depth_access)); @@ -718,7 +720,7 @@ impl<'a, C: ContextObject> JitCompiler<'a, C> { } // and return - self.emit_validate_and_profile_instruction_count(false, false, Some(0)); + self.emit_profile_instruction_count(false, Some(0)); self.emit_ins(X86Instruction::return_near()); }, @@ -732,7 +734,7 @@ impl<'a, C: ContextObject> JitCompiler<'a, C> { if self.offset_in_text_section + MAX_MACHINE_CODE_LENGTH_PER_INSTRUCTION * 2 >= self.result.text_section.len() { return Err(EbpfError::ExhaustedTextSegment(self.pc)); } - self.emit_validate_and_profile_instruction_count(true, false, Some(self.pc + 1)); + self.emit_validate_and_profile_instruction_count(false, Some(self.pc + 1)); self.emit_ins(X86Instruction::load_immediate(OperandSize::S64, REGISTER_SCRATCH, self.pc as i64)); // Save pc self.emit_set_exception_kind(EbpfError::ExecutionOverrun); self.emit_ins(X86Instruction::jump_immediate(self.relative_to_anchor(ANCHOR_THROW_EXCEPTION, 5))); @@ -895,6 +897,9 @@ impl<'a, C: ContextObject> JitCompiler<'a, C> { #[inline] fn emit_profile_instruction_count(&mut self, user_provided: bool, target_pc: Option) { + if !self.config.enable_instruction_meter { + return; + } match target_pc { Some(target_pc) => { // instruction_meter += target_pc - (self.pc + 1); @@ -907,17 +912,15 @@ impl<'a, C: ContextObject> JitCompiler<'a, C> { }, None => { self.emit_ins(X86Instruction::alu(OperandSize::S64, 0x81, 5, REGISTER_INSTRUCTION_METER, self.pc as i64 + 1, None)); // instruction_meter -= self.pc + 1; - self.emit_ins(X86Instruction::alu(OperandSize::S64, 0x01, REGISTER_SCRATCH, REGISTER_INSTRUCTION_METER, self.pc as i64, None)); // instruction_meter += target_pc; + self.emit_ins(X86Instruction::alu(OperandSize::S64, 0x01, REGISTER_SCRATCH, REGISTER_INSTRUCTION_METER, 0, None)); // instruction_meter += target_pc; }, } } #[inline] - fn emit_validate_and_profile_instruction_count(&mut self, exclusive: bool, user_provided: bool, target_pc: Option) { - if self.config.enable_instruction_meter { - self.emit_validate_instruction_count(exclusive, Some(self.pc)); - self.emit_profile_instruction_count(user_provided, target_pc); - } + fn emit_validate_and_profile_instruction_count(&mut self, user_provided: bool, target_pc: Option) { + self.emit_validate_instruction_count(true, Some(self.pc)); + self.emit_profile_instruction_count(user_provided, target_pc); } #[inline] @@ -1037,6 +1040,7 @@ impl<'a, C: ContextObject> JitCompiler<'a, C> { // Store PC in case the bounds check fails self.emit_ins(X86Instruction::load_immediate(OperandSize::S64, REGISTER_SCRATCH, self.pc as i64)); + self.emit_validate_instruction_count(true, Some(self.pc)); self.emit_ins(X86Instruction::call_immediate(self.relative_to_anchor(ANCHOR_ANCHOR_INTERNAL_FUNCTION_CALL_PROLOGUE, 5))); match dst { @@ -1049,14 +1053,14 @@ impl<'a, C: ContextObject> JitCompiler<'a, C> { self.emit_ins(X86Instruction::call_immediate(self.relative_to_anchor(ANCHOR_ANCHOR_INTERNAL_FUNCTION_CALL_REG, 5))); - self.emit_validate_and_profile_instruction_count(false, false, None); + self.emit_profile_instruction_count(false, None); self.emit_ins(X86Instruction::mov(OperandSize::S64, REGISTER_MAP[0], REGISTER_OTHER_SCRATCH)); self.emit_ins(X86Instruction::pop(REGISTER_MAP[0])); // Restore RAX self.emit_ins(X86Instruction::call_reg(REGISTER_OTHER_SCRATCH, None)); // callq *REGISTER_OTHER_SCRATCH }, Value::Constant64(target_pc, user_provided) => { debug_assert!(user_provided); - self.emit_validate_and_profile_instruction_count(false, user_provided, Some(target_pc as usize)); + self.emit_profile_instruction_count(user_provided, Some(target_pc as usize)); if user_provided && self.should_sanitize_constant(target_pc) { self.emit_sanitized_load_immediate(OperandSize::S64, REGISTER_SCRATCH, target_pc); } else { @@ -1149,7 +1153,7 @@ impl<'a, C: ContextObject> JitCompiler<'a, C> { #[inline] fn emit_conditional_branch_reg(&mut self, op: u8, bitwise: bool, first_operand: u8, second_operand: u8, target_pc: usize) { - self.emit_validate_and_profile_instruction_count(false, true, Some(target_pc)); + self.emit_validate_and_profile_instruction_count(true, Some(target_pc)); if bitwise { // Logical self.emit_ins(X86Instruction::test(OperandSize::S64, first_operand, second_operand, None)); } else { // Arithmetic @@ -1163,7 +1167,7 @@ impl<'a, C: ContextObject> JitCompiler<'a, C> { #[inline] fn emit_conditional_branch_imm(&mut self, op: u8, bitwise: bool, immediate: i64, second_operand: u8, target_pc: usize) { - self.emit_validate_and_profile_instruction_count(false, true, Some(target_pc)); + self.emit_validate_and_profile_instruction_count(true, Some(target_pc)); if self.should_sanitize_constant(immediate) { self.emit_sanitized_load_immediate(OperandSize::S64, REGISTER_SCRATCH, immediate); if bitwise { // Logical @@ -1379,7 +1383,6 @@ impl<'a, C: ContextObject> JitCompiler<'a, C> { // Quit gracefully self.set_anchor(ANCHOR_EXIT); - self.emit_validate_instruction_count(false, None); self.emit_ins(X86Instruction::lea(OperandSize::S64, REGISTER_PTR_TO_VM, REGISTER_OTHER_SCRATCH, Some(X86IndirectAccess::Offset(self.slot_in_vm(RuntimeEnvironmentSlot::ProgramResult))))); self.emit_ins(X86Instruction::store(OperandSize::S64, REGISTER_MAP[0], REGISTER_OTHER_SCRATCH, X86IndirectAccess::Offset(std::mem::size_of::() as i32))); // result.return_value = R0; self.emit_ins(X86Instruction::load_immediate(OperandSize::S64, REGISTER_MAP[0], 0));