Re: [PATCH v5a 8/8] newlib: libc: setjmp M-profile PACBTI-enablement

On 09/01/2023 19:30, Victor L. Do Nascimento wrote:
> This patch revision provides the following improvements to proposed
> patch implementation:
>    * Load ip directly when restoring PAC code from stack for
>    authentication in longjmp epilogue.
>    * Improve code readability by defining the REGS_SAVE_SIZE macro to
>    map onto the otherwise obscure 104 constant used in str/ldr
>    instructions.
> 
> Add function prologue/epilogue to conditionally add BTI landing pads
> and/or PAC code generation & authentication instructions depending on
> compilation flags.
> ---
>   newlib/libc/machine/arm/setjmp.S | 43 ++++++++++++++++++++++++++++++++
>   1 file changed, 43 insertions(+)
> 
> diff --git a/newlib/libc/machine/arm/setjmp.S b/newlib/libc/machine/arm/setjmp.S
> index d814afea8..820230458 100644
> --- a/newlib/libc/machine/arm/setjmp.S
> +++ b/newlib/libc/machine/arm/setjmp.S
> @@ -22,6 +22,11 @@
>   #define SIZE(x)
>   #endif
>   
> +/* Jump buffer allocation sizes.  */
> +#define CORE_REGS_SAVE_SIZE 40
> +#define FP_REGS_SAVE_SIZE 64
> +#define REGS_SAVE_SIZE (CORE_REGS_SAVE_SIZE + FP_REGS_SAVE_SIZE)

Nearly there, but I think the names could be a bit clearer.  I'd suggest:

JMPBUF_CORE_REGS_SIZE
JMPBUF_FP_REGS_SIZE
JMPBUF_PAC

I'd also write the values as the number of regs saved multiplied by the 
size of a register (in bytes), so (10 * 4) and (8 * 8) for the sizes and 
finally write the last one as

	(JMPBUF_CORE_REGS_SIZE + JMPBUF_FP_REGS_SIZE + 0)

the "+ 0" to make it clear that it's after the other registers; if we 
ever need to add more registers in future they can then follow the same 
style.

R.

> +
>   /* Arm/Thumb interworking support:
>   
>      The interworking scheme expects functions to use a BX instruction
> @@ -155,6 +160,8 @@ SYM (.arm_start_of.\name):
>   	.align 2
>   	MODE
>   	.globl SYM (\name)
> +	.fnstart
> +	.cfi_startproc
>   	TYPE (\name)
>   SYM (\name):
>   	PROLOGUE \name
> @@ -162,6 +169,8 @@ SYM (\name):
>   
>   .macro FUNC_END name
>   	RET
> +	.cfi_endproc
> +	.fnend
>   	SIZE (\name)
>   .endm
>   
> @@ -171,6 +180,21 @@ SYM (\name):
>   
>   	FUNC_START setjmp
>   
> +#if __ARM_FEATURE_PAC_DEFAULT
> +# if __ARM_FEATURE_BTI_DEFAULT
> +	pacbti	ip, lr, sp
> +# else
> +	pac	ip, lr, sp
> +# endif /* __ARM_FEATURE_BTI_DEFAULT */
> +	mov r3, ip
> +	str r3, [r0, #REGS_SAVE_SIZE]
> +	.cfi_register 143, 12
> +#else
> +# if __ARM_FEATURE_BTI_DEFAULT
> +	bti
> +# endif /* __ARM_FEATURE_BTI_DEFAULT */
> +#endif /* __ARM_FEATURE_PAC_DEFAULT */
> +
>   	/* Save all the callee-preserved registers into the jump buffer.  */
>   #ifdef __thumb2__
>   	mov		ip, sp
> @@ -184,6 +208,10 @@ SYM (\name):
>   
>   	/* When setting up the jump buffer return 0.  */
>   	mov		r0, #0
> +#if __ARM_FEATURE_PAC_DEFAULT
> +	mov ip, r3
> +	aut ip, lr, sp
> +#endif /* __ARM_FEATURE_PAC_DEFAULT */
>   
>   	FUNC_END setjmp
>   
> @@ -193,6 +221,16 @@ SYM (\name):
>   
>   	FUNC_START longjmp
>   
> +#if __ARM_FEATURE_BTI_DEFAULT
> +	bti
> +#endif /* __ARM_FEATURE_BTI_DEFAULT */
> +
> +#if __ARM_FEATURE_PAC_DEFAULT
> +	/* Keep original jmpbuf address for retrieving pac-code
> +	   for authentication.  */
> +	mov	r2, r0
> +#endif /* __ARM_FEATURE_PAC_DEFAULT */
> +
>   	/* If we have stack extension code it ought to be handled here.  */
>   
>   	/* Restore the registers, retrieving the state when setjmp() was called.  */
> @@ -212,5 +250,10 @@ SYM (\name):
>   	it		eq
>   	moveq		r0, #1
>   
> +#if __ARM_FEATURE_PAC_DEFAULT
> +	ldr ip, [r2, #REGS_SAVE_SIZE]
> +	aut ip, lr, sp
> +#endif /* __ARM_FEATURE_PAC_DEFAULT */
> +
>   	FUNC_END longjmp
>   #endif