[v1] arm64: kernel: remove SHF_WRITE|SHF_EXECINSTR from .idmap.text

[PATCH] arm64: kernel: remove SHF_WRITE|SHF_EXECINSTR from .idmap.text

Posted by ndesaulniers@google.com 2 years, 9 months ago

commit d54170812ef1 ("arm64: fix .idmap.text assertion for large kernels")
modified some of the section assembler directives that declare
.idmap.text to be SHF_ALLOC instead of
SHF_ALLOC|SHF_WRITE|SHF_EXECINSTR.

This patch fixes up the remaining stragglers that were left behind.

Because .idmap.text is merged into .text, LLD will retain the
SHF_EXECINSTR on .text, in addition to the synthetic .got.  This doesn't
matter to the kernel loader, but syzkaller is having trouble symboling
such sections. Clean this up while we additionally fix up syzkaller. Add
Fixes tag so that this doesn't precede related change in stable.

Fixes: d54170812ef1 ("arm64: fix .idmap.text assertion for large kernels")
Reported-by: Greg Thelen <gthelen@google.com>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
 arch/arm64/kernel/cpu-reset.S | 2 +-
 arch/arm64/kernel/sleep.S     | 2 +-
 arch/arm64/mm/proc.S          | 6 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kernel/cpu-reset.S b/arch/arm64/kernel/cpu-reset.S
index 6b752fe89745..c87445dde674 100644
--- a/arch/arm64/kernel/cpu-reset.S
+++ b/arch/arm64/kernel/cpu-reset.S
@@ -14,7 +14,7 @@
 #include <asm/virt.h>
 
 .text
-.pushsection    .idmap.text, "awx"
+.pushsection    .idmap.text, "a"
 
 /*
  * cpu_soft_restart(el2_switch, entry, arg0, arg1, arg2)
diff --git a/arch/arm64/kernel/sleep.S b/arch/arm64/kernel/sleep.S
index 2ae7cff1953a..2aa5129d8253 100644
--- a/arch/arm64/kernel/sleep.S
+++ b/arch/arm64/kernel/sleep.S
@@ -97,7 +97,7 @@ SYM_FUNC_START(__cpu_suspend_enter)
 	ret
 SYM_FUNC_END(__cpu_suspend_enter)
 
-	.pushsection ".idmap.text", "awx"
+	.pushsection ".idmap.text", "a"
 SYM_CODE_START(cpu_resume)
 	mov	x0, xzr
 	bl	init_kernel_el
diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
index 91410f488090..c2cb437821ca 100644
--- a/arch/arm64/mm/proc.S
+++ b/arch/arm64/mm/proc.S
@@ -167,7 +167,7 @@ alternative_else_nop_endif
 SYM_FUNC_END(cpu_do_resume)
 #endif
 
-	.pushsection ".idmap.text", "awx"
+	.pushsection ".idmap.text", "a"
 
 .macro	__idmap_cpu_set_reserved_ttbr1, tmp1, tmp2
 	adrp	\tmp1, reserved_pg_dir
@@ -201,7 +201,7 @@ SYM_FUNC_END(idmap_cpu_replace_ttbr1)
 
 #define KPTI_NG_PTE_FLAGS	(PTE_ATTRINDX(MT_NORMAL) | SWAPPER_PTE_FLAGS)
 
-	.pushsection ".idmap.text", "awx"
+	.pushsection ".idmap.text", "a"
 
 	.macro	kpti_mk_tbl_ng, type, num_entries
 	add	end_\type\()p, cur_\type\()p, #\num_entries * 8
@@ -400,7 +400,7 @@ SYM_FUNC_END(idmap_kpti_install_ng_mappings)
  * Output:
  *	Return in x0 the value of the SCTLR_EL1 register.
  */
-	.pushsection ".idmap.text", "awx"
+	.pushsection ".idmap.text", "a"
 SYM_FUNC_START(__cpu_setup)
 	tlbi	vmalle1				// Invalidate local TLB
 	dsb	nsh

---
base-commit: 22b8cc3e78f5448b4c5df00303817a9137cd663f
change-id: 20230428-awx-c73f4bde79c4

Best regards,
-- 
Nick Desaulniers <ndesaulniers@google.com>

Re: [PATCH] arm64: kernel: remove SHF_WRITE|SHF_EXECINSTR from .idmap.text

Posted by Ard Biesheuvel 2 years, 9 months ago

Hi Nick,

On Fri, 28 Apr 2023 at 19:09, <ndesaulniers@google.com> wrote:
>
> commit d54170812ef1 ("arm64: fix .idmap.text assertion for large kernels")
> modified some of the section assembler directives that declare
> .idmap.text to be SHF_ALLOC instead of
> SHF_ALLOC|SHF_WRITE|SHF_EXECINSTR.
>
> This patch fixes up the remaining stragglers that were left behind.
>
> Because .idmap.text is merged into .text,

