Skip to content

winch(aarch64): Revisit the shadow stack pointer approach#10146

Merged
saulecabrera merged 1 commit intobytecodealliance:mainfrom
saulecabrera:winch-aarch64-fix-loads-stores
Jan 30, 2025
Merged

winch(aarch64): Revisit the shadow stack pointer approach#10146
saulecabrera merged 1 commit intobytecodealliance:mainfrom
saulecabrera:winch-aarch64-fix-loads-stores

Conversation

@saulecabrera
Copy link
Member

@saulecabrera saulecabrera commented Jan 29, 2025

This commit marks another step toward finalizing AArch64 support in
Winch.

While enabling spec tests, I experienced some unexpected failures
related to Wasm loads/stores and traps. The observed
symptoms are as follows:

  • Under normal conditions, Wasm loads/stores work as expected.
  • In out-of-bounds scenarios, loads/stores result in a segmentation
    fault, whereas the expected behavior is to trigger an out-of-bounds trap.
  • When out-of-bounds access can be determined statically, the program
    still results in a segmentation fault instead of the anticipated
    out-of-bounds trap.

Debugging revealed the following issues:

  • The stack pointer was not correctly aligned to 16 bytes when entering
    signal handlers, which caused the segmentation fault.
  • Wasm loads and stores were not flagged as untrusted, leading to
    segmentation faults even when the stack pointer was properly aligned.

This commit fixes the previous issues by:

  • Correctly flagging wasm loads and stores as untrusted.
  • Reworking the shadow stack pointer approach such that it allows
    aligning the stack pointer at arbitrary points in the program,
    particularly where signal handling might be needed. This rework
    involves changing some principles introduced in
    winch: Use aarch64 backend for code emission. #5652; namely:
    changing the primary stack pointer register to be the shadow stack
    pointer. See the updated comments in the code for more details.

Note that this change doesn't enable spectests. I'll follow-up with more work to do so. To try this change, run:

  cargo run -- wast -Ccompiler=winch tests/spec_testsuite/address.wast

--

The diff is large-ish due to all the changes in disassembly tests, however, all code changes can be found in this commit.

@saulecabrera saulecabrera requested review from a team as code owners January 29, 2025 01:18
@saulecabrera saulecabrera requested review from abrown and alexcrichton and removed request for a team January 29, 2025 01:18
@saulecabrera saulecabrera changed the title Winch aarch64 fix loads stores winch(aarch64): Revisit the shadow stack pointer approach Jan 29, 2025
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

While I'm not expert on Winch this all looks and sounds reasonable enough to me 👍

This commit marks another step toward finalizing AArch64 support in
Winch.

While enabling spec tests, I experienced some unexpected failures
related to Wasm loads/stores and traps. The observed
symptoms are as follows:

* Under normal conditions, Wasm loads/stores work as expected.
* In out-of-bounds scenarios, loads/stores result in a segmentation
  fault, whereas the expected behavior is to trigger an out-of-bounds trap.
* When out-of-bounds access can be determined statically, the program
  still results in a segmentation fault instead of the anticipated
  out-of-bounds trap.

Debugging revealed the following issues:

* The stack pointer was not correctly aligned to 16 bytes when entering
  signal handlers, which caused the segmentation fault.
* Wasm loads and stores were not flagged as untrusted, leading to
  segmentation faults even when the stack pointer was properly aligned.

This commit fixes the previous issues by:

* Correctly flagging wasm loads and stores as untrusted.
* Reworking the shadow stack pointer approach such that it allows
  aligning the stack pointer at arbitrary points in the program,
  particularly where signal handling might be needed. This rework
  involves changing some principles introduced in
  bytecodealliance#5652; namely:
  changing the primary stack pointer register to be the shadow stack
  pointer. See the updates comments in the code for more details.

Note that this change doesn't enable spectests. To try this change, run:

  cargo run -- wast -Ccompiler=winch tests/spec_testsuite/address.wast
