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

Richard Earnshaw <[email protected]> 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.

>> +	.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.
>
> R.