8000 Validate guest-writable fields in try_pop_buffer_into before allocation by ludfjig · Pull Request #1262 · hyperlight-dev/hyperlight · GitHub
[go: up one dir, main page]
Skip to content

Validate guest-writable fields in try_pop_buffer_into before allocation#1262

Merged
ludfjig merged 1 commit intohyperlight-dev:mainfrom
ludfjig:validate_guest_output
Feb 26, 2026
Merged

Validate guest-writable fields in try_pop_buffer_into before allocation#1262
ludfjig merged 1 commit intohyperlight-dev:mainfrom
ludfjig:validate_guest_output

Conversation

@ludfjig
Copy link
Contributor
@ludfjig ludfjig commented Feb 25, 2026 •
edited
Loading

try_pop_buffer_into reads two values from guest-writable shared memory

  • Size prefix: A malicious guest can set this to near u32::MAX, causing the host to attempt a ~4 GB allocation via vec![0; fb_buffer_size] (DoS).
  • Back-pointer: A malicious guest can point this outside the buffer, causing fill(0, ...) to zero unrelated host-managed data within shared memory (e.g., PEB fields, input buffer) and leaving the buffer stack in an inconsistent state.
  • Arithmetic overflow/underflow: A back-pointer just below the stack pointer causes underflow in stack_pointer_rel - last_element_offset_rel - 8. A size prefix of 0xFFFF_FFFD causes +4 to wrap around in debug (panic) or release (silent wrap to a small value).

Changes:

  • Validate last_element_offset_rel (back-pointer) is within [[8, stack_pointer_rel) before use
  • Validate fb_buffer_size does not exceed the actual element slot size before allocation
  • Use checked_add(4) on the size prefix instead of bare + 4, returning a descriptive error on u32 overflow
  • Add unit tests covering corrupt size prefix, corrupt back-pointer (too small / too large), off-by-one, back-pointer near stack pointer (underflow), and u32 size prefix overflow
  • Add integration tests with guest functions that write corrupt output buffers and halt

@ludfjig ludfjig added kind/bugfix For PRs that fix bugs area/security Involves security-related changes or fixes labels Feb 25, 2026
@ludfjig ludfjig requested a review from Copilot February 25, 2026 21:54

This comment was marked as resolved.

Sign in to view

@ludfjig ludfjig force-pushed the validate_guest_output branch from 24dccad to 7d51b11 Compare February 25, 2026 22:22
@ludfjig ludfjig requested a review from Copilot February 25, 2026 22:23
Copilot AI reviewed Feb 25, 2026
View reviewed changes
Copy link
Contributor
Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

@ludfjig
Validate guest-writable fields in try_pop_buffer_into before allocation
56d40f6 
The back-pointer and flatbuffer size prefix in the shared output buffer
are written by the guest and were used without validation, allowing a
malicious guest to trigger a ~4 GB host-side allocation.

Add bounds checks on both fields before any heap allocation occurs and
return descriptive errors on violation. Add unit and integration tests
exercising corrupt size prefixes and back-pointers.

Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
@ludfjig ludfjig force-pushed the validate_guest_output branch from 7d51b11 to 56d40f6 Compare February 25, 2026 22:37
syntactically
syntactically reviewed Feb 26, 2026
View reviewed changes
syntactically
syntactically approved these changes Feb 26, 2026
View reviewed changes
Copy link
Member
@syntactically syntactically left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The arithmetic overflows causing unintentional panics issue I bet is something we have in other places. (I stand by some of the arguments I made earlier about panic-ing being reasonable behaviour in some situations---but it's important that we are directly aware of the panic opportunities and clear on whether they make sense---i.e. we should have a sensible reasoned argument why they ought to be impossible, even in the face of attacker-controlled data). It would be interesting to take a look at e.g. enumerating every cfg path from any exported function to panic (on an opt build, so that a bunch of the easy ones are gone) and see how much work it would be to make an argument either way on each path.

@ludfjig ludfjig merged commit 513b08e into hyperlight-dev:main Feb 26, 2026
56 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@andreiltd andreiltd andreiltd approved these changes

@syntactically syntactically syntactically approved these changes

@danbugs danbugs Awaiting requested review from danbugs danbugs is a code owner

@dblnz dblnz Awaiting requested review from dblnz dblnz is a code owner

@devigned devigned Awaiting requested review from devigned devigned is a code owner

@marosset marosset Awaiting requested review from marosset marosset is a code owner

@jprendes jprendes Awaiting requested review from jprendes jprendes is a code owner

@simongdavies simongdavies Awaiting requested review from simongdavies simongdavies is a code owner

Assignees

No one assigned

Labels

area/security Involves security-related changes or fixes kind/bugfix For PRs that fix bugs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ludfjig @andreiltd @syntactically
0