diff mbox series

[v5,15/39] arm64: idreg-override: Prepare for place relative reloc patching

Message ID 20231124101840.944737-56-ardb@google.com (mailing list archive)
State New, archived
Headers show
Series arm64: Reorganize kernel VA space for LPA2 | expand

Commit Message

Ard Biesheuvel Nov. 24, 2023, 10:18 a.m. UTC 
From: Ard Biesheuvel <ardb@kernel.org>

The ID reg override handling code uses a rather elaborate data structure
that relies on statically initialized absolute address values in pointer
fields. This means that this code cannot run until relocation fixups
have been applied, and this is unfortunate, because it means we cannot
discover overrides for KASLR or LVA/LPA without creating the kernel
mapping and performing the relocations first.

This can be solved by switching to place-relative relocations, which can
be applied by the linker at build time. This means some additional
arithmetic is required when dereferencing these pointers, as we can no
longer dereference the pointer members directly.

So let's implement this for idreg-override.c in a preliminary way, i.e.,
convert all the references in code to use a special accessor that
produces the correct absolute value at runtime.

To preserve the strong type checking for the static initializers, use
union types for representing the hybrid quantities.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/arm64/kernel/idreg-override.c | 98 +++++++++++++-------
 1 file changed, 65 insertions(+), 33 deletions(-)

Comments

Marc Zyngier Nov. 27, 2023, 12:53 p.m. UTC | #1 
On Fri, 24 Nov 2023 10:18:55 +0000,
Ard Biesheuvel <ardb@google.com> wrote:
> 
> From: Ard Biesheuvel <ardb@kernel.org>
> 
> The ID reg override handling code uses a rather elaborate data structure
> that relies on statically initialized absolute address values in pointer
> fields. This means that this code cannot run until relocation fixups
> have been applied, and this is unfortunate, because it means we cannot
> discover overrides for KASLR or LVA/LPA without creating the kernel
> mapping and performing the relocations first.
> 
> This can be solved by switching to place-relative relocations, which can
> be applied by the linker at build time. This means some additional
> arithmetic is required when dereferencing these pointers, as we can no
> longer dereference the pointer members directly.
> 
> So let's implement this for idreg-override.c in a preliminary way, i.e.,
> convert all the references in code to use a special accessor that
> produces the correct absolute value at runtime.
> 
> To preserve the strong type checking for the static initializers, use
> union types for representing the hybrid quantities.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  arch/arm64/kernel/idreg-override.c | 98 +++++++++++++-------
>  1 file changed, 65 insertions(+), 33 deletions(-)
> 
> diff --git a/arch/arm64/kernel/idreg-override.c b/arch/arm64/kernel/idreg-override.c
> index 536bc33859bc..4e32a44560bf 100644
> --- a/arch/arm64/kernel/idreg-override.c
> +++ b/arch/arm64/kernel/idreg-override.c
> @@ -21,14 +21,32 @@
>  
>  static u64 __boot_status __initdata;
>  
> +// temporary __prel64 related definitions
> +// to be removed when this code is moved under pi/
> +
> +#define __prel64_initconst	__initconst
> +
> +typedef void *prel64_t;
> +
> +static void *prel64_to_pointer(const prel64_t *p)
> +{
> +	return *p;
> +}
> +

Having played with this series a bit to see how the E2H0 override
support would fit in, I found that this cast to a void* could hide
stupid bugs that should normally be caught at compile time.

Having hacked on it a bit, I came up with this (partial patch on top
of the full series):

diff --git a/arch/arm64/kernel/pi/idreg-override.c b/arch/arm64/kernel/pi/idreg-override.c
index 84647ebb87ee..d60322477e44 100644
--- a/arch/arm64/kernel/pi/idreg-override.c
+++ b/arch/arm64/kernel/pi/idreg-override.c
@@ -246,12 +246,12 @@ static void __init match_options(const char *cmdline)
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(regs); i++) {
-		const struct ftr_set_desc *reg = prel64_to_pointer(&regs[i].reg_prel);
+		const struct ftr_set_desc *reg = prel64_pointer(regs[i].reg);
 		struct arm64_ftr_override *override;
 		int len = strlen(reg->name);
 		int f;
 
-		override = prel64_to_pointer(&reg->override_prel);
+		override = prel64_pointer(reg->override);
 
 		// set opt[] to '<name>.'
 		memcpy(opt, reg->name, len);
diff --git a/arch/arm64/kernel/pi/pi.h b/arch/arm64/kernel/pi/pi.h
index 04a1f576baee..9d922322999b 100644
--- a/arch/arm64/kernel/pi/pi.h
+++ b/arch/arm64/kernel/pi/pi.h
@@ -15,6 +15,8 @@ static inline void *prel64_to_pointer(const prel64_t *offset)
 	return (void *)offset + *offset;
 }
 
