Wasmtime GitHub notifications bot (May 31 2022 at 13:39): Wasmtime GitHub notifications bot (May 31 2022 at 13:39):akirilov-arm opened PR #4195 from fiber_cfi to main:
Now the fiber implementation on AArch64 authenticates function
return addresses and includes the relevant BTI instructions, except
on macOS.Copyright (c) 2022, Arm Limited.
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
Wasmtime GitHub notifications bot (May 31 2022 at 13:39):akirilov-arm requested alexcrichton for a review on PR #4195.
Wasmtime GitHub notifications bot (May 31 2022 at 13:40):akirilov-arm edited PR #4195 from fiber_cfi to main:
Now the fiber implementation on AArch64 authenticates function return addresses and includes the relevant BTI instructions, except on macOS.
This PR has been spun off from #3606 and fixes #3183.
Wasmtime GitHub notifications bot (May 31 2022 at 14:33):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (May 31 2022 at 14:33):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (May 31 2022 at 14:33):alexcrichton created PR review comment:
Is the "FIXME" here referring to that theoretically we should add like a
FastAppleAarch64convention or something like that? If that's the case it's definitely ok to leave this just as a comment for now, and perosnally I'd be ok leaving this as an implementation detial until someone otherwise proves that it's better to add the fast aarch64 variant.
Wasmtime GitHub notifications bot (May 31 2022 at 14:33):alexcrichton created PR review comment:
Personally I work in assembly so little I end up needing a lot of comments to help guide me through what's happening here. I can sort of piece together what I think is happening here but would you be up for writing a longer-form comment at the top of this file (or somewhere appropriate) about the register authentication bits that are implemented here? (and then refer to that comment from this section)
Wasmtime GitHub notifications bot (May 31 2022 at 14:33):alexcrichton created PR review comment:
To confirm, this is because the macos assembler doesn't have support for the
paciaspinstruction natively? (do you know if there's a version that does support it?)
Wasmtime GitHub notifications bot (May 31 2022 at 14:33):alexcrichton created PR review comment:
Personally I'm pretty unfamiliar with all of this. Would you be up for adding a comment here to why this is necessary and perhaps an online doc if possible explaining the format of this section?
Wasmtime GitHub notifications bot (May 31 2022 at 15:34):akirilov-arm submitted PR review.
Wasmtime GitHub notifications bot (May 31 2022 at 15:34):akirilov-arm created PR review comment:
Yes, that's right. TBH currently the ABI implementation does not really distinguish between calling conventions, i.e. it generates essentially the same code in all cases (except for the unwinding information, as discussed here).
Wasmtime GitHub notifications bot (May 31 2022 at 16:17):akirilov-arm submitted PR review.
Wasmtime GitHub notifications bot (May 31 2022 at 16:17):akirilov-arm created PR review comment:
macOS differs in several details in the way it handles authenticated pointers (e.g. it uses the B key, i.e. the
PACIBSPinstruction in this case), particularly with respect to unwinding, and currently I am unable to tackle the implementation properly, so I have chosen to skip it for the time being. Note that we are actually using aHINTinstruction here (essentially a no-op), which is part of the base 64-bit architecture, so assemblers should not have any issue with it, even if they are really old;PACIASPis just an alias for aHINTwith a particular immediate.
Wasmtime GitHub notifications bot (May 31 2022 at 21:23):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (May 31 2022 at 21:23):alexcrichton created PR review comment:
Oh wow sorry I completely misread "ifndef CFG_TARGET_OS_macos" as "if macos" which is entirely the opposite of what's happening here.
Otherwise those makes sense! Although the same question still somewhat applies, do general assemblers not recognize
paciaspas an instruction? (or is there like some common assembler that only recently got support for such syntax?)
Wasmtime GitHub notifications bot (Jun 01 2022 at 16:12):akirilov-arm submitted PR review.
Wasmtime GitHub notifications bot (Jun 01 2022 at 16:12):akirilov-arm created PR review comment:
Arm actually maintains documentation on the support for various architectural features by the 2 major open source toolchains - here is the page for the GNU toolchain and here is the information for the LLVM one. So, consider Ubuntu 18.04 (LTS release, still supported) - it ships with version 2.30 of Binutils, so it does support pointer authentication (i.e. the
autiasp,pacia1716, andpaciaspinstructions), but it does not support BTI; I am not sure what the case is with Linux distributions with even longer support periods such as RHEL. I have decided to be conservative and use an approach that works as long as the assembler has AArch64 support at all, but I would not mind changing it.
Wasmtime GitHub notifications bot (Jun 01 2022 at 16:25):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Jun 01 2022 at 16:25):cfallin created PR review comment:
At least for
bti, if a currently supported Ubuntu can't assemble the instruction, IMHO it seems like we should probably avoid it for the time being -- we otherwise don't really have stringent version requirements on the system toolchain AFAIK, just on Rust, and I've seen plenty of old Ubuntu (and other distro) installs in production in various places.Could we
#define _bti hint #XXand maybe the PAC instructions too for uniformity?(I'll defer to Alex on this though if he feels differently, he's far more qualified on toolchain questions generally)
Wasmtime GitHub notifications bot (Jun 01 2022 at 17:56):akirilov-arm edited PR #4195 from fiber_cfi to main:
Now the fiber implementation on AArch64 authenticates function return addresses and includes the relevant BTI instructions, except on macOS, following the guidelines presented in the bytecodealliance/rfcs#17 RFC proposal.
This PR has been spun off from #3606 and fixes #3183.
Wasmtime GitHub notifications bot (Jun 01 2022 at 17:57):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Jun 01 2022 at 17:57):alexcrichton created PR review comment:
Oh I'm mostly just curious I don't think any concrete changes are needed. The
// actual-instrcomments are clear enough to me what it's supposed to be. If there's some assembler that needs thehint ...instead of the actual instruction seems fine to leave these in.
Wasmtime GitHub notifications bot (Jun 09 2022 at 13:06):akirilov-arm updated PR #4195 from fiber_cfi to main.
Wasmtime GitHub notifications bot (Jun 09 2022 at 14:17):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Jun 09 2022 at 14:17):alexcrichton merged PR #4195.
Last updated: Nov 22 2024 at 16:03 UTC