wasmtime / issue #6799 Tail calls don't work with AArch64... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / issue #6799 Tail calls don't work with AArch64...

Wasmtime GitHub notifications bot (Aug 03 2023 at 15:12):

alexcrichton opened issue #6799:

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

Wasmtime GitHub notifications bot (Aug 03 2023 at 16:27):

cfallin commented on issue #6799:

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.

Wasmtime GitHub notifications bot (Aug 03 2023 at 16:27):

cfallin edited a comment on issue #6799:

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.

Wasmtime GitHub notifications bot (Aug 03 2023 at 16:37):

alexcrichton commented on issue #6799:

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.

Wasmtime GitHub notifications bot (Aug 03 2023 at 16:45):

cfallin commented on issue #6799:

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?

Wasmtime GitHub notifications bot (Aug 03 2023 at 16:46):

fitzgen commented on issue #6799:

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.

Wasmtime GitHub notifications bot (Aug 03 2023 at 16:48):

fitzgen commented on issue #6799:

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.

Wasmtime GitHub notifications bot (Aug 03 2023 at 16:49):

fitzgen commented on issue #6799:

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?

Wasmtime GitHub notifications bot (Aug 03 2023 at 16:54):

alexcrichton commented on issue #6799:

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 https://github.com/bytecodealliance/wasmtime/pull/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)

Wasmtime GitHub notifications bot (Aug 05 2023 at 16:31):

alexcrichton commented on issue #6799:

I've posted using auti{a,b}z and paci{a,b}z for the tail convention at https://github.com/bytecodealliance/wasmtime/pull/6810

Wasmtime GitHub notifications bot (Aug 09 2023 at 19:14):

alexcrichton closed issue #6799:

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

Last updated: Dec 23 2024 at 13:07 UTC