On Fri, 3 Feb 2023 at 19:08, Nathan Chancellor <nathan@xxxxxxxxxx> wrote:
>
> Hi Ard,
>
> On Wed, Jan 11, 2023 at 11:22:32AM +0100, Ard Biesheuvel wrote:
> > Reorganize the ID map slightly so that only code that is executed with
> > the MMU off or via the 1:1 mapping remains. This allows us to move the
> > identity map out of the .text segment, as it will no longer need
> > executable permissions via the kernel mapping.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
> > ---
> > arch/arm64/kernel/head.S | 28 +++++++++++---------
> > arch/arm64/kernel/vmlinux.lds.S | 2 +-
> > arch/arm64/mm/proc.S | 2 --
> > 3 files changed, 16 insertions(+), 16 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> > index c4e12d466a5f35f0..bec97aad092c2b43 100644
> > --- a/arch/arm64/kernel/head.S
> > +++ b/arch/arm64/kernel/head.S
> > @@ -543,19 +543,6 @@ SYM_INNER_LABEL(init_el2, SYM_L_LOCAL)
> > eret
> > SYM_FUNC_END(init_kernel_el)
> >
> > -/*
> > - * Sets the __boot_cpu_mode flag depending on the CPU boot mode passed
> > - * in w0. See arch/arm64/include/asm/virt.h for more info.
> > - */
> > -SYM_FUNC_START_LOCAL(set_cpu_boot_mode_flag)
> > - adr_l x1, __boot_cpu_mode
> > - cmp w0, #BOOT_CPU_MODE_EL2
> > - b.ne 1f
> > - add x1, x1, #4
> > -1: str w0, [x1] // Save CPU boot mode
> > - ret
> > -SYM_FUNC_END(set_cpu_boot_mode_flag)
> > -
> > /*
> > * This provides a "holding pen" for platforms to hold all secondary
> > * cores are held until we're ready for them to initialise.
> > @@ -599,6 +586,7 @@ SYM_FUNC_START_LOCAL(secondary_startup)
> > br x8
> > SYM_FUNC_END(secondary_startup)
> >
> > + .text
> > SYM_FUNC_START_LOCAL(__secondary_switched)
> > mov x0, x20
> > bl set_cpu_boot_mode_flag
> > @@ -631,6 +619,19 @@ SYM_FUNC_START_LOCAL(__secondary_too_slow)
> > b __secondary_too_slow
> > SYM_FUNC_END(__secondary_too_slow)
> >
> > +/*
> > + * Sets the __boot_cpu_mode flag depending on the CPU boot mode passed
> > + * in w0. See arch/arm64/include/asm/virt.h for more info.
> > + */
> > +SYM_FUNC_START_LOCAL(set_cpu_boot_mode_flag)
> > + adr_l x1, __boot_cpu_mode
> > + cmp w0, #BOOT_CPU_MODE_EL2
> > + b.ne 1f
> > + add x1, x1, #4
> > +1: str w0, [x1] // Save CPU boot mode
> > + ret
> > +SYM_FUNC_END(set_cpu_boot_mode_flag)
> > +
> > /*
> > * The booting CPU updates the failed status @__early_cpu_boot_status,
> > * with MMU turned off.
> > @@ -662,6 +663,7 @@ SYM_FUNC_END(__secondary_too_slow)
> > * Checks if the selected granule size is supported by the CPU.
> > * If it isn't, park the CPU
> > */
> > + .section ".idmap.text","awx"
> > SYM_FUNC_START(__enable_mmu)
> > mrs x3, ID_AA64MMFR0_EL1
> > ubfx x3, x3, #ID_AA64MMFR0_EL1_TGRAN_SHIFT, 4
> > diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
> > index 4c13dafc98b8400f..407415a5163ab62f 100644
> > --- a/arch/arm64/kernel/vmlinux.lds.S
> > +++ b/arch/arm64/kernel/vmlinux.lds.S
> > @@ -179,7 +179,6 @@ SECTIONS
> > LOCK_TEXT
> > KPROBES_TEXT
> > HYPERVISOR_TEXT
> > - IDMAP_TEXT
> > *(.gnu.warning)
> > . = ALIGN(16);
> > *(.got) /* Global offset table */
> > @@ -206,6 +205,7 @@ SECTIONS
> > TRAMP_TEXT
> > HIBERNATE_TEXT
> > KEXEC_TEXT
> > + IDMAP_TEXT
> > . = ALIGN(PAGE_SIZE);
> > }
> >
> > diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
> > index 066fa60b93d24827..91410f48809000a0 100644
> > --- a/arch/arm64/mm/proc.S
> > +++ b/arch/arm64/mm/proc.S
> > @@ -110,7 +110,6 @@ SYM_FUNC_END(cpu_do_suspend)
> > *
> > * x0: Address of context pointer
> > */
> > - .pushsection ".idmap.text", "awx"
> > SYM_FUNC_START(cpu_do_resume)
> > ldp x2, x3, [x0]
> > ldp x4, x5, [x0, #16]
> > @@ -166,7 +165,6 @@ alternative_else_nop_endif
> > isb
> > ret
> > SYM_FUNC_END(cpu_do_resume)
> > - .popsection
> > #endif
> >
> > .pushsection ".idmap.text", "awx"
> > --
> > 2.39.0
> >
>
> Sorry you have to keep hearing from me, I am starting to feel like a
> nuisance :) apologies if this is already been reported, I did a search
> of lore and did not find anything.
>
Don't be silly. If my patch broke something, it is my responsibility
to fix it, and I'd rather hear about it from you (with a high quality
report, as usual) than from someone who dumps a log on me but cannot
be bothered to follow up, or doesn't have the chops to help narrow it
down.
> I have noticed the following message on my arm64 machines recently and I
> had some time to bisect it down to this change in -next (log below):
>
> [ 0.029481] kprobes: Failed to populate blacklist (error -22), kprobes not restricted, be careful using them!
>
> I can trivially reproduce it with defconfig + CONFIG_KPROBES=y in QEMU.
> If there is any other information I can provide or patches I can test, I
> am more than happy to do so.
>
I had noticed this diagnostic a couple of times as well, but tbh, I
did not realize that it was my own patch that caused it.
I think the below should fix it: we no longer have to blacklist the ID
map for kprobes now that it is no longer part of the .text section to
begin with, and kprobes will disregard it by default
diff --git a/arch/arm64/kernel/probes/kprobes.c
b/arch/arm64/kernel/probes/kprobes.c
index f35d059a9a366fa6..70b91a8c6bb3f358 100644
--- a/arch/arm64/kernel/probes/kprobes.c
+++ b/arch/arm64/kernel/probes/kprobes.c
@@ -387,10 +387,6 @@ int __init arch_populate_kprobe_blacklist(void)
(unsigned long)__irqentry_text_end);
if (ret)
return ret;
- ret = kprobe_add_area_blacklist((unsigned long)__idmap_text_start,
- (unsigned long)__idmap_text_end);
- if (ret)
- return ret;
ret = kprobe_add_area_blacklist((unsigned long)__hyp_text_start,
(unsigned long)__hyp_text_end);
if (ret || is_kernel_in_hyp_mode())
Feel free to turn this into a patch and send it out. (The day has
already ended here :-))