Nit: this is no longer the case: the ID map code is never executed via
the kernel mapping, so we moved it into a special .rodata.text section
that contains all generated code that should not have an executable
mapping by default, but only when it gets copied and/or mapped into a
different executable region.

This doesn't impact the correctness of the patch, so with this
paragraph clarified:

Reviewed-by: Ard Biesheuvel <ardb@kernel.org>


> LLD will retain the
> SHF_EXECINSTR on .text, in addition to the synthetic .got.  This doesn't
> matter to the kernel loader, but syzkaller is having trouble symboling
> such sections. Clean this up while we additionally fix up syzkaller. Add
> Fixes tag so that this doesn't precede related change in stable.
>
> Fixes: d54170812ef1 ("arm64: fix .idmap.text assertion for large kernels")
> Reported-by: Greg Thelen <gthelen@google.com>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
>  arch/arm64/kernel/cpu-reset.S | 2 +-
>  arch/arm64/kernel/sleep.S     | 2 +-
>  arch/arm64/mm/proc.S          | 6 +++---
>  3 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm64/kernel/cpu-reset.S b/arch/arm64/kernel/cpu-reset.S
> index 6b752fe89745..c87445dde674 100644
> --- a/arch/arm64/kernel/cpu-reset.S
> +++ b/arch/arm64/kernel/cpu-reset.S
> @@ -14,7 +14,7 @@
>  #include <asm/virt.h>
>
>  .text
> -.pushsection    .idmap.text, "awx"
> +.pushsection    .idmap.text, "a"
>
>  /*
>   * cpu_soft_restart(el2_switch, entry, arg0, arg1, arg2)
> diff --git a/arch/arm64/kernel/sleep.S b/arch/arm64/kernel/sleep.S
> index 2ae7cff1953a..2aa5129d8253 100644
> --- a/arch/arm64/kernel/sleep.S
> +++ b/arch/arm64/kernel/sleep.S
> @@ -97,7 +97,7 @@ SYM_FUNC_START(__cpu_suspend_enter)
>         ret
>  SYM_FUNC_END(__cpu_suspend_enter)
>
> -       .pushsection ".idmap.text", "awx"
> +       .pushsection ".idmap.text", "a"
>  SYM_CODE_START(cpu_resume)
>         mov     x0, xzr
>         bl      init_kernel_el
> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
> index 91410f488090..c2cb437821ca 100644
> --- a/arch/arm64/mm/proc.S
> +++ b/arch/arm64/mm/proc.S
> @@ -167,7 +167,7 @@ alternative_else_nop_endif
>  SYM_FUNC_END(cpu_do_resume)
>  #endif
>
> -       .pushsection ".idmap.text", "awx"
> +       .pushsection ".idmap.text", "a"
>
>  .macro __idmap_cpu_set_reserved_ttbr1, tmp1, tmp2
>         adrp    \tmp1, reserved_pg_dir
> @@ -201,7 +201,7 @@ SYM_FUNC_END(idmap_cpu_replace_ttbr1)
>
>  #define KPTI_NG_PTE_FLAGS      (PTE_ATTRINDX(MT_NORMAL) | SWAPPER_PTE_FLAGS)
>
> -       .pushsection ".idmap.text", "awx"
> +       .pushsection ".idmap.text", "a"
>
>         .macro  kpti_mk_tbl_ng, type, num_entries
>         add     end_\type\()p, cur_\type\()p, #\num_entries * 8
> @@ -400,7 +400,7 @@ SYM_FUNC_END(idmap_kpti_install_ng_mappings)
>   * Output:
>   *     Return in x0 the value of the SCTLR_EL1 register.
>   */
> -       .pushsection ".idmap.text", "awx"
> +       .pushsection ".idmap.text", "a"
>  SYM_FUNC_START(__cpu_setup)
>         tlbi    vmalle1                         // Invalidate local TLB
>         dsb     nsh
>
> ---
> base-commit: 22b8cc3e78f5448b4c5df00303817a9137cd663f
> change-id: 20230428-awx-c73f4bde79c4
>
> Best regards,
> --
> Nick Desaulniers <ndesaulniers@google.com>
>

Re: [PATCH] arm64: kernel: remove SHF_WRITE|SHF_EXECINSTR from .idmap.text

Posted by Nick Desaulniers 2 years, 9 months ago

On Fri, Apr 28, 2023 at 11:17 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> Hi Nick,
>
> On Fri, 28 Apr 2023 at 19:09, <ndesaulniers@google.com> wrote:
> >
> > commit d54170812ef1 ("arm64: fix .idmap.text assertion for large kernels")
> > modified some of the section assembler directives that declare
> > .idmap.text to be SHF_ALLOC instead of
> > SHF_ALLOC|SHF_WRITE|SHF_EXECINSTR.
> >
> > This patch fixes up the remaining stragglers that were left behind.
> >
> > Because .idmap.text is merged into .text,
>
> Nit: this is no longer the case: the ID map code is never executed via
> the kernel mapping, so we moved it into a special .rodata.text section
> that contains all generated code that should not have an executable
> mapping by default, but only when it gets copied and/or mapped into a
> different executable region.

