| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZdUyOAQG0TK1UtTY@vm3>
Date: Wed, 21 Feb 2024 08:14:00 +0900
From: Itaru Kitayama <itaru.kitayama@...ux.dev>
To: Mark Rutland <mark.rutland@....com>
Cc: skseofh@...il.com, catalin.marinas@....com, will@...nel.org,
ryan.roberts@....com, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] arm64: add early fixmap initialization flag
On Tue, Feb 20, 2024 at 11:55:30AM +0000, Mark Rutland wrote:
> On Tue, Feb 20, 2024 at 09:29:14AM +0900, Itaru Kitayama wrote:
> > On Mon, Feb 19, 2024 at 10:48:26AM +0000, Mark Rutland wrote:
> > > On Sat, Feb 17, 2024 at 11:03:26PM +0900, skseofh@...il.com wrote:
> > > > From: Daero Lee <skseofh@...il.com>
> > > >
> > > > early_fixmap_init may be called multiple times. Since there is no
> > > > change in the page table after early fixmap initialization, an
> > > > initialization flag was added.
> > >
> > > Why is that better?
> > >
> > > We call early_fixmap_init() in two places:
> > >
> > > * early_fdt_map()
> > > * setup_arch()
> > >
> > > ... and to get to setup_arch() we *must* have gone through early_fdt_map(),
> > > since __primary_switched() calls that before going to setup_arch().
> > >
> > > So AFAICT we can remove the second call to early_fixmap_init() in setup_arch(),
> > > and rely on the earlier one in early_fdt_map().
> >
> > Removing the second call makes the code base a bit harder to understand
> > as the functions related to DT and ACPI setup are not separated cleanly.
> > I prefer calling the early_fixmap_init() in setup_arch() as well.
>
> I appreciate what you're saying here, but I don't think that it's better to
> keep the second call in setup_arch().
>
> We rely on having a (stub) DT in order to find UEFI and ACPI tables, so the DT
> and ACPI setup can never be truly separated. We always need to map that DT in
> order to find the UEFI+ACPI tables, and in order to do that we must initialize
> the fixmap first.
Okay.
>
> I don't think there's any good reason to keep a redundant call in setup_arch();
> that's just misleading and potentially problematic if we ever change
> early_fixmap_init() so that it's not idempotent.
>
> I agree it's somewhat a layering violation for early_fdt_map() to be
> responsible for initialising the fixmap, so how about we just pull that out,
> e.g. as below?
>
> Mark.
>
> ---->8----
> From 5f07f9c1d352f760fa7aba97f1b4f42d9cb99496 Mon Sep 17 00:00:00 2001
> From: Mark Rutland <mark.rutland@....com>
> Date: Tue, 20 Feb 2024 11:09:17 +0000
> Subject: [PATCH] arm64: clean up fixmap initalization
>
> Currently we have redundant initialization of the fixmap, first in
> early_fdt_map(), and then again in setup_arch() before we call
> early_ioremap_init(). This redundant initialization isn't harmful, as
> early_fixmap_init() happens to be idempotent, but it's redundant and
> potentially confusing.
>
> We need to call early_fixmap_init() before we map the FDT and before we
> call early_ioremap_init(). Ideally we'd place early_fixmap_init() and
> early_ioremap_init() in the same caller as early_ioremap_init() is
> somewhat coupled with the fixmap code.
>
> Clean this up by moving the calls to early_fixmap_init() and
> early_ioremap_init() into a new early_mappings_init() function, and
> calling this once in __primary_switched() before we call
> early_fdt_map(). This means we initialize the fixmap once, and keep
> early_fixmap_init() and early_ioremap_init() together.
>
> This is a cleanup, not a fix, and does not need to be backported to
> stable kernels.
>
> Reported-by: Daero Lee <skseofh@...il.com>
> Signed-off-by: Mark Rutland <mark.rutland@....com>
> Cc: Catalin Marinas <catalin.marinas@....com>
> Cc: Itaru Kitayama <itaru.kitayama@...ux.dev>
> Cc: Will Deacon <will@...nel.org>
> ---
> arch/arm64/include/asm/setup.h | 1 +
> arch/arm64/kernel/head.S | 2 ++
> arch/arm64/kernel/setup.c | 11 ++++++-----
> 3 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm64/include/asm/setup.h b/arch/arm64/include/asm/setup.h
> index 2e4d7da74fb87..c8ba018bcc09f 100644
> --- a/arch/arm64/include/asm/setup.h
> +++ b/arch/arm64/include/asm/setup.h
> @@ -9,6 +9,7 @@
>
> void *get_early_fdt_ptr(void);
> void early_fdt_map(u64 dt_phys);
> +void early_mappings_init(void);
>
> /*
> * These two variables are used in the head.S file.
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index cab7f91949d8f..c60c5454c5704 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -510,6 +510,8 @@ SYM_FUNC_START_LOCAL(__primary_switched)
> #if defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS)
> bl kasan_early_init
> #endif
> + bl early_mappings_init
> +
> mov x0, x21 // pass FDT address in x0
> bl early_fdt_map // Try mapping the FDT early
> mov x0, x20 // pass the full boot status
> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index 42c690bb2d608..7a539534ced78 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -176,8 +176,6 @@ void __init *get_early_fdt_ptr(void)
> asmlinkage void __init early_fdt_map(u64 dt_phys)
> {
> int fdt_size;
> -
> - early_fixmap_init();
> early_fdt_ptr = fixmap_remap_fdt(dt_phys, &fdt_size, PAGE_KERNEL);
> }
>
> @@ -290,6 +288,12 @@ u64 cpu_logical_map(unsigned int cpu)
> return __cpu_logical_map[cpu];
> }
>
> +asmlinkage void __init early_mappings_init(void)
> +{
> + early_fixmap_init();
> + early_ioremap_init();
> +}
> +
> void __init __no_sanitize_address setup_arch(char **cmdline_p)
> {
> setup_initial_init_mm(_stext, _etext, _edata, _end);
> @@ -305,9 +309,6 @@ void __init __no_sanitize_address setup_arch(char **cmdline_p)
> */
> arm64_use_ng_mappings = kaslr_requires_kpti();
>
> - early_fixmap_init();
> - early_ioremap_init();
> -
> setup_machine_fdt(__fdt_pointer);
>
> /*
Thanks for this. Makes sense to me; would you post a proper patch
so I can build and do a boot test, just to make sure?
Reviewed-by: Itaru Kitayama <itaru.kitayama@...itsu.com>
Thanks,
Itaru.
> --
> 2.30.2
>
Powered by blists - more mailing lists