| Message ID | 20220624150651.1358849-16-ardb@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: refactor boot flow | expand |
Hi Ard, On Fri, Jun 24, 2022 at 05:06:45PM +0200, Ard Biesheuvel wrote: > Instead of calling into the kernel to map the FDT into the kernel page > tables before even calling start_kernel(), let's switch to the initial, > temporary mapping of the device tree that has been added to the ID map. > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> Unfortunately, this patch breaks KASAN_INLINE=y, as the accesses to the idmap alias of the FDT get a poison check, which faults, resulting in a panic() before any of the command line is parsed. It's a bit painful to fix this; I had hoped it would be simple to fix by adding: KASAN_SANITIZE_idreg-override.o := n ... to arch/arm64/kernel/Makefile, but there are a bunch of calls to out-of-line functions that end up being instrumented (e.g. strncpy() and some fdt_*() functions), and it doesn't look like it'll be simple to go fix those. Can we revert this? IIUC this patch is an optimization rather being necessary for later changes. Thanks, Mark. > --- > arch/arm64/kernel/head.S | 1 + > arch/arm64/kernel/idreg-override.c | 17 ++++++----------- > 2 files changed, 7 insertions(+), 11 deletions(-) > > diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S > index 8283ff848328..64ebff634b83 100644 > --- a/arch/arm64/kernel/head.S > +++ b/arch/arm64/kernel/head.S > @@ -472,6 +472,7 @@ SYM_FUNC_START_LOCAL(__primary_switched) > #endif > mov x0, x21 // pass FDT address in x0 > bl early_fdt_map // Try mapping the FDT early > + mov x0, x22 // pass FDT address in x0 > bl init_feature_override // Parse cpu feature overrides > #ifdef CONFIG_RANDOMIZE_BASE > tst x23, ~(MIN_KIMG_ALIGN - 1) // already running randomized? > diff --git a/arch/arm64/kernel/idreg-override.c b/arch/arm64/kernel/idreg-override.c > index 8a2ceb591686..f92836e196e5 100644 > --- a/arch/arm64/kernel/idreg-override.c > +++ b/arch/arm64/kernel/idreg-override.c > @@ -201,16 +201,11 @@ static __init void __parse_cmdline(const char *cmdline, bool parse_aliases) > } while (1); > } > > -static __init const u8 *get_bootargs_cmdline(void) > +static __init const u8 *get_bootargs_cmdline(const void *fdt) > { > const u8 *prop; > - void *fdt; > int node; > > - fdt = get_early_fdt_ptr(); > - if (!fdt) > - return NULL; > - > node = fdt_path_offset(fdt, "/chosen"); > if (node < 0) > return NULL; > @@ -222,9 +217,9 @@ static __init const u8 *get_bootargs_cmdline(void) > return strlen(prop) ? prop : NULL; > } > > -static __init void parse_cmdline(void) > +static __init void parse_cmdline(const void *fdt) > { > - const u8 *prop = get_bootargs_cmdline(); > + const u8 *prop = get_bootargs_cmdline(fdt); > > if (IS_ENABLED(CONFIG_CMDLINE_FORCE) || !prop) > __parse_cmdline(CONFIG_CMDLINE, true); > @@ -234,9 +229,9 @@ static __init void parse_cmdline(void) > } > > /* Keep checkers quiet */ > -void init_feature_override(void); > +void init_feature_override(const void *fdt); > > -asmlinkage void __init init_feature_override(void) > +asmlinkage void __init init_feature_override(const void *fdt) > { > int i; > > @@ -247,7 +242,7 @@ asmlinkage void __init init_feature_override(void) > } > } > > - parse_cmdline(); > + parse_cmdline(fdt); > > for (i = 0; i < ARRAY_SIZE(regs); i++) { > if (regs[i]->override) > -- > 2.35.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On Mon, 11 Jul 2022 at 17:39, Mark Rutland <mark.rutland@arm.com> wrote: > > Hi Ard, > > On Fri, Jun 24, 2022 at 05:06:45PM +0200, Ard Biesheuvel wrote: > > Instead of calling into the kernel to map the FDT into the kernel page > > tables before even calling start_kernel(), let's switch to the initial, > > temporary mapping of the device tree that has been added to the ID map. > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > Unfortunately, this patch breaks KASAN_INLINE=y, as the accesses to the idmap > alias of the FDT get a poison check, which faults, resulting in a panic() > before any of the command line is parsed. > > It's a bit painful to fix this; I had hoped it would be simple to fix by > adding: > > KASAN_SANITIZE_idreg-override.o := n > > ... to arch/arm64/kernel/Makefile, but there are a bunch of calls to > out-of-line functions that end up being instrumented (e.g. strncpy() and some > fdt_*() functions), and it doesn't look like it'll be simple to go fix those. > Ugh. I suppose the KASAN projection produces invalid shadow addresses for the TTBR0 range, so there is no way we can map some zero shadow for the FDT? > Can we revert this? IIUC this patch is an optimization rather being necessary > for later changes. > Indeed, let's revert this for now, along with the follow-up patch that removes the early FDT fixmap remap hack. > > > --- > > arch/arm64/kernel/head.S | 1 + > > arch/arm64/kernel/idreg-override.c | 17 ++++++----------- > > 2 files changed, 7 insertions(+), 11 deletions(-) > > > > diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S > > index 8283ff848328..64ebff634b83 100644 > > --- a/arch/arm64/kernel/head.S > > +++ b/arch/arm64/kernel/head.S > > @@ -472,6 +472,7 @@ SYM_FUNC_START_LOCAL(__primary_switched) > > #endif > > mov x0, x21 // pass FDT address in x0 > > bl early_fdt_map // Try mapping the FDT early > > + mov x0, x22 // pass FDT address in x0 > > bl init_feature_override // Parse cpu feature overrides > > #ifdef CONFIG_RANDOMIZE_BASE > > tst x23, ~(MIN_KIMG_ALIGN - 1) // already running randomized? > > diff --git a/arch/arm64/kernel/idreg-override.c b/arch/arm64/kernel/idreg-override.c > > index 8a2ceb591686..f92836e196e5 100644 > > --- a/arch/arm64/kernel/idreg-override.c > > +++ b/arch/arm64/kernel/idreg-override.c > > @@ -201,16 +201,11 @@ static __init void __parse_cmdline(const char *cmdline, bool parse_aliases) > > } while (1); > > } > > > > -static __init const u8 *get_bootargs_cmdline(void) > > +static __init const u8 *get_bootargs_cmdline(const void *fdt) > > { > > const u8 *prop; > > - void *fdt; > > int node; > > > > - fdt = get_early_fdt_ptr(); > > - if (!fdt) > > - return NULL; > > - > > node = fdt_path_offset(fdt, "/chosen"); > > if (node < 0) > > return NULL; > > @@ -222,9 +217,9 @@ static __init const u8 *get_bootargs_cmdline(void) > > return strlen(prop) ? prop : NULL; > > } > > > > -static __init void parse_cmdline(void) > > +static __init void parse_cmdline(const void *fdt) > > { > > - const u8 *prop = get_bootargs_cmdline(); > > + const u8 *prop = get_bootargs_cmdline(fdt); > > > > if (IS_ENABLED(CONFIG_CMDLINE_FORCE) || !prop) > > __parse_cmdline(CONFIG_CMDLINE, true); > > @@ -234,9 +229,9 @@ static __init void parse_cmdline(void) > > } > > > > /* Keep checkers quiet */ > > -void init_feature_override(void); > > +void init_feature_override(const void *fdt); > > > > -asmlinkage void __init init_feature_override(void) > > +asmlinkage void __init init_feature_override(const void *fdt) > > { > > int i; > > > > @@ -247,7 +242,7 @@ asmlinkage void __init init_feature_override(void) > > } > > } > > > > - parse_cmdline(); > > + parse_cmdline(fdt); > > > > for (i = 0; i < ARRAY_SIZE(regs); i++) { > > if (regs[i]->override) > > -- > > 2.35.1 > > > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > >
On Tue, Jul 12, 2022 at 07:03:56PM +0200, Ard Biesheuvel wrote: > On Mon, 11 Jul 2022 at 17:39, Mark Rutland <mark.rutland@arm.com> wrote: > > > > Hi Ard, > > > > On Fri, Jun 24, 2022 at 05:06:45PM +0200, Ard Biesheuvel wrote: > > > Instead of calling into the kernel to map the FDT into the kernel page > > > tables before even calling start_kernel(), let's switch to the initial, > > > temporary mapping of the device tree that has been added to the ID map. > > > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > > > Unfortunately, this patch breaks KASAN_INLINE=y, as the accesses to the idmap > > alias of the FDT get a poison check, which faults, resulting in a panic() > > before any of the command line is parsed. > > > > It's a bit painful to fix this; I had hoped it would be simple to fix by > > adding: > > > > KASAN_SANITIZE_idreg-override.o := n > > > > ... to arch/arm64/kernel/Makefile, but there are a bunch of calls to > > out-of-line functions that end up being instrumented (e.g. strncpy() and some > > fdt_*() functions), and it doesn't look like it'll be simple to go fix those. > > Ugh. I suppose the KASAN projection produces invalid shadow addresses > for the TTBR0 range, so there is no way we can map some zero shadow > for the FDT? Yup, exactly. > > Can we revert this? IIUC this patch is an optimization rather being necessary > > for later changes. > > Indeed, let's revert this for now, along with the follow-up patch that > removes the early FDT fixmap remap hack. I'll send a patch shortly. Thanks, Mark.
diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S index 8283ff848328..64ebff634b83 100644 --- a/arch/arm64/kernel/head.S +++ b/arch/arm64/kernel/head.S @@ -472,6 +472,7 @@ SYM_FUNC_START_LOCAL(__primary_switched) #endif mov x0, x21 // pass FDT address in x0 bl early_fdt_map // Try mapping the FDT early + mov x0, x22 // pass FDT address in x0 bl init_feature_override // Parse cpu feature overrides #ifdef CONFIG_RANDOMIZE_BASE tst x23, ~(MIN_KIMG_ALIGN - 1) // already running randomized? diff --git a/arch/arm64/kernel/idreg-override.c b/arch/arm64/kernel/idreg-override.c index 8a2ceb591686..f92836e196e5 100644 --- a/arch/arm64/kernel/idreg-override.c +++ b/arch/arm64/kernel/idreg-override.c @@ -201,16 +201,11 @@ static __init void __parse_cmdline(const char *cmdline, bool parse_aliases) } while (1); } -static __init const u8 *get_bootargs_cmdline(void) +static __init const u8 *get_bootargs_cmdline(const void *fdt) { const u8 *prop; - void *fdt; int node; - fdt = get_early_fdt_ptr(); - if (!fdt) - return NULL; - node = fdt_path_offset(fdt, "/chosen"); if (node < 0) return NULL; @@ -222,9 +217,9 @@ static __init const u8 *get_bootargs_cmdline(void) return strlen(prop) ? prop : NULL; } -static __init void parse_cmdline(void) +static __init void parse_cmdline(const void *fdt) { - const u8 *prop = get_bootargs_cmdline(); + const u8 *prop = get_bootargs_cmdline(fdt); if (IS_ENABLED(CONFIG_CMDLINE_FORCE) || !prop) __parse_cmdline(CONFIG_CMDLINE, true); @@ -234,9 +229,9 @@ static __init void parse_cmdline(void) } /* Keep checkers quiet */ -void init_feature_override(void); +void init_feature_override(const void *fdt); -asmlinkage void __init init_feature_override(void) +asmlinkage void __init init_feature_override(const void *fdt) { int i; @@ -247,7 +242,7 @@ asmlinkage void __init init_feature_override(void) } } - parse_cmdline(); + parse_cmdline(fdt); for (i = 0; i < ARRAY_SIZE(regs); i++) { if (regs[i]->override)
Instead of calling into the kernel to map the FDT into the kernel page tables before even calling start_kernel(), let's switch to the initial, temporary mapping of the device tree that has been added to the ID map. Signed-off-by: Ard Biesheuvel <ardb@kernel.org> --- arch/arm64/kernel/head.S | 1 + arch/arm64/kernel/idreg-override.c | 17 ++++++----------- 2 files changed, 7 insertions(+), 11 deletions(-)