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: <cover.1682673542.git.houwenlong.hwl@antgroup.com> <980069339b23a6cc4ae6d605d188338467a5b08b.1682673543.git.houwenlong.hwl@antgroup.com>
In-Reply-To: <980069339b23a6cc4ae6d605d188338467a5b08b.1682673543.git.houwenlong.hwl@antgroup.com>
From:   Ard Biesheuvel <ardb@kernel.org>
Date:   Sun, 30 Apr 2023 16:23:56 +0200
Message-ID: <CAMj1kXFFo6Y=p63N41DrN4wLzMNVdtD-hp6gBVQwNqrzt7oqwQ@mail.gmail.com>
Subject: Re: [PATCH RFC 25/43] x86/mm: Make the x86 GOT read-only
To:     Hou Wenlong <houwenlong.hwl@antgroup.com>
Cc:     linux-kernel@vger.kernel.org,
        Thomas Garnier <thgarnie@chromium.org>,
        Lai Jiangshan <jiangshan.ljs@antgroup.com>,
        Kees Cook <keescook@chromium.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
        "H. Peter Anvin" <hpa@zytor.com>, Arnd Bergmann <arnd@arndb.de>,
        Peter Zijlstra <peterz@infradead.org>,
        Josh Poimboeuf <jpoimboe@kernel.org>,
        Juergen Gross <jgross@suse.com>,
        Brian Gerst <brgerst@gmail.com>, linux-arch@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, 28 Apr 2023 at 11:55, Hou Wenlong <houwenlong.hwl@antgroup.com> wrote:
>
> From: Thomas Garnier <thgarnie@chromium.org>
>
> From: Thomas Garnier <thgarnie@chromium.org>
>
> The GOT is changed during early boot when relocations are applied. Make
> it read-only directly. This table exists only for PIE binary. Since weak
> symbol reference would always be GOT reference, there are 8 entries in
> GOT, but only one entry for __fentry__() is in use.  Other GOT
> references have been optimized by linker.
>
> [Hou Wenlong: Change commit message and skip GOT size check]
>
> Signed-off-by: Thomas Garnier <thgarnie@chromium.org>
> Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
> Cc: Lai Jiangshan <jiangshan.ljs@antgroup.com>
> Cc: Kees Cook <keescook@chromium.org>
> ---
>  arch/x86/kernel/vmlinux.lds.S     |  2 ++
>  include/asm-generic/vmlinux.lds.h | 12 ++++++++++++
>  2 files changed, 14 insertions(+)
>
> diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S
> index f02dcde9f8a8..fa4c6582663f 100644
> --- a/arch/x86/kernel/vmlinux.lds.S
> +++ b/arch/x86/kernel/vmlinux.lds.S
> @@ -462,6 +462,7 @@ SECTIONS
>  #endif
>                "Unexpected GOT/PLT entries detected!")
>
> +#ifndef CONFIG_X86_PIE
>         /*
>          * Sections that should stay zero sized, which is safer to
>          * explicitly check instead of blindly discarding.
> @@ -470,6 +471,7 @@ SECTIONS
>                 *(.got) *(.igot.*)
>         }
>         ASSERT(SIZEOF(.got) == 0, "Unexpected GOT entries detected!")
> +#endif
>
>         .plt : {
>                 *(.plt) *(.plt.*) *(.iplt)
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index d1f57e4868ed..438ed8b39896 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -441,6 +441,17 @@
>         __end_ro_after_init = .;
>  #endif
>
> +#ifdef CONFIG_X86_PIE
> +#define RO_GOT_X86

Please don't put X86 specific stuff in generic code.

> +       .got        : AT(ADDR(.got) - LOAD_OFFSET) {                    \
> +               __start_got = .;                                        \
> +               *(.got) *(.igot.*);                                     \
> +               __end_got = .;                                          \
> +       }
> +#else
> +#define RO_GOT_X86
> +#endif
> +

I don't think it makes sense for this definition to be conditional.
You can include it conditionally from the x86 code, but even that
seems unnecessary, given that it will be empty otherwise.

>  /*
>   * .kcfi_traps contains a list KCFI trap locations.
>   */
> @@ -486,6 +497,7 @@
>                 BOUNDED_SECTION_PRE_LABEL(.pci_fixup_suspend_late, _pci_fixups_suspend_late, __start, __end) \
>         }                                                               \
>                                                                         \
> +       RO_GOT_X86                                                      \
>         FW_LOADER_BUILT_IN_DATA                                         \
>         TRACEDATA                                                       \
>                                                                         \
> --
> 2.31.1
>