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
> >