Re: [PATCH v2 07/10] arm64: asm: Provide a mechanism for generating ELF note for BTI

[Will Deacon <will@kernel.org>]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel