[PATCH] ACPICA: Parser: do not set the AML pointer to a NULL AmlLastWhile
Vlatko Kosturjak <[email protected]>
| Newsgroups | org.kernel.vger.linux-acpi,dev.linux.lists.acpica-devel |
|---|---|
| Message-ID | <CAPAw8HGxmd2V5GgjL191f3pwOnbuGp4cfDaxCWiR57EquLOecw@mail.gmail.com> |
AcpiPsNextParseState() can set ParserState->Aml to NULL, after which the parse
loop dereferences it in AcpiPsPeekOpcode(). A malformed ACPI table supplied by
firmware therefore panics the kernel in PID 1 during acpi_init(), before
userspace exists - the machine does not boot.
This is the largest of a set of related findings: it accounts for 110 of 189
malformed SSDTs that leave a stock x86_64_defconfig kernel unbootable
(tested on pristine Linux 7.2.2 - tag: v7.2.2). The smallest trigger
is a 46-byte table containing no While, Break or Continue at all.
WalkState->AmlLastWhile is only ever assigned in AcpiDsExecEndOp() while
handling a While loop, so outside a loop it is still NULL from the zeroed walk
state. AcpiPsNextParseState() assigns it to ParserState->Aml for AE_CTRL_BREAK,
AE_CTRL_CONTINUE and AE_CTRL_PENDING.
The first two are guarded at their source; AE_CTRL_PENDING is not, and it is not
a loop-only status. AcpiExNameSegment() returns it for any name segment that
starts with a digit or a non-alpha character - entirely controlled by the AML
being parsed, with no loop involved. AcpiPsPeekOpcode()'s bounds check does not
help, because NULL < AmlEnd passes.
In Linux this is reached from AcpiTbLoadNamespace() during acpi_init(), so a
table supplied by firmware faults in PID 1 inside do_one_initcall() and
panics the kernel before userspace exists:
BUG: kernel NULL pointer dereference at 0000000000000000
RIP: 0010:acpi_ps_peek_opcode+0x16/0x40
acpi_ps_get_next_arg / acpi_ps_parse_loop / acpi_ps_parse_aml
acpi_ns_load_table / acpi_tb_load_namespace / acpi_init
Kernel panic - not syncing: Attempted to kill init!
Refuse to take the branch when AmlLastWhile is NULL and fail the parse with
AE_AML_NO_WHILE, whose existing text is "Break or Continue without a While".
Also NULL-check WalkState->ControlState on the two paths that dereference it;
the AE_CTRL_FALSE comment already says "a predicate (if any)".
One detail worth keeping if you rework the patch: it also sets
ParserState->Aml = ParserState->AmlEnd on the error paths. That is required, not
cosmetic. During a table load AcpiPsParseLoop() deliberately converts AML
exceptions back to AE_OK ("keep trying to load the table"), so an error return
that does not advance the AML pointer re-parses the same position forever. An
earlier version of this patch omitted it and turned one table's panic into a
hang.
Why this matters beyond "bad firmware breaks your machine": under confidential
computing the host is untrusted by design and supplies the guest's ACPI tables.
Documentation/security/snp-tdx-threat-model.rst lists "Malformed runtime input"
from a misbehaving host in its guest threat matrix. That is the model in which
these are security bugs rather than robustness bugs.
Reproducer, evidence and step-by-step instructions can be found at the
issue reported here:
https://github.com/open-acpica/acpica/issues/1226
Sending this to the public list rather than [email protected]: the attacker
must already control the platform firmware, no control over the faulting
pointer was demonstrated, and the fix is small with nothing to coordinate.
Say so explicitly if you would rather it had gone elsewhere.
Signed-off-by: Vlatko Kosturjak <[email protected]>
---
diff --git a/source/components/parser/psparse.c
b/source/components/parser/psparse.c
index 1aacc0c..7fb28dc 100644
--- a/source/components/parser/psparse.c
+++ b/source/components/parser/psparse.c
@@ -338,6 +338,15 @@ AcpiPsNextParseState (
case AE_CTRL_BREAK:
+ if (!WalkState->AmlLastWhile || !WalkState->ControlState)
+ {
+ ACPI_ERROR ((AE_INFO,
+ "Break without a valid enclosing While"));
+ ParserState->Aml = ParserState->AmlEnd;
+ Status = AE_AML_NO_WHILE;
+ break;
+ }
+
ParserState->Aml = WalkState->AmlLastWhile;
WalkState->ControlState->Common.Value = FALSE;
Status = AE_CTRL_BREAK;
@@ -345,12 +354,30 @@ AcpiPsNextParseState (
case AE_CTRL_CONTINUE:
+ if (!WalkState->AmlLastWhile)
+ {
+ ACPI_ERROR ((AE_INFO,
+ "Continue without a valid enclosing While"));
+ ParserState->Aml = ParserState->AmlEnd;
+ Status = AE_AML_NO_WHILE;
+ break;
+ }
+
ParserState->Aml = WalkState->AmlLastWhile;
Status = AE_CTRL_CONTINUE;
break;
case AE_CTRL_PENDING:
+ if (!WalkState->AmlLastWhile)
+ {
+ ACPI_ERROR ((AE_INFO,
+ "Pending control state without a valid enclosing While"));
+ ParserState->Aml = ParserState->AmlEnd;
+ Status = AE_AML_NO_WHILE;
+ break;
+ }
+
ParserState->Aml = WalkState->AmlLastWhile;
break;
@@ -391,7 +418,10 @@ AcpiPsNextParseState (
/* In the case of a BREAK, just force a predicate (if any) to FALSE */
- WalkState->ControlState->Common.Value = FALSE;
+ if (WalkState->ControlState)
+ {
+ WalkState->ControlState->Common.Value = FALSE;
+ }
Status = AE_CTRL_END;
break;