@saulecabrera saulecabrera force-pushed the winch-aarch64-fix-loads-stores branch from 05b90cd to 13fe61c Compare January 29, 2025 23:50
@saulecabrera saulecabrera added this pull request to the merge queue Jan 30, 2025
Merged via the queue into bytecodealliance:main with commit af3c029 Jan 30, 2025
39 checks passed
@saulecabrera saulecabrera deleted the winch-aarch64-fix-loads-stores branch January 30, 2025 00:25
saulecabrera added a commit to saulecabrera/wasmtime that referenced this pull request Feb 6, 2025
This commit is one more in the series of executing spec tests for
aarch64.

It's mostly a small follow-up to bytecodealliance#10146,
in which I omitted contextualizing the memory flags for stores.
saulecabrera added a commit to saulecabrera/wasmtime that referenced this pull request Feb 6, 2025
This commit is one more in the series of executing spec tests for
aarch64.

It's mostly a small follow-up to bytecodealliance#10146,
in which I omitted contextualizing the memory flags for stores as well
as ensuring that the SP is aligned when emitting other trapping
instructions like `checked_uadd`.
saulecabrera added a commit to saulecabrera/wasmtime that referenced this pull request Feb 20, 2025
This commit is a follow-up to
bytecodealliance#10146  and represents
another step toward fixing the remaining issues discovered through spec
tests in the same vein as bytecodealliance#10201

Specifically, this commit ensures that the stack pointer is always in
sync with the shadow stack pointer. The previous approach was lossy
because it only performed the sync when reserving stack space. While
this approach worked in some cases, it failed to account for situations
where the shadow stack pointer might be adjusted and aligned for calls.
As a result, the stack pointer could become unaligned when claiming
stack space, leading to issues at call sites.

It is possible to avoid the unconditional move and perform it only when
alignment is needed, i.e., at call sites and when the real stack pointer
is unaligned. However, as of now, the simplest solution is to always
perform the sync, which integrates best with the current infrastructure.
github-merge-queue bot pushed a commit that referenced this pull request Feb 24, 2025
…10201)

* winch(aarch64): Correct treatment for stores and other trapping ops

This commit is one more in the series of executing spec tests for
aarch64.

It's mostly a small follow-up to #10146,
in which I omitted contextualizing the memory flags for stores as well
as ensuring that the SP is aligned when emitting other trapping
instructions like `checked_uadd`.

* Add note around why SP alignment is needed in `checked_uadd`
saulecabrera added a commit to saulecabrera/wasmtime that referenced this pull request Feb 26, 2025
This commit is a follow-up to
bytecodealliance#10146  and represents
another step toward fixing the remaining issues discovered through spec
tests in the same vein as bytecodealliance#10201

Specifically, this commit ensures that the stack pointer is always in
sync with the shadow stack pointer. The previous approach was lossy
because it only performed the sync when reserving stack space. While
this approach worked in some cases, it failed to account for situations
where the shadow stack pointer might be adjusted and aligned for calls.
As a result, the stack pointer could become unaligned when claiming
stack space, leading to issues at call sites.

It is possible to avoid the unconditional move and perform it only when
alignment is needed, i.e., at call sites and when the real stack pointer
is unaligned. However, as of now, the simplest solution is to always
perform the sync, which integrates best with the current infrastructure.
github-merge-queue bot pushed a commit that referenced this pull request Feb 26, 2025
* winch(aarch64): Sync SP with SSP when dropping stack

This commit is a follow-up to
#10146  and represents
another step toward fixing the remaining issues discovered through spec
tests in the same vein as #10201

Specifically, this commit ensures that the stack pointer is always in
sync with the shadow stack pointer. The previous approach was lossy
because it only performed the sync when reserving stack space. While
this approach worked in some cases, it failed to account for situations
where the shadow stack pointer might be adjusted and aligned for calls.
As a result, the stack pointer could become unaligned when claiming
stack space, leading to issues at call sites.

It is possible to avoid the unconditional move and perform it only when
alignment is needed, i.e., at call sites and when the real stack pointer
is unaligned. However, as of now, the simplest solution is to always
perform the sync, which integrates best with the current infrastructure.

* Update disassembly tests
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.

2 participants