Richard Earnshaw <richard.earns...@foss.arm.com> writes:
> On 09/12/2021 17:36, Andrea Corallo via Gcc-patches wrote:
>> Richard Earnshaw via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
>>
>>> On 28/10/2021 12:43, Tejas Belagod via Gcc-patches wrote:
>>>>
>>>>> -----Original Message-----
>>>>> From: Gcc-patches <gcc-patches-
>>>>> bounces+belagod=gcc.gnu....@gcc.gnu.org> On Behalf Of Tejas Belagod via
>>>>> Gcc-patches
>>>>> Sent: Friday, October 8, 2021 1:18 PM
>>>>> To: gcc-patches@gcc.gnu.org
>>>>> Subject: [Patch 5/7, Arm. GCC] Add pointer authentication for stack-
>>>>> unwinding runtime.
>>>>>
>>>>> Hi,
>>>>>
>>>>> This patch adds authentication for when the stack is unwound when an
>>>>> exception is taken. All the changes here are done to the runtime code in
>>>>> libgcc's unwinder code for Arm target. All the changes are guarded under
>>>>> defined (__ARM_FEATURE_PAC_DEFAULT) and activates only if the +pacbti
>>>>> feature is switched on for the architecture. This means that switching on
>>>>> the
>>>>> target feature via -march or -mcpu is sufficient and -mbranch-protection
>>>>> need not be enabled. This ensures that the unwinder is authenticated only
>>>>> if
>>>>> the PACBTI instructions are available in the non-NOP space as it uses
>>>>> AUTG.
>>>>> Just generating PAC/AUT instructions using -mbranch-protection will not
>>>>> enable authentication on the unwinder.
>>>>>
>>>>> Tested on arm-none-eabi. OK for trunk?
>>>>>
>>>>> 2021-10-04 Tejas Belagod <tbela...@arm.com>
>>>>>
>>>>> gcc/ChangeLog:
>>>>>
>>>>> * ginclude/unwind-arm-common.h (_Unwind_VRS_RegClass):
>>>>> Introduce
>>>>> new pseudo register class _UVRSC_PAC.
>>>>> * libgcc/config/arm/pr-support.c (__gnu_unwind_execute): Decode
>>>>> exception opcode (0xb4) for saving RA_AUTH_CODE and
>>>>> authenticate
>>>>> with AUTG if found.
>>>>> * libgcc/config/arm/unwind-arm.c (struct pseudo_regs): New.
>>>>> (phase1_vrs): Introduce new field to store pseudo-reg state.
>>>>> (phase2_vrs): Likewise.
>>>>> (_Unwind_VRS_Get): Load pseudo register state from virtual reg set.
>>>>> (_Unwind_VRS_Set): Store pseudo register state to virtual reg set.
>>>>> (_Unwind_VRS_Pop): Load pseudo register value from stack into
>>>>> VRS.
>>>> Rebased and respin based on reviews for previous patches.
>>>> This patch adds authentication for when the stack is unwound when
>>>> an exception is taken. All the changes here are done to the runtime
>>>> code in libgcc's unwinder code for Arm target. All the changes are
>>>> guarded under defined (__ARM_FEATURE_PAUTH) and activates only
>>>> if the +pacbti feature is switched on for the architecture. This means
>>>> that switching on the target feature via -march or -mcpu is sufficient
>>>> and -mbranch-protection need not be enabled. This ensures that the
>>>> unwinder is authenticated only if the PACBTI instructions are available
>>>> in the non-NOP space as it uses AUTG. Just generating PAC/AUT instructions
>>>> using -mbranch-protection will not enable authentication on the unwinder.
>>>> 2021-10-25 Tejas Belagod <tbela...@arm.com>
>>>> gcc/ChangeLog:
>>>> * ginclude/unwind-arm-common.h (_Unwind_VRS_RegClass):
>>>> Introduce
>>>> new pseudo register class _UVRSC_PAC.
>>>> * libgcc/config/arm/pr-support.c (__gnu_unwind_execute): Decode
>>>> exception opcode (0xb4) for saving RA_AUTH_CODE and authenticate
>>>> with AUTG if found.
>>>> * libgcc/config/arm/unwind-arm.c (struct pseudo_regs): New.
>>>> (phase1_vrs): Introduce new field to store pseudo-reg state.
>>>> (phase2_vrs): Likewise.
>>>> (_Unwind_VRS_Get): Load pseudo register state from virtual reg set.
>>>> (_Unwind_VRS_Set): Store pseudo register state to virtual reg set.
>>>> (_Unwind_VRS_Pop): Load pseudo register value from stack into VRS.
>>>> Tested the following configurations, OK for trunk?
>>>> -mthumb/-march=armv8.1-m.main+pacbti/-mfloat-abi=soft
>>>> -marm/-march=armv7-a/-mfpu=vfpv3-d16/-mfloat-abi=softfp
>>>> mcmodel=small and tiny
>>>> aarch64-none-linux-gnu native test and bootstrap
>>>> Thanks,
>>>> Tejas.
>>>>
>>
>>> I'd like to try to get rid of most of the ifdefs from this patch; at
>>> least, it shouldn't be using the ACLE PAUTH feature. The unwinder
>>> should be able to cope with any unwind sequence thrown at it.
>>>
>>> Things are a little more complicated for pointer authentication,
>>> though, because some operations in the main code constructing the
>>> frame may be using architectural NOP instructions, while the unwinder
>>> cannot do the validation using only the architectural NOPs.
>>>
>>> So we need a fall-back: if the unwinder is built without the PAUTH
>>> feature it needs to unwind the pauth frames without the additional
>>> validation (but it still needs to be able to handle them).
>>>
>>> So the only remaining question is whether the additional support
>>> should only be enabled for M-profile targets, or whether we should
>>> just put this code into all builds of the unwinder. I'm not sure I
>>> have a complete answer to that. My inclination is to put it in
>>> unconditionally - we haven't had conditionals for any other optional
>>> architecture feature before. If something similar is added for
>>> A/R-profiles, then either we will share the code exactly, or we'll end
>>> up with a different unwind code to use as a suitable discriminator.
>>>
>>> R.
>> Hi Richard,
>> thanks for reviewing.
>> The attached patch implements what I think we want. That unwinders
>> is
>> always able to unwind the stack but will perform authentication only if
>> built with PACBTI support.
>> WDYT?
>> Thanks
>> Andrea
>>
>
>
> @@ -114,6 +115,22 @@ __gnu_unwind_execute (_Unwind_Context * context,
> __gnu_unwind_state * uws)
> op = next_unwind_byte (uws);
> if (op == CODE_FINISH)
> {
> + /* When we reach end, we have to authenticate R12 we just
> popped earlier. */
> + if (set_pac)
> + {
> +#if defined(TARGET_HAVE_PACBTI)
> + _uw sp;
> + _uw lr;
> + _uw pac;
> + _Unwind_VRS_Get (context, _UVRSC_CORE, R_SP, _UVRSD_UINT32, &sp);
> + _Unwind_VRS_Get (context, _UVRSC_CORE, R_LR, _UVRSD_UINT32, &lr);
> + _Unwind_VRS_Get (context, _UVRSC_PAC, R_IP,
> + _UVRSD_UINT32, &pac);
> + __asm__ __volatile__
> + ("autg %0, %1, %2" : : "r"(pac), "r"(lr), "r"(sp) ;
> +#endif
> + }
> +
>
> You would be better moving the ifdef outside of the 'if (set_pac)'
> clause, which becomes empty otherwise. Also, I think a comment here
> is warranted that, while the check provides additional security
> against a corrupted unwind chain, it isn't essential for correct
> unwinding of an uncorrupted chain.
>
> Otherwise, this is ok.
Will do thanks.
Andrea