+#define prel64_pointer(__d)	(typeof(__d))prel64_to_pointer(&__d##_prel)
+
 extern bool dynamic_scs_is_enabled;
 
 extern pgd_t init_idmap_pg_dir[];


which preserves the type-checking, at the expense of the implicit
*_prel field access. Not sure if that's something you have considered,
but I thought I'd raise it here.

Thanks,

	M.
Ard Biesheuvel Nov. 27, 2023, 12:58 p.m. UTC | #2 
On Mon, 27 Nov 2023 at 13:53, Marc Zyngier <maz@kernel.org> wrote:
>
> On Fri, 24 Nov 2023 10:18:55 +0000,
> Ard Biesheuvel <ardb@google.com> wrote:
> >
> > From: Ard Biesheuvel <ardb@kernel.org>
> >
> > The ID reg override handling code uses a rather elaborate data structure
> > that relies on statically initialized absolute address values in pointer
> > fields. This means that this code cannot run until relocation fixups
> > have been applied, and this is unfortunate, because it means we cannot
> > discover overrides for KASLR or LVA/LPA without creating the kernel
> > mapping and performing the relocations first.
> >
> > This can be solved by switching to place-relative relocations, which can
> > be applied by the linker at build time. This means some additional
> > arithmetic is required when dereferencing these pointers, as we can no
> > longer dereference the pointer members directly.
> >
> > So let's implement this for idreg-override.c in a preliminary way, i.e.,
> > convert all the references in code to use a special accessor that
> > produces the correct absolute value at runtime.
> >
> > To preserve the strong type checking for the static initializers, use
> > union types for representing the hybrid quantities.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  arch/arm64/kernel/idreg-override.c | 98 +++++++++++++-------
> >  1 file changed, 65 insertions(+), 33 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/idreg-override.c b/arch/arm64/kernel/idreg-override.c
> > index 536bc33859bc..4e32a44560bf 100644
> > --- a/arch/arm64/kernel/idreg-override.c
> > +++ b/arch/arm64/kernel/idreg-override.c
> > @@ -21,14 +21,32 @@
> >
> >  static u64 __boot_status __initdata;
> >
> > +// temporary __prel64 related definitions
> > +// to be removed when this code is moved under pi/
> > +
> > +#define __prel64_initconst   __initconst
> > +
> > +typedef void *prel64_t;
> > +
> > +static void *prel64_to_pointer(const prel64_t *p)
> > +{
> > +     return *p;
> > +}
> > +
>
> Having played with this series a bit to see how the E2H0 override
> support would fit in, I found that this cast to a void* could hide
> stupid bugs that should normally be caught at compile time.
>
> Having hacked on it a bit, I came up with this (partial patch on top
> of the full series):
>
> diff --git a/arch/arm64/kernel/pi/idreg-override.c b/arch/arm64/kernel/pi/idreg-override.c
> index 84647ebb87ee..d60322477e44 100644
> --- a/arch/arm64/kernel/pi/idreg-override.c
> +++ b/arch/arm64/kernel/pi/idreg-override.c
> @@ -246,12 +246,12 @@ static void __init match_options(const char *cmdline)
>         int i;
>
>         for (i = 0; i < ARRAY_SIZE(regs); i++) {
> -               const struct ftr_set_desc *reg = prel64_to_pointer(&regs[i].reg_prel);
> +               const struct ftr_set_desc *reg = prel64_pointer(regs[i].reg);
>                 struct arm64_ftr_override *override;
>                 int len = strlen(reg->name);
>                 int f;
>
> -               override = prel64_to_pointer(&reg->override_prel);
> +               override = prel64_pointer(reg->override);
>
>                 // set opt[] to '<name>.'
>                 memcpy(opt, reg->name, len);
> diff --git a/arch/arm64/kernel/pi/pi.h b/arch/arm64/kernel/pi/pi.h
> index 04a1f576baee..9d922322999b 100644
> --- a/arch/arm64/kernel/pi/pi.h
> +++ b/arch/arm64/kernel/pi/pi.h
> @@ -15,6 +15,8 @@ static inline void *prel64_to_pointer(const prel64_t *offset)
>         return (void *)offset + *offset;
>  }
>
> +#define prel64_pointer(__d)    (typeof(__d))prel64_to_pointer(&__d##_prel)
> +
>  extern bool dynamic_scs_is_enabled;
>
>  extern pgd_t init_idmap_pg_dir[];
>
>
> which preserves the type-checking, at the expense of the implicit
> *_prel field access. Not sure if that's something you have considered,
> but I thought I'd raise it here.
>

