Prevent some infinite recursion and loops

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<H, R> BaseInterpreter<H, R>
 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)]

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)));
+}

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<H: Handler>(asl: &'static str, handler: H) -> Interpreter<LoggingHandler<H>> {
-    // 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<H: Handler>(asl: &'static str, handler: H) -> RunTestResult<LoggingHandler<H>> {
+    // 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.

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)
     }
 }

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)));
+}

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;