From db2213671e1972e6b67b2220010cea752f7d1ae6 Mon Sep 17 00:00:00 2001 From: Martin Hughes Date: Mon, 14 Sep 2026 12:05:07 +0100 Subject: [PATCH] Prevent some infinite recursion and loops The motivation is to help stop AML causing a thread (or the system as a whole) to become deadlocked. The first protection adds a maximum method call stack depth. This has been chosen arbitrarily, for now. It does not protect against all possible ways of recursing infinitely. For example, it does not stop recursive table loads. (Although LoadTable is not currently supported) The second protection is a straightforward timeout for while loops, with associated tests. These are both based on the uACPI protections, although they differ in a key way: in uACPI, the method stack is unwound. If a table definition is reached, execution continues with the next statement, but the execution is still flagged as failed. In this crate, execution is stopped with an error. --- src/aml/mod.rs | 90 +++++++--- tests/infinite_recursion.rs | 20 +++ tests/test_infra/mod.rs | 23 ++- tests/while.asl | 71 +++++--- tests/while_loop.rs | 23 +++ tools/aml-test-tools/src/handlers/mod.rs | 1 + .../src/handlers/sys_timer_handler.rs | 157 ++++++++++++++++++ tools/aml-test-tools/src/lib.rs | 15 +- tools/aml-tester/src/main.rs | 8 +- 9 files changed, 351 insertions(+), 57 deletions(-) create mode 100644 tests/infinite_recursion.rs create mode 100644 tests/while_loop.rs create mode 100644 tools/aml-test-tools/src/handlers/sys_timer_handler.rs diff --git a/src/aml/mod.rs b/src/aml/mod.rs index 44ef3583..e81eb95e 100644 --- a/src/aml/mod.rs +++ b/src/aml/mod.rs @@ -142,6 +142,21 @@ const INTERPRETER_REVISION: u64 = 1; /// to make us loop forever. const MAX_NAME_PATH_INDIRECTIONS: usize = 8; +// The following constants are public as they are referenced from the AmlError docs. +/// How many nested function calls should we allow? This is a fairly arbitrary figure that could be +/// changed with experience if it proves too small. +/// +/// The interpreter doesn't allocate a stack frame on the processor stack for each AML method call, +/// so we're not worried about overflowing the kernel or process stack. This is a basic way to +/// ensure the AML is not stuck in an infinite recursion (which would deadlock the caller) +pub const MAX_STACK_DEPTH: usize = 1000; + +/// The maximum length of time a single loop should be allowed to execute for, in nanoseconds. +/// +/// This is a fairly arbitrary limit to protect against infinite loops and the risk of system +/// deadlock. +pub const LOOP_TIMEOUT_NS: u64 = 10_000_000_000; + impl BaseInterpreter where H: Handler, @@ -969,6 +984,10 @@ where context.retire_op(op); } Opcode::InternalMethodCall => { + if context_stack.len() >= MAX_STACK_DEPTH { + return Err(AmlError::MethodStackExceeded); + } + extract_args!(op[0..2] => [Argument::Object(method), Argument::Namestring(method_scope)]); let args = op.arguments[2..] .iter() @@ -1174,13 +1193,8 @@ where continue; } - BlockKind::While { start_pc } => { - /* - * Go round again, and create a new in-flight op to have a look at the - * predicate. - */ - context.current_block.pc = start_pc; - context.start(OpInFlight::new(Opcode::While, &[ResolveBehaviour::TermArg])); + BlockKind::While { .. } => { + self.continue_loop(&mut context)?; continue; } } @@ -1781,26 +1795,16 @@ where let pkg_length = context.pkglength()?; let remaining_length = pkg_length - (context.current_block.pc - start_pc); context.start_new_block( - BlockKind::While { start_pc: context.current_block.pc }, + BlockKind::While { + start_pc: context.current_block.pc, + start_nanos: self.handler.nanos_since_boot(), + }, remaining_length, ); context.start(OpInFlight::new(Opcode::While, &[ResolveBehaviour::TermArg])); } Opcode::Continue => { - if let BlockKind::While { start_pc } = &context.current_block.kind { - context.current_block.pc = *start_pc; - } else { - loop { - let Some(block) = context.block_stack.pop() else { - Err(AmlError::ContinueOutsideOfWhile)? - }; - if let BlockKind::While { start_pc } = block.kind { - context.current_block.pc = start_pc; - break; - } - } - } - context.start(OpInFlight::new(Opcode::While, &[ResolveBehaviour::TermArg])); + self.continue_loop(&mut context)?; } Opcode::Break => { if let BlockKind::While { .. } = &context.current_block.kind { @@ -2855,6 +2859,33 @@ where }; Ok(PciAddress::new(seg as u16, bus as u8, device as u8, function as u8)) } + + /// Return to the beginning of a While loop - either because `Continue` was executed or + /// because we reached the end of the While block. + fn continue_loop(&self, context: &mut MethodContext) -> Result<(), AmlError> { + let start: u64; + + if let BlockKind::While { start_pc, start_nanos } = &context.current_block.kind { + context.current_block.pc = *start_pc; + start = *start_nanos; + } else { + loop { + let Some(block) = context.block_stack.pop() else { Err(AmlError::ContinueOutsideOfWhile)? }; + if let BlockKind::While { start_pc, start_nanos } = block.kind { + context.current_block.pc = start_pc; + start = start_nanos; + break; + } + } + } + + if self.handler.nanos_since_boot() - start > LOOP_TIMEOUT_NS { + return Err(AmlError::LoopTimeout); + } + + context.start(OpInFlight::new(Opcode::While, &[ResolveBehaviour::TermArg])); + Ok(()) + } } /// A `MethodContext` represents a piece of running AML code - either a real method, or the @@ -2898,6 +2929,7 @@ pub enum BlockKind { IfThenBranch, While { start_pc: usize, + start_nanos: u64, }, } @@ -3027,11 +3059,8 @@ impl MethodContext { if args.len() != flags.arg_count() { return Err(AmlError::MethodArgCountIncorrect); } - let block = Block { - stream: code.clone(), - pc: 0, - kind: BlockKind::Method { method_scope: scope.clone() }, - }; + let block = + Block { stream: code.clone(), pc: 0, kind: BlockKind::Method { method_scope: scope.clone() } }; let args = core::array::from_fn(|i| { if let Some(arg) = args.get(i) { arg.clone() } else { Object::Uninitialized.wrap() } }); @@ -3562,6 +3591,13 @@ pub enum AmlError { /// An internal interpreter error has occured, and the interpreter has been left in an unknown /// state. More information may be given in the contained value. InternalError(String), + + /// The maximum stack depths of method calls [`MAX_STACK_DEPTH`] has been reached, so the method + /// call has been aborted. + MethodStackExceeded, + + /// The maximum length of time a loop ([`LOOP_TIMEOUT_NS`]) can execute for has been exceeded. + LoopTimeout, } #[derive(Debug, Clone, Copy, PartialEq)] diff --git a/tests/infinite_recursion.rs b/tests/infinite_recursion.rs new file mode 100644 index 00000000..8329b51c --- /dev/null +++ b/tests/infinite_recursion.rs @@ -0,0 +1,20 @@ +mod test_infra; +use acpi::aml::AmlError; +use aml_test_tools::{RunTestResult, TestFailureReason, handlers::null_handler::NullHandler}; +use std::assert_matches; + +#[test] +fn infinite_method_recursion() { + const ASL: &str = r#" +DefinitionBlock("", "DSDT", 1, "RSACPI", "UACPI", 1) { + Name(X, 0) + Method(INF) { + X++ + INF() + } + INF() +}"#; + + let r = test_infra::run_aml_test_with_result(ASL, NullHandler); + assert_matches!(r, RunTestResult::Failed(_, TestFailureReason::ParseFail(AmlError::MethodStackExceeded))); +} diff --git a/tests/test_infra/mod.rs b/tests/test_infra/mod.rs index 45f4fc7b..aa94b987 100644 --- a/tests/test_infra/mod.rs +++ b/tests/test_infra/mod.rs @@ -15,23 +15,32 @@ use std::str::FromStr; // `run_aml_test` and `run_opcodes_test` are very similar in structure, but whilst there are only // two of them it's not worth adding complexity to make them DRY. -/// Run a test against an ASL string. +/// Run a test against an ASL string and check for successful execution. /// /// The string `asl` represents a compile-able ASL string, so needs to include the `DefinitionBlock` /// statement. #[allow(dead_code)] pub fn run_aml_test(asl: &'static str, handler: H) -> Interpreter> { - // Tests calling `run_aml_test` don't do much else, and we usually want logging, so initialize it here. + let result = run_aml_test_with_result(asl, handler); + match result { + RunTestResult::Pass(interpreter) => interpreter, + result => panic!("Test failed with: {:?}", TestResult::from(&result)), + } +} + +/// Run a test against an ASL string and return the raw result. +/// +/// The string `asl` represents a compile-able ASL string, so needs to include the `DefinitionBlock` +/// statement. +#[allow(dead_code)] +pub fn run_aml_test_with_result(asl: &'static str, handler: H) -> RunTestResult> { + // Tests calling the test functions don't do much else, and we usually want logging, so initialize it here. let _ = pretty_env_logger::try_init(); let logged_handler = LoggingHandler::new(handler); let interpreter = new_interpreter(logged_handler); - let result = run_test_for_string(asl, interpreter, &None); - match result { - RunTestResult::Pass(interpreter) => interpreter, - result => panic!("Test failed with: {:?}", TestResult::from(&result)), - } + run_test_for_string(asl, interpreter, &None) } /// Evaluate an object without arguments and return its unwrapped value. diff --git a/tests/while.asl b/tests/while.asl index 7d6c8d85..e6b4a9b6 100644 --- a/tests/while.asl +++ b/tests/while.asl @@ -1,31 +1,62 @@ DefinitionBlock("while.aml", "DSDT", 1, "RSACPI", "WHILE", 1) { - Name(X, 0) - While (X < 5) { - X++ + Name(FCNT, 0) + + Method (CHEK, 2) { + If (Arg0 != Arg1) { + FCNT++ + } } - // Test `DefBreak` - Y should only make it to 5 - Name(Y, 0) - While (Y < 10) { - If (Y >= 5) { - Break + Method(T1) { + Name(X, 0) + While (X < 5) { + X++ } - Y++ + CHEK(X, 5) } - // Test `DefContinue` - Z should remain at zero - Name(CNT, 0) - Name(Z, 0) - While (CNT < 5) { - CNT++ - Continue - Z++ + Method(T2) { + // Test `DefBreak` - Y should only make it to 5 + Name(Y, 0) + While (Y < 10) { + If (Y >= 5) { + Break + } + + Y++ + } + CHEK(Y, 5) } - // Test `Decrement` in the predicate - common pattern - Local0 = 5 - While (Local0--) { - Continue + Method(T3) { + // Test `DefContinue` - Z should remain at zero + Name(CNT, 0) + Name(Z, 0) + While (CNT < 5) { + CNT++ + Continue + Z++ + } + CHEK(Z, 0) + CHEK(CNT, 5) + } + + Method(T4) { + // Test `Decrement` in the predicate - common pattern + Local0 = 5 + While (Local0--) { + Continue + } + CHEK(Local0, 0) + } + + Method(MAIN) { + T1() + T2() + T3() + T4() + + Return(FCNT) } } diff --git a/tests/while_loop.rs b/tests/while_loop.rs new file mode 100644 index 00000000..238574e9 --- /dev/null +++ b/tests/while_loop.rs @@ -0,0 +1,23 @@ +mod test_infra; +use acpi::aml::AmlError; +use aml_test_tools::{ + RunTestResult, + TestFailureReason, + handlers::{null_handler::NullHandler, sys_timer_handler::SystemTimerHandler}, +}; +use std::assert_matches; + +#[test] +fn infinite_while_loop() { + const ASL: &str = r#" +DefinitionBlock("", "DSDT", 1, "RSACPI", "UACPI", 1) { + Name(X, 0) + While (1) { + X++ + } +}"#; + + let handler = SystemTimerHandler::new(NullHandler, 100); + let r = test_infra::run_aml_test_with_result(ASL, handler); + assert_matches!(r, RunTestResult::Failed(_, TestFailureReason::ParseFail(AmlError::LoopTimeout))); +} diff --git a/tools/aml-test-tools/src/handlers/mod.rs b/tools/aml-test-tools/src/handlers/mod.rs index 85d58708..99315c30 100644 --- a/tools/aml-test-tools/src/handlers/mod.rs +++ b/tools/aml-test-tools/src/handlers/mod.rs @@ -7,3 +7,4 @@ pub mod listed_response_handler; pub mod logging_handler; pub mod null_handler; pub mod std_test_handler; +pub mod sys_timer_handler; diff --git a/tools/aml-test-tools/src/handlers/sys_timer_handler.rs b/tools/aml-test-tools/src/handlers/sys_timer_handler.rs new file mode 100644 index 00000000..7dda035a --- /dev/null +++ b/tools/aml-test-tools/src/handlers/sys_timer_handler.rs @@ -0,0 +1,157 @@ +//! A wrapper around another [`Handler`] that checks for the correct sequence of commands in a test. + +use acpi::{Handle, Handler, RawPhysicalMapping, aml::AmlError}; +use pci_types::PciAddress; +use std::{ + thread::sleep, + time::{Duration, Instant}, +}; + +/// A wrapper around another [`Handler`] that enables the handler to use the system's timing +/// capabilities. +/// +/// All commands are forwarded except [`sleep`], [`stall`] and [`nanos_since_boot`]. +#[derive(Clone, Debug)] +pub struct SystemTimerHandler +where + H: Handler + Clone, +{ + next_handler: H, + start_time: Instant, + scale_factor: u64, +} + +impl SystemTimerHandler +where + H: Handler + Clone, +{ + /// Construct a new `SystemTimerHandler` + /// + /// * `next_handler` is the handler to forward non-timing method calls to. + /// * `scale_factor` effectively accelerates time by the given integer multiple. For example, a + /// 10-second wait with a scale factor of 5 would lead to a 2-second wait. Note: Large + /// multiples combined with small waits may lead to reduced accuracy or zero-length waits. + pub fn new(next_handler: H, scale_factor: u64) -> Self { + Self { next_handler, start_time: Instant::now(), scale_factor } + } +} + +impl Handler for SystemTimerHandler +where + H: Handler + Clone, +{ + unsafe fn map_physical_region(&self, physical_address: usize, size: usize) -> RawPhysicalMapping { + unsafe { self.next_handler.map_physical_region::(physical_address, size) } + } + + unsafe fn unmap_physical_region(&self, region: RawPhysicalMapping) { + unsafe { + self.next_handler.unmap_physical_region(region); + } + } + + fn read_u8(&self, address: usize) -> u8 { + self.next_handler.read_u8(address) + } + + fn read_u16(&self, address: usize) -> u16 { + self.next_handler.read_u16(address) + } + + fn read_u32(&self, address: usize) -> u32 { + self.next_handler.read_u32(address) + } + + fn read_u64(&self, address: usize) -> u64 { + self.next_handler.read_u64(address) + } + + fn write_u8(&self, address: usize, value: u8) { + self.next_handler.write_u8(address, value); + } + + fn write_u16(&self, address: usize, value: u16) { + self.next_handler.write_u16(address, value); + } + + fn write_u32(&self, address: usize, value: u32) { + self.next_handler.write_u32(address, value); + } + + fn write_u64(&self, address: usize, value: u64) { + self.next_handler.write_u64(address, value); + } + + fn read_io_u8(&self, port: u16) -> u8 { + self.next_handler.read_io_u8(port) + } + + fn read_io_u16(&self, port: u16) -> u16 { + self.next_handler.read_io_u16(port) + } + + fn read_io_u32(&self, port: u16) -> u32 { + self.next_handler.read_io_u32(port) + } + + fn write_io_u8(&self, port: u16, value: u8) { + self.next_handler.write_io_u8(port, value); + } + + fn write_io_u16(&self, port: u16, value: u16) { + self.next_handler.write_io_u16(port, value); + } + + fn write_io_u32(&self, port: u16, value: u32) { + self.next_handler.write_io_u32(port, value); + } + + fn read_pci_u8(&self, address: PciAddress, offset: u16) -> u8 { + self.next_handler.read_pci_u8(address, offset) + } + + fn read_pci_u16(&self, address: PciAddress, offset: u16) -> u16 { + self.next_handler.read_pci_u16(address, offset) + } + + fn read_pci_u32(&self, address: PciAddress, offset: u16) -> u32 { + self.next_handler.read_pci_u32(address, offset) + } + + fn write_pci_u8(&self, address: PciAddress, offset: u16, value: u8) { + self.next_handler.write_pci_u8(address, offset, value); + } + + fn write_pci_u16(&self, address: PciAddress, offset: u16, value: u16) { + self.next_handler.write_pci_u16(address, offset, value); + } + + fn write_pci_u32(&self, address: PciAddress, offset: u16, value: u32) { + self.next_handler.write_pci_u32(address, offset, value); + } + + fn nanos_since_boot(&self) -> u64 { + ((Instant::now() - self.start_time).as_nanos() as u64) * self.scale_factor + } + + fn stall(&self, microseconds: u64) { + // There's no `std` equivalent to stall, and sleep is probably OK for a test environment. + sleep(Duration::from_micros(microseconds / self.scale_factor)); + } + + fn sleep(&self, milliseconds: u64) { + sleep(Duration::from_millis(milliseconds / self.scale_factor)); + } + + fn create_mutex(&self) -> Handle { + self.next_handler.create_mutex() + } + + fn acquire(&self, mutex: Handle, timeout: u16) -> Result<(), AmlError> { + self.next_handler.acquire(mutex, timeout) + } + + fn release(&self, mutex: Handle) { + self.next_handler.release(mutex); + } +} diff --git a/tools/aml-test-tools/src/lib.rs b/tools/aml-test-tools/src/lib.rs index 20b054d3..6cad0af9 100644 --- a/tools/aml-test-tools/src/lib.rs +++ b/tools/aml-test-tools/src/lib.rs @@ -25,7 +25,7 @@ use log::{error, trace}; use std::{ cell::SyncUnsafeCell, ffi::OsStr, - fmt::Debug, + fmt::{Debug, Formatter}, fs::File, io::{Read, Write}, panic::{AssertUnwindSafe, catch_unwind}, @@ -53,6 +53,19 @@ where Panicked, } +impl std::fmt::Debug for RunTestResult +where + T: Handler, +{ + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + f.write_str(match self { + RunTestResult::Pass(_) => "Pass", + RunTestResult::Failed(_, _) => "Failed", + RunTestResult::Panicked => "Panicked", + }) + } +} + /// The result of a test #[derive(Debug, PartialEq)] pub enum TestResult { diff --git a/tools/aml-tester/src/main.rs b/tools/aml-tester/src/main.rs index 7568e45c..13e3ae17 100644 --- a/tools/aml-tester/src/main.rs +++ b/tools/aml-tester/src/main.rs @@ -14,7 +14,11 @@ use acpi::Handler; use aml_test_tools::{ - handlers::{logging_handler::LoggingHandler, null_handler::NullHandler}, + handlers::{ + logging_handler::LoggingHandler, + null_handler::NullHandler, + sys_timer_handler::SystemTimerHandler, + }, new_interpreter, resolve_and_compile, result::ExpectedResult, @@ -300,5 +304,5 @@ fn find_tests(matches: &clap::ArgMatches) -> std::io::Result> { } fn new_handler() -> impl Handler { - LoggingHandler::new(NullHandler {}) + LoggingHandler::new(SystemTimerHandler::new(NullHandler {}, 1)) }