Thanks!

I agree the strict type checking is desirable, and I don't think the
implicit _prel concatenation is an issue given that this is only used
in a single source file (and if that changes, we can revisit)

I'll fold this in for the next rev.
diff mbox series

Patch

diff --git a/arch/arm64/kernel/idreg-override.c b/arch/arm64/kernel/idreg-override.c
index 536bc33859bc..4e32a44560bf 100644
--- a/arch/arm64/kernel/idreg-override.c
+++ b/arch/arm64/kernel/idreg-override.c
@@ -21,14 +21,32 @@ 
 
 static u64 __boot_status __initdata;
 
+// temporary __prel64 related definitions
+// to be removed when this code is moved under pi/
+
+#define __prel64_initconst	__initconst
+
+typedef void *prel64_t;
+
+static void *prel64_to_pointer(const prel64_t *p)
+{
+	return *p;
+}
+
 struct ftr_set_desc {
 	char 				name[FTR_DESC_NAME_LEN];
-	struct arm64_ftr_override	*override;
+	union {
+		struct arm64_ftr_override *override;
+		prel64_t		override_prel;
+	};
 	struct {
 		char			name[FTR_DESC_FIELD_LEN];
 		u8			shift;
 		u8			width;
-		bool			(*filter)(u64 val);
+		union {
+			bool		(*filter)(u64 val);
+			prel64_t	filter_prel;
+		};
 	} 				fields[];
 };
 
@@ -46,7 +64,7 @@  static bool __init mmfr1_vh_filter(u64 val)
 		 val == 0);
 }
 
