Re: [PATCH v2 1/8] newlib: libc: define M-profile PACBTI-enablement macros

["Victor L. Do Nascimento" <victor.donascimento@arm.com>]

"Victor L. Do Nascimento" <victor.donascimento@arm.com> writes:

> Richard Earnshaw <Richard.Earnshaw@foss.arm.com> writes:
>
>> On 03/08/2022 16:35, Victor Do Nascimento wrote:
>>> Create an assembly header file that conditionally defines fuction
>>> prologues/epilogues depending on the compile-time mbranch-protection
>>> argument values.
>>> * newlib/libc/machine/arm/pacbti.h: New.
>>> ---
>>>   newlib/libc/machine/arm/pacbti.h | 64 ++++++++++++++++++++++++++++++++
>>>   1 file changed, 64 insertions(+)
>>>   create mode 100644 newlib/libc/machine/arm/pacbti.h
>>> diff --git a/newlib/libc/machine/arm/pacbti.h
>>> b/newlib/libc/machine/arm/pacbti.h
>>> new file mode 100644
>>> index 000000000..4c31d00bc
>>> --- /dev/null
>>> +++ b/newlib/libc/machine/arm/pacbti.h
>>> @@ -0,0 +1,64 @@
>>> +/*
>>> + * Copyright (c) 2022 Arm Ltd
>>> + * All rights reserved.
>>> + *
>>> + * Redistribution and use in source and binary forms, with or without
>>> + * modification, are permitted provided that the following conditions
>>> + * are met:
>>> + * 1. Redistributions of source code must retain the above copyright
>>> + *    notice, this list of conditions and the following disclaimer.
>>> + * 2. Redistributions in binary form must reproduce the above copyright
>>> + *    notice, this list of conditions and the following disclaimer in the
>>> + *    documentation and/or other materials provided with the distribution.
>>> + * 3. The name of the company may not be used to endorse or promote
>>> + *    products derived from this software without specific prior written
>>> + *    permission.
>>> + *
>>> + * THIS SOFTWARE IS PROVIDED BY ARM LTD ``AS IS'' AND ANY EXPRESS OR IMPLIED
>>> + * WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
>>> + * MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
>>> + * IN NO EVENT SHALL ARM LTD BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
>>> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED
>>> + * TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
>>> + * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
>>> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
>>> + * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
>>> + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>>> + */
>>> +
>>> +/* Checki whether leaf function PAC signing has been requested
>>> +   in the -mbranch-protect compile-time option.  */
>>> +#define LEAF_PROTECT_BIT 2
>>> +#define __HAVE_PAC_LEAF \
>>> +	__ARM_FEATURE_PAC_DEFAULT & (1 << LEAF_PROTECT_BIT)
>>
>> Either this header needs to avoid polluting the user namespace, or it
>> doesn't.  If it does, then LEAF_PROTECT_BIT fails to do that.  If it
>> doesn't then __HAVE_PAC_LEAF should really be HAVE_PAC_LEAF (to avoid
>> polluting the reserved namespace.  I suspect this header is private to
>> newlib (won't be exported to users), so should not be prefixing names
>> with __.
>>
>>> +
>>> +/* Macro to handle function entry depending on branch-protection
>>> +   schemes.  */
>>> +	.macro pacbti_prologue
>>> +#if __HAVE_PAC_LEAF
>>> +#if __ARM_FEATURE_BTI_DEFAULT
>>> +	pacbti ip, lr, sp
>>> +#else
>>> +	pac ip, lr, sp
>>> +#endif /* __ARM_FEATURE_BTI_DEFAULT */
>>> +	.cfi_register 143, 12
>>> +	str ip, [sp, #-4]!
>>
>> This causes the stack to lose 8-byte alignment.  Whilst for leaf
>> functions that's probably not a problem, the macro should have an
>> option where the user can ask for alignment to be preserved.  But
>> there should also be an option to not save IP on the stack at all (for
>> when the user does not modify IP in the function body).
>
> Work is ongoing on implementing the macro with all necessary features.
>
> A better macro implementation for the prologue is given as follows (but
> still lacks any alignment preservation logic):
>
> /* Emit .cfi_offset directives for a consecutive sequence of registers.
>    PAC_CFI_ADJ will be 4 bytes if IP reg is pushed. */
> 	.macro cfisavelist first, last, index=1
> 	.cfi_offset \last, -4*(\index) - PAC_CFI_ADJ
> 	.if \last-\first
> 	cfisavelist \first, \last-1, \index+1
> 	.endif
> 	.endm
>
> /* Create a prologue entry sequence handling PAC/BTI, if required and emitting
>    CFI directives for generated PAC code and any pushed registers.  */
>
> #define NREG ((\last-\first)+1)
> 	
> 	.macro prologue first=-1, last=-1, savepac=PAC_LEAF_PUSH_IP
> #if HAVE_PAC_LEAF
> #if __ARM_FEATURE_BTI_DEFAULT
> 	pacbti	ip, lr, sp
> #else
> 	pac	ip, lr, sp
> #endif /* __ARM_FEATURE_BTI_DEFAULT */
> 	.cfi_register 143, 12
> #else
> #if __ARM_FEATURE_BTI_DEFAULT
> 	bti
> #endif /* __ARM_FEATURE_BTI_DEFAULT */
> #endif /* HAVE_PAC_LEAF */
> 	.if \first != -1
> 	.if \last != -1
> 	.if \savepac
> 	push {r\first-r\last, ip}
> 	.cfi_adjust_cfa_offset NREG*4 + PAC_CFI_ADJ
> 	.cfi_offset 143, -PAC_CFI_ADJ
> 	cfisavelist \first, \last
> 	.else
> 	push {r\first-r\last}
> 	.cfi_adjust_cfa_offset NREG*4
> 	cfisavelist \first, \last
> 	.endif
> 	.else
> 	.if \savepac
> 	push {r\first, ip}
> 	.cfi_adjust_cfa_offset 4 + PAC_CFI_ADJ
> 	.cfi_offset 143, -PAC_CFI_ADJ 
> 	cfisavelist \first, \first
> 	.else // !\savepac
> 	push {r\first}
> 	cfisavelist \first, \first
> 	.endif
> 	.endif
> 	.else // \first == -1
> 	.if \savepac
> 	push {ip}
> 	.cfi_adjust_cfa_offset PAC_CFI_ADJ
> 	.cfi_offset 143, -PAC_CFI_ADJ
> 	.endif
> 	.endif
> 	.endm
>
> The question remains of how I can ammend the register list in the push
> directive when we wish to push an extra dummy register to the stack for
> alignment purposes.
>
> A hypothetical solution (albeit a broken one, as we can't redefine macro
> arguments in the body of the macro) for when pushing an even no. of
> registers + the pac code is:
>
> 	.if \first != -1
> 	.if \last != -1
> 	.if \savepac
>
> +	.if \align_requested && ( ( (NREG+1) % 2 ) != 0 && (\first != 0) )
> +	\first = \first - 1
> +       .endif
>         
>  	push {r\first-r\last, ip}
> 	.cfi_adjust_cfa_offset NREG*4 + PAC_CFI_ADJ
>
> +       .if \align_requested && ( ((NREG+1) % 2 ) != 0 && (\first != 0) )
> +       /* Having pushed the dummy, restore original register range.  */
> +       \first = \first + 1
> +       .endif
>
> 	.cfi_offset 143, -PAC_CFI_ADJ
>  	cfisavelist \first, \last
>
>         .else
>         ...
>
> The logic above seems correct to me, but I need to figure out a way of
> adjusting the value of \first in the least invasive way.
>
> If we emit the push instruction via a separate macro, we could then pass
> the \first argument as \first-1 (e.g. pushmacro \first=\first-1,
> last=last, savepac=1). Even so, creating a macro just to emit the push
> instruction seems overkill. We may end up having way too many macros.
>
> Do you have any insights on how I may be able to fix the above code to
> allow for alignment preservation elegantly within the limitations of
> what assembler macros can do?
>
> Cheers,
> V.
>

I've also been thinking about simplifying the align logic, such that if
we request alignment preservation, it always pushes a dummy register along
w/ the pac code.  This way, we don't preserve alignment, but rather
alignment "parity.

If in the absence of pushing the pac-code a simple push operation would
result in a misaligned stack pointer, I believe that adding the pac code
to the register list should also result in the preservation of
misalignment.

I think that if in a hypothetical situation code gets written such
that the original prologue results in a misaligned stack, it's correct
to assume that the developer is aware of this and will manually
compensate for it later on in the code if alignment is needed.  The
risk I see is that having too much hidden logic in our prologue macro
might result in unforseen bugs for coders with insufficient familiarity
with it.

The alignment preservation feature should behave as follows: "With this
turned on, no perceptable difference is observed in program behaviour
irrespective of the presence/absence of the pac code on the stack."

>>> +	.save {ra_auth_code}
>>> +	.cfi_def_cfa_offset 4
>>> +	.cfi_offset 143, -4
>>> +#elif __ARM_FEATURE_BTI_DEFAULT
>>> +	bti
>>> +#endif /* __HAVE_PAC_LEAF */
>>> +	.endm
>>> +
>>> +/* Macro to handle different branch exchange cases depending on
>>> +   branch-protection schemes.  */
>>> +	.macro pacbti_epilogue
>>> +#if __HAVE_PAC_LEAF
>>> +	ldr ip, [sp], #4
>>> +	.cfi_restore 143
>>> +	.cfi_def_cfa_offset 0
>>> +	aut ip, lr, sp
>>> +#endif /* __HAVE_PAC_LEAF */
>>> +	bx lr
>>> +	.endm
>>
>> I think these macros are really misnamed, firstly, they're only for
>> leaf functions and secondly, they (particularly the epilogue) does
>> something even if PAC is not needed.  So I think they should be
>> renamed as 'leaf_prologue' and 'leaf_epilogue' respectively.
>>
>> In consequence, I think this file should really be merged into the
>> existing arm_asm.h, then we don't need yet another header file.
>>
>> Finally, the header needs to define a (C) macro that defines how much
>> to adjust the CFI offset by for various scenarios:
>> - When PAC is not used
>> - When PAC is used with no alignment
>> - When PAC is used and asked for 8-byte alignment to be preserved.

This creates anoter level of complexity. As you mention that users
should have the option of whether or not to push the pac code onto the stack
when it won't be overwritten over the course of a leaf fuction, the
logic for this C macro will take this into account.  This leads us to
create something as follows:

#if HAVE_PAC_LEAF        /* Pac code requested for leaf functions.  */
# if PAC_LEAF_PUSH_IP    /* Always push pac code to  the stack.  */
#  if ALIGN_PRESERVE
#   define PAC_CFI_ADJ 8
#  else
#   define PAC_CFI_ADJ 4
#  endif
# else
#  define PAC_CFI_ADJ 0
# endif /* PAC_LEAF_PUSH_IP*/
#else
# define PAC_CFI_ADJ 0
#endif /* HAVE_PAC_LEAF */

Thus, if PAC_LEAF_PUSH_IP is not requested, PAC_CFI_ADJ is set to 0.
This creates a problem for functions where pushing the pac code onto
the stack is mandatory, as is the case for functions where the register
storing it will be reused in the body of the function.
In That case, we need a second macro which is independent of the value
of PAC_LEAF_PUSH_IP.  This leads to our second macro to be used for such
functions: 

#if HAVE_PAC_LEAF
# if ALIGN_PRESERVE	
#  define PAC_CFI_ADJ_DEFAULT 8
# else
#  define PAC_CFI_ADJ_DEFAULT 4
# endif
#endif

and developers would have to choose correctly whether to write something
like

       	.cfi_offset <reg>, -(<base_offset>+PAC_CFI_ADJ)
 or
	.cfi_offset <reg>, -(<base_offset>+PAC_CFI_ADJ_DEFAULT)

Does this sound remotely acceptable?

We could use an assembler macro rather than C macro and calculate the
offset based on some passed argument, but either way we find ourselves
catering for a growing number of use cases and with it the complexity of
any solution grows.

I just thought I'd ask what your thoughts were on these matters.
V.

>>
>> R.