This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Add support for the SEH opcode for return address signing
ClosedPublic

Authored by mstorsjo on Oct 5 2022, 8:45 AM.

Details

Summary

See https://github.com/MicrosoftDocs/cpp-docs/pull/4202 for
progress on trying to get it documented upstream.

The external facing assembler directive ".seh_sign_ra" should be
considered experimental until proper naming for the opcode has been
settled upstream.

This opcode can be expressed in packed unwind too, but I'm not
implementing that packing until documentation for these cases have
settled.

Diff Detail

Event Timeline

mstorsjo created this revision.Oct 5 2022, 8:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 5 2022, 8:45 AM
mstorsjo requested review of this revision.Oct 5 2022, 8:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 5 2022, 8:45 AM

@efriedma - Do you have access to Windows running on HW that actually supports the PAC instructions? Can you check what happens when unwinding through a call frame that is built with PAC enabled today (where the unwind info lacks the opcode that corresponds to the signing) - does the unwind fail, or does the unwinder leniently strip out the pointer authentication before stepping to the next function?

As far as I know, there isn't any publicly released hardware running Windows that supports PAC.

As far as I know, there isn't any publicly released hardware running Windows that supports PAC.

Yeah, that's my understanding too.

(I kinda wondered that you might have access to such HW though, but if you have, you're of course also not allowed to comment on it. Although for this case, it wouldn't really reveal anything about the HW itself, other than that it exists, only about the Windows unwinder implementation.)

The reason I'm asking is for assessing the impact of the bug that we're currently willingly generated PAC instructions without matching unwind info. In https://code.videolan.org/videolan/vlc/-/merge_requests/2662 I suggested stopping using -mbranch-protection=standard for Windows targets (so current binaries won't start misbehaving on future HW) until this is implemented properly in LLVM. However, I'm getting counterarguments for preferring keeping the added security (even though no current HW benefits from it, only future HW), and that the lacking unwind info shouldn't have any notable runtime effect.

I guess if you run Windows in a virtual machine on a computer with PAC instructions (like a Mac), you might theoretically have access to them? I don't currently have a setup like that, though.

I guess if you run Windows in a virtual machine on a computer with PAC instructions (like a Mac), you might theoretically have access to them? I don't currently have a setup like that, though.

Oh, thanks, that's a good hint, I'll try to dig around to see if I can find someone with such a setup to run some tests for me.

mstorsjo updated this revision to Diff 466190.Oct 7 2022, 2:48 PM

Updated with the real name of the opcode.

I guess if you run Windows in a virtual machine on a computer with PAC instructions (like a Mac), you might theoretically have access to them? I don't currently have a setup like that, though.

Oh, thanks, that's a good hint, I'll try to dig around to see if I can find someone with such a setup to run some tests for me.

I did get hold of a person with such a setup, but apparently either the PAC extensions weren't visible in the VM guest, or the OS hadn't set up the signing keys, so after either paciasp or pacibsp, the return address was unchanged - so the experiment didn't teach us anything about how this bug affects code on future devices. :-( (I did verify that the same test binaries do show the expected changes if I run them in Wine on Linux on Graviton 3 though.)

I guess if you run Windows in a virtual machine on a computer with PAC instructions (like a Mac), you might theoretically have access to them? I don't currently have a setup like that, though.

Oh, thanks, that's a good hint, I'll try to dig around to see if I can find someone with such a setup to run some tests for me.

I did get hold of a person with such a setup, but apparently either the PAC extensions weren't visible in the VM guest, or the OS hadn't set up the signing keys, so after either paciasp or pacibsp, the return address was unchanged - so the experiment didn't teach us anything about how this bug affects code on future devices. :-( (I did verify that the same test binaries do show the expected changes if I run them in Wine on Linux on Graviton 3 though.)

The PR to add public docs about this was merged - and I did get a clarification that unwinding through a signed return address without this opcode doesn't work: https://github.com/MicrosoftDocs/cpp-docs/pull/4202#issuecomment-1273849028 So with that settled, any binaries built with current versions of LLVM, with -mbranch-protection=standard (or similar), will be sublty broken in the future once this feature is taken into use, if those binaries expect to do any unwinding.

So in hindsight, it would have been better to have errored out on this combination for previous releases, but there's not much we can do about that now, and sneaking that into 15.0.3 is a bit late too, and the best we can do probably is just to fix this. Too bad that we can't really test run the generated unwind info here for now, but we can at least match MSVC.

mstorsjo updated this revision to Diff 466770.Oct 11 2022, 4:41 AM

Fixed a typo in the testcase.

No other changes were needed on this one now that it is properly documented upstream.

This revision is now accepted and ready to land.Oct 11 2022, 10:19 AM
This revision was landed with ongoing or failed builds.Oct 12 2022, 1:07 AM
This revision was automatically updated to reflect the committed changes.