Skip to content

Handle overlong AML package lengths - #365

Open
mikamikasuki wants to merge 2 commits into
rust-osdev:mainfrom
mikamikasuki:fix-overlong-aml-package-length-362
Open

mikamikasuki wants to merge 2 commits into
rust-osdev:mainfrom
mikamikasuki:fix-overlong-aml-package-length-362

Conversation

@mikamikasuki

Copy link
Copy Markdown

Fixes #362.

When a Package's declared element count is reached before its encoded PkgLength ends, the parser currently asserts that the block also reached the length boundary. An overlong PkgLength followed by a NameOp therefore panics and prevents later namespace objects from loading.

On package completion, resume at the next AML term only when its prefix cannot encode a PackageElement. Keep the encoded boundary when the next bytes may be a DataRefObject or NameString, whose ownership is ambiguous. Add a raw AML regression with adjacent packages where the first PkgLength overlaps the following NameOp.

Validation:

  • cargo test -p acpi --no-default-features --features aml --test incorrect_package_length (1 passed)
  • cargo +nightly-2026-04-05 fmt --all -- --check
  • git diff --check

@martin-hughes

Copy link
Copy Markdown
Contributor

Thanks for this @mikamikasuki! Before I review / test / merge this I'm going to take some time to explore how other interpreters deal with the ambiguous case mentioned in your comments.

Alternatively, have you already looked into this?

@martin-hughes

Copy link
Copy Markdown
Contributor

Sorry for not getting round to this already - it'll probably be another couple of weeks I'm afraid. Thanks for being patient so far!

@martin-hughes

Copy link
Copy Markdown
Contributor

Thanks for your patience @mikamikasuki. You can see from the discussion in #362 that I've taken a look at the Windows interpreter.

If we took this change I think it'd make us the only interpreter to attempt to recover ambiguous packages lengths this way.

My feeling is we should use one of the existing methods, either:

  • The ACPICA method of relying on the number of package elements (and discarding the package length field)
  • The uACPI way of abandoning the rest of the table, but not discarding the previously loaded elements.

The Windows way of not loading any changes would be more tricky for us to implement, and I don't feel strongly that table loading should be atomic. I can't see any security benefit for making it so, and I doubt any hardware would be adversely affected by having namespace elements loaded that would be missing on Windows. If we receive a bug report we could revisit this.

Conversely, I could see issues with your method - consider if a bad package length happens to end in the middle of a String with a valid character to start a new element. We could then continue parsing that string as namespace elements, leading us to have a completely different set of namespace object loaded compared to Windows or Linux. I can see how that could lead to undesirable behaviours.

@IsaacWoods thoughts? I'm minded to close without merging.

@mikamikasuki Thanks again - we both appreciate anyone taking an interest in this crate!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incorrect pkglength can cause panic

2 participants