Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230428-awx-v1-1-1f490286ba62@google.com> <CAMj1kXHskJLA7yfy1j4rLYPjscK6bsW0CGjmhpNYg5fuirwinA@mail.gmail.com>
In-Reply-To: <CAMj1kXHskJLA7yfy1j4rLYPjscK6bsW0CGjmhpNYg5fuirwinA@mail.gmail.com>
From:   Nick Desaulniers <ndesaulniers@google.com>
Date:   Fri, 28 Apr 2023 11:22:48 -0700
Message-ID: <CAKwvOdkpbZeG_W9kNHn4hadrkhrMCWjdEAq8rMUYi9rog3iz4A@mail.gmail.com>
Subject: Re: [PATCH] arm64: kernel: remove SHF_WRITE|SHF_EXECINSTR from .idmap.text
To:     Ard Biesheuvel <ardb@kernel.org>
Cc:     Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        Mark Rutland <mark.rutland@arm.com>,
        Fangrui Song <maskray@google.com>,
        Greg Thelen <gthelen@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Apr 28, 2023 at 11:17=E2=80=AFAM Ard Biesheuvel <ardb@kernel.org> w=
rote:
>
> Hi Nick,
>
> On Fri, 28 Apr 2023 at 19:09, <ndesaulniers@google.com> wrote:
> >
> > commit d54170812ef1 ("arm64: fix .idmap.text assertion for large kernel=
s")
> > 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@go=
ogle.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. Ad=
d
> > Fixes tag so that this doesn't precede related change in stable.
> >
> > Fixes: d54170812ef1 ("arm64: fix .idmap.text assertion for large kernel=
s")
> > 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-rese=
t.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>
> >


--=20
Thanks,
~Nick Desaulniers