Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tail calls don't work with AArch64 pointer authentication #6799

Closed
alexcrichton opened this issue Aug 3, 2023 · 8 comments · Fixed by #6810
Closed

Tail calls don't work with AArch64 pointer authentication #6799

alexcrichton opened this issue Aug 3, 2023 · 8 comments · Fixed by #6810

Comments

@alexcrichton
Copy link
Member

Currently Wasmtime will enable pointer authentication bits on AArch64 when possible, notably for macOS by default. This does not currently work with tail calls which means that cargo test currently fails with various segfaults.

From what I can understand the general pointer authentication scheme right now is:

  • When a function is entered, the return address, lr, is "encrypted" with both sp and the B key
  • When a function returns, the return address is "decrypted" with sp and the B key

For tail calls this decryption isn't happening currently, meaning that the return address is corrupted for when the tail-called function finally returns.

I've attempted a naive fix for this where I insert an auti*sp instruction before the branch for the ReturnCall instruction, but this does not work because if a function tail calls a function with stack arguments it means that sp is different from the beginning of the function and the end of the function. This means that different values go into the encryption/decryption phases which means that the return address gets corrupted again.

I'm not entirely sure how to fix this myself, but I'm also a little tired right now so may be missing something. cc @fitzgen

@cfallin
Copy link
Member

cfallin commented Aug 3, 2023

Ah, we missed the SP dependence here when thinking about this earlier: we had assumed that moving the return address would be safe.

It looks like we'll need to decrypt and re-encrypt in the tail-call sequence; the pacibsp instruction encrypts the value currently in LR with the value currently in SP and the B key. Or at least, that's what I'm grokking from here.

@alexcrichton
Copy link
Member Author

I was poking at this a bit more and trying to see what LLVM does but I don't think LLVM handles this where it doesn't use a tail call with stack arguments and using __attribute__((musttail)) it says that the signatures must match exactly.

I was also wondering if what you said would work, because wouldn't that have the problem that the stack pointer is temporarily decremented to not encompass the current frame? I'm not sure how that interacts with red zone/etc or whether it's ok for the stack pointer to be temporarily invalid.

Alternatively though, looking at the various instructions, there may be a few other options too:

  • One would be to use pacibz and autibz where the "modifier" is the zero register rather than the stack pointer for tail calls. I'll admit though that I don't fully understand the implications of this and if that makes it too easy to "guess" the return pointer or forge it by accident or something like that. I'm also not sure whether the current pacibsp is required for ABI reasons, but at least from my naive current reading I think that functionally the tail convention could switch to pacibz and autibz functionally at least.
  • Instead of using autibsp the autib instruction could be used which uses a "modifier" in an arbitrary register. That way the old stack pointer could be calculated in a non-stack-pointer register (e.g. the fp+16) and the original pacibsp would be paired with autib $tmp or something like that.

@cfallin
Copy link
Member

cfallin commented Aug 3, 2023

Ah, yeah, I was forgetting that on aarch64 the return address goes in LR, not on the stack, at the call boundary. And is unencrypted in LR; encrypted only by the prologue as it stores the address to the stack.

That tells me that the autibsp approach with SP at the right spot, or autib as you suggest, should work? It also raises another possibility maybe: could we just... not encrypt in the prologue of a function that does tail calls?

@fitzgen
Copy link
Member

fitzgen commented Aug 3, 2023

I was poking at this a bit more and trying to see what LLVM does but I don't think LLVM handles this where it doesn't use a tail call with stack arguments and using __attribute__((musttail)) it says that the signatures must match exactly.

The signatures-must-match thing is for the regular calling conventions. It has a bunch of other calling conventions designed to support tail calls (used by like ghc and such) that don't have this restriction. But also I wouldn't be surprised if those conventions just don't support pointer auth.

@fitzgen
Copy link
Member

fitzgen commented Aug 3, 2023

I'm a bit surprised that we didn't catch this earlier, as I made sure to have our filetests enable pointer auth for the tail calls runtests, and everything passed so I assumed everything was Just Working.