Ah, sorry, I see IDMAP_TEXT is being placed in .rodata.text in
arch/arm64/kernel/vmlinux.lds.S, so it's just the synthetic .got then
that's affecting .text. So Fangrui's patch is meant to address that.
https://lore.kernel.org/linux-arm-kernel/20230428050442.180913-1-maskray@google.com/

In that case, I'll drop this paragraph starting with "Because"; I
misspelled symbolizing anyways.

>
> This doesn't impact the correctness of the patch, so with this
> paragraph clarified:
>
> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
>
>
> > LLD will retain the
> > SHF_EXECINSTR on .text, in addition to the synthetic .got.  This doesn't
> > matter to the kernel loader, but syzkaller is having trouble symboling
> > such sections. Clean this up while we additionally fix up syzkaller. Add
> > Fixes tag so that this doesn't precede related change in stable.
> >
> > Fixes: d54170812ef1 ("arm64: fix .idmap.text assertion for large kernels")
> > Reported-by: Greg Thelen <gthelen@google.com>
> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > ---
> >  arch/arm64/kernel/cpu-reset.S | 2 +-
> >  arch/arm64/kernel/sleep.S     | 2 +-
> >  arch/arm64/mm/proc.S          | 6 +++---
> >  3 files changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/cpu-reset.S b/arch/arm64/kernel/cpu-reset.S
> > index 6b752fe89745..c87445dde674 100644
> > --- a/arch/arm64/kernel/cpu-reset.S
> > +++ b/arch/arm64/kernel/cpu-reset.S
> > @@ -14,7 +14,7 @@
> >  #include <asm/virt.h>
> >
> >  .text
> > -.pushsection    .idmap.text, "awx"
> > +.pushsection    .idmap.text, "a"
> >
> >  /*
> >   * cpu_soft_restart(el2_switch, entry, arg0, arg1, arg2)
> > diff --git a/arch/arm64/kernel/sleep.S b/arch/arm64/kernel/sleep.S
> > index 2ae7cff1953a..2aa5129d8253 100644
> > --- a/arch/arm64/kernel/sleep.S
> > +++ b/arch/arm64/kernel/sleep.S
> > @@ -97,7 +97,7 @@ SYM_FUNC_START(__cpu_suspend_enter)
> >         ret
> >  SYM_FUNC_END(__cpu_suspend_enter)
> >
> > -       .pushsection ".idmap.text", "awx"
> > +       .pushsection ".idmap.text", "a"
> >  SYM_CODE_START(cpu_resume)
> >         mov     x0, xzr
> >         bl      init_kernel_el
> > diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
> > index 91410f488090..c2cb437821ca 100644
> > --- a/arch/arm64/mm/proc.S
> > +++ b/arch/arm64/mm/proc.S
> > @@ -167,7 +167,7 @@ alternative_else_nop_endif
> >  SYM_FUNC_END(cpu_do_resume)
> >  #endif
> >
> > -       .pushsection ".idmap.text", "awx"
> > +       .pushsection ".idmap.text", "a"
> >
> >  .macro __idmap_cpu_set_reserved_ttbr1, tmp1, tmp2
> >         adrp    \tmp1, reserved_pg_dir
> > @@ -201,7 +201,7 @@ SYM_FUNC_END(idmap_cpu_replace_ttbr1)
> >
> >  #define KPTI_NG_PTE_FLAGS      (PTE_ATTRINDX(MT_NORMAL) | SWAPPER_PTE_FLAGS)
> >
> > -       .pushsection ".idmap.text", "awx"
> > +       .pushsection ".idmap.text", "a"
> >
> >         .macro  kpti_mk_tbl_ng, type, num_entries
> >         add     end_\type\()p, cur_\type\()p, #\num_entries * 8
> > @@ -400,7 +400,7 @@ SYM_FUNC_END(idmap_kpti_install_ng_mappings)
> >   * Output:
> >   *     Return in x0 the value of the SCTLR_EL1 register.
> >   */
> > -       .pushsection ".idmap.text", "awx"
> > +       .pushsection ".idmap.text", "a"
> >  SYM_FUNC_START(__cpu_setup)
> >         tlbi    vmalle1                         // Invalidate local TLB
> >         dsb     nsh
> >
> > ---
> > base-commit: 22b8cc3e78f5448b4c5df00303817a9137cd663f
> > change-id: 20230428-awx-c73f4bde79c4
> >
> > Best regards,
> > --
> > Nick Desaulniers <ndesaulniers@google.com>
> >



-- 
Thanks,
~Nick Desaulniers