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

[Ard Biesheuvel]

On Mon, 27 Nov 2023 at 13:53, Marc Zyngier <maz at kernel.org> wrote:
>
> On Fri, 24 Nov 2023 10:18:55 +0000,
> Ard Biesheuvel <ardb at google.com> wrote:
> >
> > From: Ard Biesheuvel <ardb at 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 at 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.