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

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

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).

Thanks for the feedback here. So we introduce two options the user is
given control over. I guess this raises two questions concerning design
choices to be made.

1. How do we expose these options to the user, and
2. What should we define as default values?

Regarding defaults, in order to maximize performance my initial guess
would be that we don't push IP to the stack, such that we save on
instructions by omitting pushes and pops and automatically preserve
stack alignment that way.

Even so, that leaves the question of how to best allow users to modify
these parameters prior to compiling the library.

Any thoughts??

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

Agreed.

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

I'm thinking we set the value of PAC_CFI_ADJ contigent on values of
PAC_LEAF_PUSH_SP and PAC_PRESERVE_STACK_ALIGN when HAVE_PAC_LEAF is
set (once I define these new macros, of course).

V.

> R.