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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230508114048.GB116442@k08j02272.eu95sqa>
Date:   Mon, 08 May 2023 19:40:48 +0800
From:   "Hou Wenlong" <houwenlong.hwl@...group.com>
To:     Ard Biesheuvel <ardb@...nel.org>
Cc:     <linux-kernel@...r.kernel.org>,
        "Thomas Garnier" <thgarnie@...omium.org>,
        "Lai Jiangshan" <jiangshan.ljs@...group.com>,
        "Kees Cook" <keescook@...omium.org>,
        "Thomas Gleixner" <tglx@...utronix.de>,
        "Ingo Molnar" <mingo@...hat.com>, "Borislav Petkov" <bp@...en8.de>,
        "Dave Hansen" <dave.hansen@...ux.intel.com>, <x86@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>, "Arnd Bergmann" <arnd@...db.de>,
        "Peter Zijlstra" <peterz@...radead.org>,
        "Josh Poimboeuf" <jpoimboe@...nel.org>,
        "Juergen Gross" <jgross@...e.com>,
        "Brian Gerst" <brgerst@...il.com>, <linux-arch@...r.kernel.org>
Subject: Re: [PATCH RFC 25/43] x86/mm: Make the x86 GOT read-only

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@...group.com> wrote:
> >
> > From: Thomas Garnier <thgarnie@...omium.org>
> >
> > From: Thomas Garnier <thgarnie@...omium.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@...omium.org>
> > Signed-off-by: Hou Wenlong <houwenlong.hwl@...group.com>
> > Cc: Lai Jiangshan <jiangshan.ljs@...group.com>
> > Cc: Kees Cook <keescook@...omium.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
> >