-static const struct ftr_set_desc mmfr1 __initconst = {
+static const struct ftr_set_desc mmfr1 __prel64_initconst = {
 	.name		= "id_aa64mmfr1",
 	.override	= &id_aa64mmfr1_override,
 	.fields		= {
@@ -70,7 +88,7 @@  static bool __init pfr0_sve_filter(u64 val)
 	return true;
 }
 
-static const struct ftr_set_desc pfr0 __initconst = {
+static const struct ftr_set_desc pfr0 __prel64_initconst = {
 	.name		= "id_aa64pfr0",
 	.override	= &id_aa64pfr0_override,
 	.fields		= {
@@ -94,7 +112,7 @@  static bool __init pfr1_sme_filter(u64 val)
 	return true;
 }
 
-static const struct ftr_set_desc pfr1 __initconst = {
+static const struct ftr_set_desc pfr1 __prel64_initconst = {
 	.name		= "id_aa64pfr1",
 	.override	= &id_aa64pfr1_override,
 	.fields		= {
@@ -105,7 +123,7 @@  static const struct ftr_set_desc pfr1 __initconst = {
 	},
 };
 
-static const struct ftr_set_desc isar1 __initconst = {
+static const struct ftr_set_desc isar1 __prel64_initconst = {
 	.name		= "id_aa64isar1",
 	.override	= &id_aa64isar1_override,
 	.fields		= {
@@ -117,7 +135,7 @@  static const struct ftr_set_desc isar1 __initconst = {
 	},
 };
 
-static const struct ftr_set_desc isar2 __initconst = {
+static const struct ftr_set_desc isar2 __prel64_initconst = {
 	.name		= "id_aa64isar2",
 	.override	= &id_aa64isar2_override,
 	.fields		= {
@@ -128,7 +146,7 @@  static const struct ftr_set_desc isar2 __initconst = {
 	},
 };
 
-static const struct ftr_set_desc smfr0 __initconst = {
+static const struct ftr_set_desc smfr0 __prel64_initconst = {
 	.name		= "id_aa64smfr0",
 	.override	= &id_aa64smfr0_override,
 	.fields		= {
@@ -149,7 +167,7 @@  static bool __init hvhe_filter(u64 val)
 						     ID_AA64MMFR1_EL1_VH_SHIFT));
 }
 
-static const struct ftr_set_desc sw_features __initconst = {
+static const struct ftr_set_desc sw_features __prel64_initconst = {
 	.name		= "arm64_sw",
 	.override	= &arm64_sw_feature_override,
 	.fields		= {
@@ -159,14 +177,17 @@  static const struct ftr_set_desc sw_features __initconst = {
 	},
 };
 
-static const struct ftr_set_desc * const regs[] __initconst = {
-	&mmfr1,
-	&pfr0,
-	&pfr1,
-	&isar1,
-	&isar2,
-	&smfr0,
-	&sw_features,
+static const union {
+	const struct ftr_set_desc	*reg;
+	prel64_t			reg_prel;
+} regs[] __prel64_initconst = {
+	{ .reg = &mmfr1		},
+	{ .reg = &pfr0 		},
+	{ .reg = &pfr1 		},
+	{ .reg = &isar1		},
+	{ .reg = &isar2		},
+	{ .reg = &smfr0		},
+	{ .reg = &sw_features	},
 };
 
 static const struct {
@@ -214,15 +235,20 @@  static void __init match_options(const char *cmdline)
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(regs); i++) {
+		const struct ftr_set_desc *reg = prel64_to_pointer(&regs[i].reg_prel);
+		struct arm64_ftr_override *override;
 		int f;
 
-		for (f = 0; strlen(regs[i]->fields[f].name); f++) {
-			u64 shift = regs[i]->fields[f].shift;
-			u64 width = regs[i]->fields[f].width ?: 4;
+		override = prel64_to_pointer(&reg->override_prel);
+
+		for (f = 0; strlen(reg->fields[f].name); f++) {
+			u64 shift = reg->fields[f].shift;
+			u64 width = reg->fields[f].width ?: 4;
 			u64 mask = GENMASK_ULL(shift + width - 1, shift);
+			bool (*filter)(u64 val);
 			u64 v;
 
-			if (find_field(cmdline, regs[i], f, &v))
+			if (find_field(cmdline, reg, f, &v))
 				continue;
 
 			/*
@@ -230,16 +256,16 @@  static void __init match_options(const char *cmdline)
 			 * it by setting the value to the all-ones while
 			 * clearing the mask... Yes, this is fragile.
 			 */
-			if (regs[i]->fields[f].filter &&
-			    !regs[i]->fields[f].filter(v)) {
-				regs[i]->override->val  |= mask;
-				regs[i]->override->mask &= ~mask;
+			filter = prel64_to_pointer(&reg->fields[f].filter_prel);
+			if (filter && !filter(v)) {
+				override->val  |= mask;
+				override->mask &= ~mask;
 				continue;
 			}
 
-			regs[i]->override->val  &= ~mask;
-			regs[i]->override->val  |= (v << shift) & mask;
-			regs[i]->override->mask |= mask;
+			override->val  &= ~mask;
+			override->val  |= (v << shift) & mask;
+			override->mask |= mask;
 
 			return;
 		}
@@ -313,11 +339,16 @@  void init_feature_override(u64 boot_status);
 
 asmlinkage void __init init_feature_override(u64 boot_status)
 {
+	struct arm64_ftr_override *override;
+	const struct ftr_set_desc *reg;
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(regs); i++) {
-		regs[i]->override->val  = 0;
-		regs[i]->override->mask = 0;
+		reg = prel64_to_pointer(&regs[i].reg_prel);
+		override = prel64_to_pointer(&reg->override_prel);
+
+		override->val  = 0;
+		override->mask = 0;
 	}
 
 	__boot_status = boot_status;
@@ -325,8 +356,9 @@  asmlinkage void __init init_feature_override(u64 boot_status)
 	parse_cmdline();
 
 	for (i = 0; i < ARRAY_SIZE(regs); i++) {
-		dcache_clean_inval_poc((unsigned long)regs[i]->override,
-				       (unsigned long)regs[i]->override +
-				       sizeof(*regs[i]->override));
+		reg = prel64_to_pointer(&regs[i].reg_prel);
+		override = prel64_to_pointer(&reg->override_prel);
+		dcache_clean_inval_poc((unsigned long)override,
+				       (unsigned long)(override + 1));
 	}
 }