Re: [PATCH v5 18/21] arm64: Move "nokaslr" over to the early cpufeature infrastructure

[Ard Biesheuvel <ardb@kernel.org>]

On Mon, 25 Jan 2021 at 11:53, Marc Zyngier <maz@kernel.org> wrote:
>
> Given that the early cpufeature infrastructure has borrowed quite
> a lot of code from the kaslr implementation, let's reimplement
> the matching of the "nokaslr" option with it.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> Acked-by: David Brazdil <dbrazdil@google.com>
> ---
>  arch/arm64/kernel/idreg-override.c | 15 +++++++++++++
>  arch/arm64/kernel/kaslr.c          | 36 ++----------------------------
>  2 files changed, 17 insertions(+), 34 deletions(-)
>
> diff --git a/arch/arm64/kernel/idreg-override.c b/arch/arm64/kernel/idreg-override.c
> index cbb8eaa48742..3ccf51b84ba4 100644
> --- a/arch/arm64/kernel/idreg-override.c
> +++ b/arch/arm64/kernel/idreg-override.c
> @@ -31,8 +31,22 @@ static const struct ftr_set_desc mmfr1 __initdata = {
>         },
>  };
>
> +extern struct arm64_ftr_override kaslr_feature_override;
> +
> +static const struct ftr_set_desc kaslr __initdata = {

This should be __initconst not __initdata (below too)

> +       .name           = "kaslr",
> +#ifdef CONFIG_RANDOMIZE_BASE
> +       .override       = &kaslr_feature_override,
> +#endif
> +       .fields         = {
> +               { "disabled", 0 },
> +               {}
> +       },
> +};
> +
>  static const struct ftr_set_desc * const regs[] __initdata = {
>         &mmfr1,
> +       &kaslr,
>  };
>
>  static const struct {
> @@ -41,6 +55,7 @@ static const struct {
>  } aliases[] __initdata = {
>         { "kvm-arm.mode=nvhe",          "id_aa64mmfr1.vh=0" },
>         { "kvm-arm.mode=protected",     "id_aa64mmfr1.vh=0" },
> +       { "nokaslr",                    "kaslr.disabled=1" },
>  };
>

This struct now takes up
- ~100 bytes for the characters themselves (which btw are not emitted
into __initdata or __initconst)
- 6x8 bytes for the char pointers
- 6x24 bytes for the RELA relocations that annotate these pointers as
quantities that need to be relocated at boot (on a kernel built with
KASLR)

I know it's only a drop in the ocean, but in this case, where the
struct is statically declared and defined only once, and in the same
place, we could easily turn this into

static const struct {
   char alias[24];
   char param[20];
};

and get rid of all the overhead. The only slightly annoying thing is
that the array sizes need to be kept in sync with the largest instance
appearing in the array, but this is easy when the struct type is
declared in the same place where its only instance is defined.


>  static char *cmdline_contains_option(const char *cmdline, const char *option)
> diff --git a/arch/arm64/kernel/kaslr.c b/arch/arm64/kernel/kaslr.c
> index 5fc86e7d01a1..27f8939deb1b 100644
> --- a/arch/arm64/kernel/kaslr.c
> +++ b/arch/arm64/kernel/kaslr.c
> @@ -51,39 +51,7 @@ static __init u64 get_kaslr_seed(void *fdt)
>         return ret;
>  }
>
> -static __init bool cmdline_contains_nokaslr(const u8 *cmdline)
> -{
> -       const u8 *str;
> -
> -       str = strstr(cmdline, "nokaslr");
> -       return str == cmdline || (str > cmdline && *(str - 1) == ' ');
> -}
> -
> -static __init bool is_kaslr_disabled_cmdline(void *fdt)
> -{
> -       if (!IS_ENABLED(CONFIG_CMDLINE_FORCE)) {
> -               int node;
> -               const u8 *prop;
> -
> -               node = fdt_path_offset(fdt, "/chosen");
> -               if (node < 0)
> -                       goto out;
> -
> -               prop = fdt_getprop(fdt, node, "bootargs", NULL);
> -               if (!prop)
> -                       goto out;
> -
> -               if (cmdline_contains_nokaslr(prop))
> -                       return true;
> -
> -               if (IS_ENABLED(CONFIG_CMDLINE_EXTEND))
> -                       goto out;
> -
> -               return false;
> -       }
> -out:
> -       return cmdline_contains_nokaslr(CONFIG_CMDLINE);
> -}
> +struct arm64_ftr_override kaslr_feature_override __initdata;
>
>  /*
>   * This routine will be executed with the kernel mapped at its default virtual
> @@ -126,7 +94,7 @@ u64 __init kaslr_early_init(void)
>          * Check if 'nokaslr' appears on the command line, and
>          * return 0 if that is the case.
>          */
> -       if (is_kaslr_disabled_cmdline(fdt)) {
> +       if (kaslr_feature_override.val & kaslr_feature_override.mask & 0xf) {
>                 kaslr_status = KASLR_DISABLED_CMDLINE;
>                 return 0;
>         }
> --
> 2.29.2
>