Re: [PATCH RFC 25/43] x86/mm: Make the x86 GOT read-only

From: "Hou Wenlong" <houwenlong.hwl@antgroup.com>
To: Ard Biesheuvel <ardb@kernel.org>
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>
Subject: Re: [PATCH RFC 25/43] x86/mm: Make the x86 GOT read-only
Date: Mon, 08 May 2023 19:40:48 +0800	[thread overview]
Message-ID: <20230508114048.GB116442@k08j02272.eu95sqa> (raw)
In-Reply-To: <CAMj1kXFFo6Y=p63N41DrN4wLzMNVdtD-hp6gBVQwNqrzt7oqwQ@mail.gmail.com>

On Sun, Apr 30, 2023 at 10:23:56PM +0800, Ard Biesheuvel wrote:
> 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.
>
Hi Ard,
I know that X86 specific stuff should be in generic code. I notice that
even relocation relaxation is enabled, the GOT would not be empty. I
want it to be included in the read-only data section (RODATA) between
the symbols __start_rodata and __end_rodata for safety. I noticed that
you placed it in the data section during your PIE linking.  Should it be
marked as read-only if it is not empty?

Thanks.
> >  /*
> >   * .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
> >