| Message ID | 20200429211641.9279-8-broonie@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: BTI kernel and vDSO support | expand |
[+Dave] On Wed, Apr 29, 2020 at 10:16:38PM +0100, Mark Brown wrote: > ELF files built for BTI should have a program property note section which > identifies them as such. The linker expects to find this note in all > object files it is linking into a BTI annotated output, the compiler will > ensure that this happens for C files but for assembler files we need to do > this in the source so provide a macro which can be used for this purpose. > > This is mainly for use in the VDSO which should be a normal ELF shared > library and should therefore include BTI annotations when built for BTI. > > Signed-off-by: Mark Brown <broonie@kernel.org> > --- > arch/arm64/include/asm/assembler.h | 41 ++++++++++++++++++++++++++++++ > 1 file changed, 41 insertions(+) > > diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h > index 0bff325117b4..85a88df2d0fe 100644 > --- a/arch/arm64/include/asm/assembler.h > +++ b/arch/arm64/include/asm/assembler.h > @@ -736,4 +736,45 @@ USER(\label, ic ivau, \tmp2) // invalidate I line PoU > .Lyield_out_\@ : > .endm > > +/* > + * This macro emits a program property note section identifying > + * architecture features which require special handling, mainly for > + * use in assembly files included in the VDSO. > + */ > + > +#ifdef CONFIG_ARM64_BTI_KERNEL > + > +#define NT_GNU_PROPERTY_TYPE_0 5 > +#define GNU_PROPERTY_AARCH64_FEATURE_1_AND 0xc0000000 > + > +#define GNU_PROPERTY_AARCH64_FEATURE_1_BTI (1U << 0) > +#define GNU_PROPERTY_AARCH64_FEATURE_1_PAC (1U << 1) > + > +.macro emit_aarch64_feature_1_and Might be useful to take the features as a macro argument, so we can re-use this when extra features get added in the future. > + .pushsection .note.gnu.property, "a" > + .align 3 > + .long 2f - 1f > + .long 6f - 3f > + .long NT_GNU_PROPERTY_TYPE_0 > +1: .string "GNU" > +2: > + .align 3 > +3: .long GNU_PROPERTY_AARCH64_FEATURE_1_AND > + .long 5f - 4f > +4: > + .long GNU_PROPERTY_AARCH64_FEATURE_1_PAC | \ > + GNU_PROPERTY_AARCH64_FEATURE_1_BTI Hmm. The Linux ABI doc [1] says this field is: unsigned char pr_data[PR_DATASZ]; but the AArch64 PCS [2] says: "It has a single 32-bit value for the pr_data field." What does this mean for endianness? Will [1] https://github.com/hjl-tools/linux-abi/wiki/linux-abi-draft.pdf [2] https://static.docs.arm.com/ihi0056/g/aaelf64.pdf
On Tue, May 05, 2020 at 03:58:59PM +0100, Will Deacon wrote: > [+Dave] > > On Wed, Apr 29, 2020 at 10:16:38PM +0100, Mark Brown wrote: > > ELF files built for BTI should have a program property note section which > > identifies them as such. The linker expects to find this note in all > > object files it is linking into a BTI annotated output, the compiler will > > ensure that this happens for C files but for assembler files we need to do > > this in the source so provide a macro which can be used for this purpose. > > > > This is mainly for use in the VDSO which should be a normal ELF shared > > library and should therefore include BTI annotations when built for BTI. > > > > Signed-off-by: Mark Brown <broonie@kernel.org> > > --- > > arch/arm64/include/asm/assembler.h | 41 ++++++++++++++++++++++++++++++ > > 1 file changed, 41 insertions(+) > > > > diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h > > index 0bff325117b4..85a88df2d0fe 100644 > > --- a/arch/arm64/include/asm/assembler.h > > +++ b/arch/arm64/include/asm/assembler.h > > @@ -736,4 +736,45 @@ USER(\label, ic ivau, \tmp2) // invalidate I line PoU > > .Lyield_out_\@ : > > .endm > > > > +/* > > + * This macro emits a program property note section identifying > > + * architecture features which require special handling, mainly for > > + * use in assembly files included in the VDSO. > > + */ > > + > > +#ifdef CONFIG_ARM64_BTI_KERNEL > > + > > +#define NT_GNU_PROPERTY_TYPE_0 5 > > +#define GNU_PROPERTY_AARCH64_FEATURE_1_AND 0xc0000000 > > + > > +#define GNU_PROPERTY_AARCH64_FEATURE_1_BTI (1U << 0) > > +#define GNU_PROPERTY_AARCH64_FEATURE_1_PAC (1U << 1) > > + > > +.macro emit_aarch64_feature_1_and > > Might be useful to take the features as a macro argument, so we can > re-use this when extra features get added in the future. Probably a good idea, though I hope this doesn't crop up too often... > > + .pushsection .note.gnu.property, "a" > > + .align 3 > > + .long 2f - 1f > > + .long 6f - 3f > > + .long NT_GNU_PROPERTY_TYPE_0 > > +1: .string "GNU" > > +2: > > + .align 3 > > +3: .long GNU_PROPERTY_AARCH64_FEATURE_1_AND > > + .long 5f - 4f > > +4: > > + .long GNU_PROPERTY_AARCH64_FEATURE_1_PAC | \ > > + GNU_PROPERTY_AARCH64_FEATURE_1_BTI > > Hmm. The Linux ABI doc [1] says this field is: > > unsigned char pr_data[PR_DATASZ]; > > but the AArch64 PCS [2] says: > > "It has a single 32-bit value for the pr_data field." > > What does this mean for endianness? I think this means it's poorly specified. The spirit of this is that each property is a container for a random ELF structure, whose elements are encoded with endianness matching that of the ELF file. Because these structures are variably sized, they can't be described properly using a C type. The pseudo-C in [1] is illustrative but a bit of a bodge IMHO. Attempting to do things this way for real would also get you into trouble with strict-aliasing IIUC. This ship has already sailed: someone should check what comes out of cc -mbig-endian -mbranch-protection=standard. (Extra points scored if you can persuade gcc and clang to do something different!) Cheers ---Dave
On Tue, May 05, 2020 at 03:58:59PM +0100, Will Deacon wrote: > On Wed, Apr 29, 2020 at 10:16:38PM +0100, Mark Brown wrote: > > +#define GNU_PROPERTY_AARCH64_FEATURE_1_BTI (1U << 0) > > +#define GNU_PROPERTY_AARCH64_FEATURE_1_PAC (1U << 1) > > +.macro emit_aarch64_feature_1_and > Might be useful to take the features as a macro argument, so we can > re-use this when extra features get added in the future. I was unsure about that - it'd be a bit annoying to have to have all the callers of the macro list things like BTI where > > +3: .long GNU_PROPERTY_AARCH64_FEATURE_1_AND > > + .long 5f - 4f > > +4: > > + .long GNU_PROPERTY_AARCH64_FEATURE_1_PAC | \ > > + GNU_PROPERTY_AARCH64_FEATURE_1_BTI > Hmm. The Linux ABI doc [1] says this field is: > unsigned char pr_data[PR_DATASZ]; > but the AArch64 PCS [2] says: > "It has a single 32-bit value for the pr_data field." > What does this mean for endianness? It's not entirely clear is it? What we're doing here means that we're emitting as a long rather than a character array so the endianness matters. The ABI doc does have language about the elements being "an array of X-byte integers in the format of the target processor" which seems to align with that as well, my impression is that the intention of the ABI doc is that there should be a Processor_Word type corresponding to the Elf_Word type but there isn't so the char arrays are used to handle the word size difference between AArch32 and AArch64. Unless I'm missing something this at least appears to agree with what the compilers (both GCC and clang) are emitting for both big and little endian and what a readelf that understands these is decoding so I think at this point it's de facto the way things are interpreted.
Hi Mark, Dave, On Tue, May 05, 2020 at 06:06:29PM +0100, Mark Brown wrote: > On Tue, May 05, 2020 at 03:58:59PM +0100, Will Deacon wrote: > > On Wed, Apr 29, 2020 at 10:16:38PM +0100, Mark Brown wrote: > > > > +#define GNU_PROPERTY_AARCH64_FEATURE_1_BTI (1U << 0) > > > +#define GNU_PROPERTY_AARCH64_FEATURE_1_PAC (1U << 1) > > > > +.macro emit_aarch64_feature_1_and > > > Might be useful to take the features as a macro argument, so we can > > re-use this when extra features get added in the future. > > I was unsure about that - it'd be a bit annoying to have to have all the > callers of the macro list things like BTI where It just feels inevitable that we'll need to do this at some point! Do you know what is supposed to happen if an object has multiple instances of this property identifying different features? For example, could we do something like: emit_aarch64_feature_1_and_pac_bti emit_aarch64_feature_1_and_whizz emit_aarch64_feature_1_and_bang all of which wrap emit_aarch64_feature_1_and, but result in an object that supports pac, bti, whizz and bang? If we have to merge this stuff in a single .long, then I think we'll probably have to put up with passing in the features as an optional macro argument, which defaults to "all features" if it's omitted. So on top of your patches, we could do: diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h index 85a88df2d0fe..53801250a639 100644 --- a/arch/arm64/include/asm/assembler.h +++ b/arch/arm64/include/asm/assembler.h @@ -750,7 +750,11 @@ USER(\label, ic ivau, \tmp2) // invalidate I line PoU #define GNU_PROPERTY_AARCH64_FEATURE_1_BTI (1U << 0) #define GNU_PROPERTY_AARCH64_FEATURE_1_PAC (1U << 1) -.macro emit_aarch64_feature_1_and +#define GNU_PROPERTY_AARCH64_FEATURE_1_ALL \ + (GNU_PROPERTY_AARCH64_FEATURE_1_BTI | \ + GNU_PROPERTY_AARCH64_FEATURE_1_PAC) + +.macro emit_aarch64_feature_1_and, feat=GNU_PROPERTY_AARCH64_FEATURE_1_ALL .pushsection .note.gnu.property, "a" .align 3 .long 2f - 1f @@ -762,8 +766,13 @@ USER(\label, ic ivau, \tmp2) // invalidate I line PoU 3: .long GNU_PROPERTY_AARCH64_FEATURE_1_AND .long 5f - 4f 4: - .long GNU_PROPERTY_AARCH64_FEATURE_1_PAC | \ - GNU_PROPERTY_AARCH64_FEATURE_1_BTI + /* + * Although the Linux ABI spec describes this as an array of + * unsigned char, the rest of the world (including clang and gcc) + * treat it as a 32-bit value and so no swizzling is required + * when building for big-endian. + */ + .long \feat 5: .align 3 6: @@ -772,7 +781,7 @@ USER(\label, ic ivau, \tmp2) // invalidate I line PoU #else -.macro emit_aarch64_feature_1_and +.macro emit_aarch64_feature_1_and, feat=0 .endm #endif /* CONFIG_ARM64_BTI_KERNEL */ > > > +3: .long GNU_PROPERTY_AARCH64_FEATURE_1_AND > > > + .long 5f - 4f > > > +4: > > > + .long GNU_PROPERTY_AARCH64_FEATURE_1_PAC | \ > > > + GNU_PROPERTY_AARCH64_FEATURE_1_BTI > > > Hmm. The Linux ABI doc [1] says this field is: > > > unsigned char pr_data[PR_DATASZ]; > > > but the AArch64 PCS [2] says: > > > "It has a single 32-bit value for the pr_data field." > > > What does this mean for endianness? > > It's not entirely clear is it? What we're doing here means that we're > emitting as a long rather than a character array so the endianness > matters. The ABI doc does have language about the elements being "an > array of X-byte integers in the format of the target processor" which > seems to align with that as well, my impression is that the intention of > the ABI doc is that there should be a Processor_Word type corresponding > to the Elf_Word type but there isn't so the char arrays are used to > handle the word size difference between AArch32 and AArch64. > > Unless I'm missing something this at least appears to agree with what > the compilers (both GCC and clang) are emitting for both big and little > endian and what a readelf that understands these is decoding so I think > at this point it's de facto the way things are interpreted. As long as the compilers agree with each other and with the way we roll the note ourself, then I think all we should do is add a comment above the .long. Sound reasonable? Will
On Wed, May 06, 2020 at 12:26:33PM +0100, Will Deacon wrote: > On Tue, May 05, 2020 at 06:06:29PM +0100, Mark Brown wrote: > > I was unsure about that - it'd be a bit annoying to have to have all the > > callers of the macro list things like BTI where > It just feels inevitable that we'll need to do this at some point! > Do you know what is supposed to happen if an object has multiple instances > of this property identifying different features? For example, could we > do something like: Regardless of what is supposed to happen my strong suspicion is that we'll have some more. > If we have to merge this stuff in a single .long, then I think we'll > probably have to put up with passing in the features as an optional macro > argument, which defaults to "all features" if it's omitted. So on top of > your patches, we could do: > +#define GNU_PROPERTY_AARCH64_FEATURE_1_ALL \ > + (GNU_PROPERTY_AARCH64_FEATURE_1_BTI | \ > + GNU_PROPERTY_AARCH64_FEATURE_1_PAC) > + > +.macro emit_aarch64_feature_1_and, feat=GNU_PROPERTY_AARCH64_FEATURE_1_ALL Right, I was just expecting to have the ifdefs selecting the flags to emit in the middle of the asm macro definiton rather than separately - I didn't see a huge win in defining a macro with the only user being another macro. I can do something along those lines though. > -.macro emit_aarch64_feature_1_and > +.macro emit_aarch64_feature_1_and, feat=0 > .endm That will result in us emitting the note with no flags set which *should* be totally fine but is a bit unusual and feels like tempting fate.
On Wed, May 06, 2020 at 01:38:55PM +0100, Mark Brown wrote: > On Wed, May 06, 2020 at 12:26:33PM +0100, Will Deacon wrote: > > On Tue, May 05, 2020 at 06:06:29PM +0100, Mark Brown wrote: > > > > I was unsure about that - it'd be a bit annoying to have to have all the > > > callers of the macro list things like BTI where > > > It just feels inevitable that we'll need to do this at some point! > > Do you know what is supposed to happen if an object has multiple instances > > of this property identifying different features? For example, could we > > do something like: > > Regardless of what is supposed to happen my strong suspicion is that > we'll have some more. Yup! :) > > If we have to merge this stuff in a single .long, then I think we'll > > probably have to put up with passing in the features as an optional macro > > argument, which defaults to "all features" if it's omitted. So on top of > > your patches, we could do: > > > +#define GNU_PROPERTY_AARCH64_FEATURE_1_ALL \ > > + (GNU_PROPERTY_AARCH64_FEATURE_1_BTI | \ > > + GNU_PROPERTY_AARCH64_FEATURE_1_PAC) > > + > > +.macro emit_aarch64_feature_1_and, feat=GNU_PROPERTY_AARCH64_FEATURE_1_ALL > > Right, I was just expecting to have the ifdefs selecting the flags to > emit in the middle of the asm macro definiton rather than separately - I > didn't see a huge win in defining a macro with the only user being > another macro. I can do something along those lines though. With my suggestion, we still only have the 'emit_aarch64_feature_1_and' macro, it just provides a way to override the properties if we need that later on. All I'm proposing is adding the optional feat parameter, which defaults to all of the properties we know about. > > -.macro emit_aarch64_feature_1_and > > +.macro emit_aarch64_feature_1_and, feat=0 > > .endm > > That will result in us emitting the note with no flags set which > *should* be totally fine but is a bit unusual and feels like tempting > fate. Nah, that's just the dummy .macro definition. Will
On Wed, May 06, 2020 at 02:44:34PM +0100, Will Deacon wrote: > On Wed, May 06, 2020 at 01:38:55PM +0100, Mark Brown wrote: > > Right, I was just expecting to have the ifdefs selecting the flags to > > emit in the middle of the asm macro definiton rather than separately - I > > didn't see a huge win in defining a macro with the only user being > > another macro. I can do something along those lines though. > With my suggestion, we still only have the 'emit_aarch64_feature_1_and' > macro, it just provides a way to override the properties if we need that > later on. All I'm proposing is adding the optional feat parameter, which > defaults to all of the properties we know about. > > That will result in us emitting the note with no flags set which > > *should* be totally fine but is a bit unusual and feels like tempting > > fate. > Nah, that's just the dummy .macro definition. I see - I had been reading the idea as being to have the macro outside the #ifdef for BTI so that it's usable separately from that and that you'd just not updated the ifdefs while sketching it out. I think I've got a sensible way of achieving that without too much pain though so it should be fine anyway.
diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h index 0bff325117b4..85a88df2d0fe 100644 --- a/arch/arm64/include/asm/assembler.h +++ b/arch/arm64/include/asm/assembler.h @@ -736,4 +736,45 @@ USER(\label, ic ivau, \tmp2) // invalidate I line PoU .Lyield_out_\@ : .endm +/* + * This macro emits a program property note section identifying + * architecture features which require special handling, mainly for + * use in assembly files included in the VDSO. + */ + +#ifdef CONFIG_ARM64_BTI_KERNEL + +#define NT_GNU_PROPERTY_TYPE_0 5 +#define GNU_PROPERTY_AARCH64_FEATURE_1_AND 0xc0000000 + +#define GNU_PROPERTY_AARCH64_FEATURE_1_BTI (1U << 0) +#define GNU_PROPERTY_AARCH64_FEATURE_1_PAC (1U << 1) + +.macro emit_aarch64_feature_1_and + .pushsection .note.gnu.property, "a" + .align 3 + .long 2f - 1f + .long 6f - 3f + .long NT_GNU_PROPERTY_TYPE_0 +1: .string "GNU" +2: + .align 3 +3: .long GNU_PROPERTY_AARCH64_FEATURE_1_AND + .long 5f - 4f +4: + .long GNU_PROPERTY_AARCH64_FEATURE_1_PAC | \ + GNU_PROPERTY_AARCH64_FEATURE_1_BTI +5: + .align 3 +6: + .popsection +.endm + +#else + +.macro emit_aarch64_feature_1_and +.endm + +#endif /* CONFIG_ARM64_BTI_KERNEL */ + #endif /* __ASM_ASSEMBLER_H */
ELF files built for BTI should have a program property note section which identifies them as such. The linker expects to find this note in all object files it is linking into a BTI annotated output, the compiler will ensure that this happens for C files but for assembler files we need to do this in the source so provide a macro which can be used for this purpose. This is mainly for use in the VDSO which should be a normal ELF shared library and should therefore include BTI annotations when built for BTI. Signed-off-by: Mark Brown <broonie@kernel.org> --- arch/arm64/include/asm/assembler.h | 41 ++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)