@fitzgen
Copy link
Member

fitzgen commented Aug 3, 2023

So does that mean that qemu isn't doing the pointer auth for our runtests that claim to enable pointer auth? Or do I have some other misunderstanding here?

@alexcrichton
Copy link
Member Author

could we just... not encrypt in the prologue of a function that does tail calls?

While possible I think the {autib,pacib}z paired instructions may be better to switch to as they do a little bit of pointer authentication but just not maximally so. They should be a workable drop-in replacement when using the tail convention in Cranelift I think to ensure all functions have pointer auth right.

I'm a bit surprised that we didn't catch this earlier

That's probably because we don't have pointer authentication enabled in QEMU so AFAIK the only way to run into this is on macOS aarch64 hardware. I don't run the cranelift tests that often locally so I first ran into it recently after #6774.

I believe at the time of the original writing QEMU didn't support pointer authentication (or something like that). If it does support it now, which it may, then we'll want to configure it to turn that on and it should get auto-detected with /proc/cpuinfo support I believe. (or similar)

alexcrichton added a commit to alexcrichton/wasmtime that referenced this issue Aug 5, 2023
This commit fixes an issue where a `return_call` would not decrypt the
return address when pointer authentication is enabled. The return
address would be encrypted upon entry into the function but would never
get restored later on.

This addresses the issue by changing the encryption keys in use from the
A/B key plus SP to instead using A/B plus the zero key. The reason for
this is that during a normal function call before returning the SP value
is guaranteed to be the same as it was upon entry. For tail calls though
SP may be different due to differing numbers of stack arguments. This
means that the modifier of SP can't be used for the tail convention.

New `APIKey` definitions were added and that now additionally represents
the A/B key plus the modifier. Non-`tail` calling conventions still use
the same keys as before, it's just the `tail` convention that's different.

Closes bytecodealliance#6799
@alexcrichton
Copy link
Member Author

I've posted using auti{a,b}z and paci{a,b}z for the tail convention at #6810

github-merge-queue bot pushed a commit that referenced this issue Aug 9, 2023
…6810)

* aarch64: Fix `return_call`'s interaction with pointer authentication

This commit fixes an issue where a `return_call` would not decrypt the
return address when pointer authentication is enabled. The return
address would be encrypted upon entry into the function but would never
get restored later on.

This addresses the issue by changing the encryption keys in use from the
A/B key plus SP to instead using A/B plus the zero key. The reason for
this is that during a normal function call before returning the SP value
is guaranteed to be the same as it was upon entry. For tail calls though
SP may be different due to differing numbers of stack arguments. This
means that the modifier of SP can't be used for the tail convention.

New `APIKey` definitions were added and that now additionally represents
the A/B key plus the modifier. Non-`tail` calling conventions still use
the same keys as before, it's just the `tail` convention that's different.

Closes #6799

* Fix tests

* Fix test expectations

* Allow `sign_return_address` at all times in filetests
eduardomourar pushed a commit to eduardomourar/wasmtime that referenced this issue Aug 18, 2023
…ytecodealliance#6810)

* aarch64: Fix `return_call`'s interaction with pointer authentication

This commit fixes an issue where a `return_call` would not decrypt the
return address when pointer authentication is enabled. The return
address would be encrypted upon entry into the function but would never
get restored later on.

This addresses the issue by changing the encryption keys in use from the
A/B key plus SP to instead using A/B plus the zero key. The reason for
this is that during a normal function call before returning the SP value
is guaranteed to be the same as it was upon entry. For tail calls though
SP may be different due to differing numbers of stack arguments. This
means that the modifier of SP can't be used for the tail convention.

New `APIKey` definitions were added and that now additionally represents
the A/B key plus the modifier. Non-`tail` calling conventions still use
the same keys as before, it's just the `tail` convention that's different.

Closes bytecodealliance#6799

* Fix tests

* Fix test expectations

* Allow `sign_return_address` at all times in filetests
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 a pull request may close this issue.

3 participants