Skip to content

fix: use checked arithmetic in push_buffer (defense in depth) - #1820

Merged
simongdavies merged 8 commits into
hyperlight-dev:mainfrom
JM00NJ:main
Sep 10, 2026
Merged

fix: use checked arithmetic in push_buffer (defense in depth)#1820
simongdavies merged 8 commits into
hyperlight-dev:mainfrom
JM00NJ:main

Conversation

@JM00NJ

@JM00NJ JM00NJ commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

push_buffer computed data.len() + 8 and stack_pointer_rel + data.len() + 8 with unchecked arithmetic.
In a release build, if data.len() is near usize::MAX, these additions silently wrap around, causing size_required to appear smaller than size_available and bypassing the buffer-full check.

The subsequent copy_from_slice call catches this via bounds_check! today, so there is no currently exploitable path. However, as try_pop_buffer_into already uses checked_add for its size prefix, this is an inconsistency worth closing before future refactors introduce a caller where the downstream safety net is absent (defense in depth).

Changes:

  • Add StackError::PushSizeOverflow variant with a clear error message
  • Replace unchecked + with checked_add in both arithmetic sites
  • Add a test that constructs a maximally-sized slice and asserts PushSizeOverflow is returned before any memory access occurs

`push_buffer` computed `data.len() + 8` and
`stack_pointer_rel + data.len() + 8` with unchecked arithmetic.
In a release build, if `data.len()` is near `usize::MAX`, these
additions silently wrap around, causing `size_required` to appear
smaller than `size_available` and bypassing the buffer-full check.

The subsequent `copy_from_slice` call catches this via `bounds_check!`
today, so there is no currently exploitable path. However, as
`try_pop_buffer_into` already uses `checked_add` for its size
prefix, this is an inconsistency worth closing before future
refactors introduce a caller where the downstream safety net is
absent (defense in depth).

Changes:
- Add `StackError::PushSizeOverflow` variant with a clear error message
- Replace unchecked `+` with `checked_add` in both arithmetic sites
- Add a test that constructs a maximally-sized slice and asserts
  `PushSizeOverflow` is returned before any memory access occurs

Signed-off-by: commSync <51642194+JM00NJ@users.noreply.github.com>
@simongdavies simongdavies added kind/bugfix For PRs that fix bugs area/security Involves security-related changes or fixes labels Sep 10, 2026
Signed-off-by: commSync <51642194+JM00NJ@users.noreply.github.com>
Signed-off-by: commSync <51642194+JM00NJ@users.noreply.github.com>
Signed-off-by: commSync <51642194+JM00NJ@users.noreply.github.com>
@JM00NJ

JM00NJ commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

Why the fuzz target is included

The fuzz target (fuzz/fuzz_targets/fuzz_push_pop_buffer.rs)
was written alongside this fix to validate the checked_add
guards and to provide ongoing regression coverage.

Key differences from existing fuzz targets:

  • Exercises the memory stack API directly, no sandbox or
    hypervisor required
  • ~35K exec/s vs existing targets which require KVM spin-up
  • Targets the guest-controlled code path in try_pop_buffer_into,
    which is the actual trust boundary

Fuzz results across three configurations:

Input size Iterations Crashes
4KB 184M+ 0
64KB 10M+ 0
256KB 57M+ 0

At 256KB, the fuzzer autonomously identified 0xFFFFFFFB
(u32::MAX - 4) as a critical boundary value and exercised
it 3.28M times — the exact overflow point that
checked_add(4) in try_pop_buffer_into guards against.

Coverage plateau reached at 399 branches / 872 features,
confirming all reachable code paths were exercised.

As the guest protocol evolves and new message types are
added to try_pop_buffer_into, the fuzzer will automatically
exercise new code paths without any modifications.

simongdavies
simongdavies previously approved these changes Sep 10, 2026

@simongdavies simongdavies left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks , LGTM, one minor nit on the comments.

Comment thread fuzz/fuzz_targets/fuzz_push_pop_buffer.rs Outdated
Updated comments to clarify the purpose of the test case.

Signed-off-by: commSync <51642194+JM00NJ@users.noreply.github.com>

@simongdavies simongdavies left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@JM00NJ LGTM thanks!!

Comment thread fuzz/Cargo.toml
@simongdavies
simongdavies enabled auto-merge (squash) September 10, 2026 18:47
@simongdavies
simongdavies merged commit bacb222 into hyperlight-dev:main Sep 10, 2026
54 checks passed
Comment thread src/hyperlight_host/src/mem/shared_mem.rs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants