| Message ID | gkrmtd2964g.fsf_-_@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On 21/07/2022 10:17, Andrea Corallo via Gcc-patches wrote: > Richard Earnshaw <Richard.Earnshaw@foss.arm.com> writes: > >> On 28/04/2022 10:48, Andrea Corallo via Gcc-patches wrote: >>> This change add bti instructions at the beginning of arm specific >>> libgcc hand written assembly routines. >>> 2022-03-31 Andrea Corallo <andrea.corallo@arm.com> >>> * libgcc/config/arm/crti.S (FUNC_START): Add bti instruction >>> if >>> necessary. >>> * libgcc/config/arm/lib1funcs.S (THUMB_FUNC_START, FUNC_START): >>> Likewise. >>> >> >> +#if defined(__ARM_FEATURE_BTI) >> >> Wouldn't it be better to use __ARM_FEATURE_BTI_DEFAULT? That way we >> only get BTI instructions in multilib variants that have asked for >> BTI. >> >> R. > > Hi Richard, > > good point, yes I think so. > > Please find attached the updated patch. > > BR > > Andrea > I've been pondering this patch. The way it is implemented would put a BTI instruction at the start of every assembler routine in libgcc. But the vast majority of functions in libgcc cannot have their address taken, so a BTI isn't needed (BTI is only needed when an indirect jump could be used). So I wonder if we really need to do this so aggressively? Perhaps a better approach would be to define a macro (eg MAYBEBTI) which expands a BTI if the compilation requires it and nothing otherwise), and then manually insert that in any functions that really need this (if any). R.
Richard Earnshaw <Richard.Earnshaw@foss.arm.com> writes: > On 21/07/2022 10:17, Andrea Corallo via Gcc-patches wrote: >> Richard Earnshaw <Richard.Earnshaw@foss.arm.com> writes: >> >>> On 28/04/2022 10:48, Andrea Corallo via Gcc-patches wrote: >>>> This change add bti instructions at the beginning of arm specific >>>> libgcc hand written assembly routines. >>>> 2022-03-31 Andrea Corallo <andrea.corallo@arm.com> >>>> * libgcc/config/arm/crti.S (FUNC_START): Add bti instruction >>>> if >>>> necessary. >>>> * libgcc/config/arm/lib1funcs.S (THUMB_FUNC_START, FUNC_START): >>>> Likewise. >>>> >>> >>> +#if defined(__ARM_FEATURE_BTI) >>> >>> Wouldn't it be better to use __ARM_FEATURE_BTI_DEFAULT? That way we >>> only get BTI instructions in multilib variants that have asked for >>> BTI. >>> >>> R. >> Hi Richard, >> good point, yes I think so. >> Please find attached the updated patch. >> BR >> Andrea >> > > I've been pondering this patch. The way it is implemented would put a > BTI instruction at the start of every assembler routine in libgcc. > But the vast majority of functions in libgcc cannot have their address > taken, so a BTI isn't needed (BTI is only needed when an indirect jump > could be used). So I wonder if we really need to do this so > aggressively? > > Perhaps a better approach would be to define a macro (eg MAYBEBTI) > which expands a BTI if the compilation requires it and nothing > otherwise), and then manually insert that in any functions that really > need this (if any). I guess the main downside of this approach would be the maintanace burden, we'll have to remember forever that every time an asm function is called by function pointer we have to add the bti landing pad manually, otherwise this will be broken when pacbti enabled. WDYT? If we want to go this way I'll start reworking the patch in this direction (tho this might not be trivial). BR Andrea
On 22/07/2022 16:09, Andrea Corallo via Gcc-patches wrote: > Richard Earnshaw <Richard.Earnshaw@foss.arm.com> writes: > >> On 21/07/2022 10:17, Andrea Corallo via Gcc-patches wrote: >>> Richard Earnshaw <Richard.Earnshaw@foss.arm.com> writes: >>> >>>> On 28/04/2022 10:48, Andrea Corallo via Gcc-patches wrote: >>>>> This change add bti instructions at the beginning of arm specific >>>>> libgcc hand written assembly routines. >>>>> 2022-03-31 Andrea Corallo <andrea.corallo@arm.com> >>>>> * libgcc/config/arm/crti.S (FUNC_START): Add bti instruction >>>>> if >>>>> necessary. >>>>> * libgcc/config/arm/lib1funcs.S (THUMB_FUNC_START, FUNC_START): >>>>> Likewise. >>>>> >>>> >>>> +#if defined(__ARM_FEATURE_BTI) >>>> >>>> Wouldn't it be better to use __ARM_FEATURE_BTI_DEFAULT? That way we >>>> only get BTI instructions in multilib variants that have asked for >>>> BTI. >>>> >>>> R. >>> Hi Richard, >>> good point, yes I think so. >>> Please find attached the updated patch. >>> BR >>> Andrea >>> >> >> I've been pondering this patch. The way it is implemented would put a >> BTI instruction at the start of every assembler routine in libgcc. >> But the vast majority of functions in libgcc cannot have their address >> taken, so a BTI isn't needed (BTI is only needed when an indirect jump >> could be used). So I wonder if we really need to do this so >> aggressively? >> >> Perhaps a better approach would be to define a macro (eg MAYBEBTI) >> which expands a BTI if the compilation requires it and nothing >> otherwise), and then manually insert that in any functions that really >> need this (if any). > > I guess the main downside of this approach would be the maintanace > burden, we'll have to remember forever that every time an asm function > is called by function pointer we have to add the bti landing pad > manually, otherwise this will be broken when pacbti enabled. WDYT? > > If we want to go this way I'll start reworking the patch in this > direction (tho this might not be trivial). > Yes, it's a trade-off. The lazy way, however, costs all users even if a function is never addressed (which I think is the case for practically all functions in libgcc). So I think in this case it's worth taking that extra development pain. R. > BR > > Andrea
Richard Earnshaw <Richard.Earnshaw@foss.arm.com> writes: > On 22/07/2022 16:09, Andrea Corallo via Gcc-patches wrote: >> Richard Earnshaw <Richard.Earnshaw@foss.arm.com> writes: >> >>> On 21/07/2022 10:17, Andrea Corallo via Gcc-patches wrote: >>>> Richard Earnshaw <Richard.Earnshaw@foss.arm.com> writes: >>>> >>>>> On 28/04/2022 10:48, Andrea Corallo via Gcc-patches wrote: >>>>>> This change add bti instructions at the beginning of arm specific >>>>>> libgcc hand written assembly routines. >>>>>> 2022-03-31 Andrea Corallo <andrea.corallo@arm.com> >>>>>> * libgcc/config/arm/crti.S (FUNC_START): Add bti instruction >>>>>> if >>>>>> necessary. >>>>>> * libgcc/config/arm/lib1funcs.S (THUMB_FUNC_START, FUNC_START): >>>>>> Likewise. >>>>>> >>>>> >>>>> +#if defined(__ARM_FEATURE_BTI) >>>>> >>>>> Wouldn't it be better to use __ARM_FEATURE_BTI_DEFAULT? That way we >>>>> only get BTI instructions in multilib variants that have asked for >>>>> BTI. >>>>> >>>>> R. >>>> Hi Richard, >>>> good point, yes I think so. >>>> Please find attached the updated patch. >>>> BR >>>> Andrea >>>> >>> >>> I've been pondering this patch. The way it is implemented would put a >>> BTI instruction at the start of every assembler routine in libgcc. >>> But the vast majority of functions in libgcc cannot have their address >>> taken, so a BTI isn't needed (BTI is only needed when an indirect jump >>> could be used). So I wonder if we really need to do this so >>> aggressively? >>> >>> Perhaps a better approach would be to define a macro (eg MAYBEBTI) >>> which expands a BTI if the compilation requires it and nothing >>> otherwise), and then manually insert that in any functions that really >>> need this (if any). >> I guess the main downside of this approach would be the maintanace >> burden, we'll have to remember forever that every time an asm function >> is called by function pointer we have to add the bti landing pad >> manually, otherwise this will be broken when pacbti enabled. WDYT? >> If we want to go this way I'll start reworking the patch in this >> direction (tho this might not be trivial). >> > > Yes, it's a trade-off. The lazy way, however, costs all users even if > a function is never addressed (which I think is the case for > practically all functions in libgcc). > > So I think in this case it's worth taking that extra development pain. > > R. As a late follow-up to this. I believe there are no hand written asm functions in libgcc that are addressed, so this patch was dropped from the series in the following iteration. It is true that we could pac instrument them but ATM we don't. Andrea
diff --git a/libgcc/config/arm/crti.S b/libgcc/config/arm/crti.S index 0192972a7e6..4098353af1c 100644 --- a/libgcc/config/arm/crti.S +++ b/libgcc/config/arm/crti.S @@ -51,7 +51,9 @@ .macro FUNC_START #ifdef __thumb__ .thumb - +#if defined(__ARM_FEATURE_BTI_DEFAULT) + bti +#endif push {r3, r4, r5, r6, r7, lr} #else .arm diff --git a/libgcc/config/arm/lib1funcs.S b/libgcc/config/arm/lib1funcs.S index 8c39c9f20a2..de98edcc300 100644 --- a/libgcc/config/arm/lib1funcs.S +++ b/libgcc/config/arm/lib1funcs.S @@ -345,6 +345,9 @@ LSYM(Ldiv0): TYPE (\name) .thumb_func SYM (\name): +#if defined(__ARM_FEATURE_BTI_DEFAULT) + bti +#endif .endm /* Function start macros. Variants for ARM and Thumb. */ @@ -372,6 +375,9 @@ SYM (\name): THUMB_FUNC THUMB_SYNTAX SYM (__\name): +#if defined(__ARM_FEATURE_BTI_DEFAULT) + bti +#endif .endm .macro ARM_SYM_START name