From: Ard Biesheuvel <[email protected]>
The .got.plt section contains the part of the GOT which is used by PLT
entries, and which gets updated lazily by the dynamic loader when
function calls are dispatched through those PLT entries.
On fully linked binaries such as the kernel proper or the decompressor,
this never happens, and so in practice, the .got.plt section consists
only of the first 3 magic entries that are meant to point at the _DYNAMIC
section and at the fixup routine in the loader. However, since we don't
use a dynamic loader, those entries are never populated or used.
This means that treating those entries like ordinary GOT entries, and
updating their values based on the actual placement of the executable in
memory is completely pointless, and we can just ignore the .got.plt
section entirely, provided that it has no additional entries beyond
the first 3 ones.
So add an assertion in the linker script to ensure that this assumption
holds, and move the contents out of the [_got, _egot) memory range that
is modified by the GOT fixup routines.
While at it, drop the KEEP(), since it has no effect on the contents
of output sections that are created by the linker itself.
Signed-off-by: Ard Biesheuvel <[email protected]>
Acked-by: Arvind Sankar <[email protected]>
Signed-off-by: Arvind Sankar <[email protected]>
From: Ard Biesheuvel <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
arch/x86/boot/compressed/vmlinux.lds.S | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/arch/x86/boot/compressed/vmlinux.lds.S b/arch/x86/boot/compressed/vmlinux.lds.S
index 8f1025d1f681..b17d218ccdf9 100644
--- a/arch/x86/boot/compressed/vmlinux.lds.S
+++ b/arch/x86/boot/compressed/vmlinux.lds.S
@@ -44,10 +44,13 @@ SECTIONS
}
.got : {
_got = .;
- KEEP(*(.got.plt))
KEEP(*(.got))
_egot = .;
}
+ .got.plt : {
+ *(.got.plt)
+ }
+
.data : {
_data = . ;
*(.data)
@@ -77,3 +80,9 @@ SECTIONS
DISCARDS
}
+
+#ifdef CONFIG_X86_64
+ASSERT(SIZEOF(.got.plt) == 0 || SIZEOF(.got.plt) == 0x18, "Unexpected GOT/PLT entries detected!")
+#else
+ASSERT(SIZEOF(.got.plt) == 0 || SIZEOF(.got.plt) == 0xc, "Unexpected GOT/PLT entries detected!")
+#endif
--
2.26.2
On Mon, 29 Jun 2020 at 17:50, Arvind Sankar <[email protected]> wrote:
>
> On Mon, Jun 29, 2020 at 08:48:05AM -0700, Kees Cook wrote:
> > On Mon, Jun 29, 2020 at 10:09:22AM -0400, Arvind Sankar wrote:
> >
> > This is also being done on arm64, and the section was specified slightly
> > differently (with INFO) which maybe should be done here too?
>
> I was actually just about to email you to ask what that INFO is for :)
> What does it do?
>
It makes the section non-allocatable, so it is dropped from the final image
> >
> > .got.plt (INFO) : { *(.got.plt) }
> > ASSERT(SIZEOF(.got.plt) == 0 || SIZEOF(.got.plt) == 0x18, ".got.plt not empty")
> >
> > Otherwise, yes, looks good.
> >
> > Reviewed-by: Kees Cook <[email protected]>
> >
> > > +
> > > .data : {
> > > _data = . ;
> > > *(.data)
> > > @@ -77,3 +80,9 @@ SECTIONS
> > >
> > > DISCARDS
> > > }
> > > +
> > > +#ifdef CONFIG_X86_64
> > > +ASSERT(SIZEOF(.got.plt) == 0 || SIZEOF(.got.plt) == 0x18, "Unexpected GOT/PLT entries detected!")
> > > +#else
> > > +ASSERT(SIZEOF(.got.plt) == 0 || SIZEOF(.got.plt) == 0xc, "Unexpected GOT/PLT entries detected!")
> > > +#endif
> > > --
> > > 2.26.2
> > >
> >
> > --
> > Kees Cook
On Mon, Jun 29, 2020 at 10:09:22AM -0400, Arvind Sankar wrote:
> From: Ard Biesheuvel <[email protected]>
>
> The .got.plt section contains the part of the GOT which is used by PLT
> entries, and which gets updated lazily by the dynamic loader when
> function calls are dispatched through those PLT entries.
>
> On fully linked binaries such as the kernel proper or the decompressor,
> this never happens, and so in practice, the .got.plt section consists
> only of the first 3 magic entries that are meant to point at the _DYNAMIC
> section and at the fixup routine in the loader. However, since we don't
> use a dynamic loader, those entries are never populated or used.
>
> This means that treating those entries like ordinary GOT entries, and
> updating their values based on the actual placement of the executable in
> memory is completely pointless, and we can just ignore the .got.plt
> section entirely, provided that it has no additional entries beyond
> the first 3 ones.
>
> So add an assertion in the linker script to ensure that this assumption
> holds, and move the contents out of the [_got, _egot) memory range that
> is modified by the GOT fixup routines.
>
> While at it, drop the KEEP(), since it has no effect on the contents
> of output sections that are created by the linker itself.
>
> Signed-off-by: Ard Biesheuvel <[email protected]>
> Acked-by: Arvind Sankar <[email protected]>
> Signed-off-by: Arvind Sankar <[email protected]>
> From: Ard Biesheuvel <[email protected]>
> Link: https://lore.kernel.org/r/[email protected]
> ---
> arch/x86/boot/compressed/vmlinux.lds.S | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/boot/compressed/vmlinux.lds.S b/arch/x86/boot/compressed/vmlinux.lds.S
> index 8f1025d1f681..b17d218ccdf9 100644
> --- a/arch/x86/boot/compressed/vmlinux.lds.S
> +++ b/arch/x86/boot/compressed/vmlinux.lds.S
> @@ -44,10 +44,13 @@ SECTIONS
> }
> .got : {
> _got = .;
> - KEEP(*(.got.plt))
> KEEP(*(.got))
> _egot = .;
> }
> + .got.plt : {
> + *(.got.plt)
> + }
This is also being done on arm64, and the section was specified slightly
differently (with INFO) which maybe should be done here too?
.got.plt (INFO) : { *(.got.plt) }
ASSERT(SIZEOF(.got.plt) == 0 || SIZEOF(.got.plt) == 0x18, ".got.plt not empty")
Otherwise, yes, looks good.
Reviewed-by: Kees Cook <[email protected]>
> +
> .data : {
> _data = . ;
> *(.data)
> @@ -77,3 +80,9 @@ SECTIONS
>
> DISCARDS
> }
> +
> +#ifdef CONFIG_X86_64
> +ASSERT(SIZEOF(.got.plt) == 0 || SIZEOF(.got.plt) == 0x18, "Unexpected GOT/PLT entries detected!")
> +#else
> +ASSERT(SIZEOF(.got.plt) == 0 || SIZEOF(.got.plt) == 0xc, "Unexpected GOT/PLT entries detected!")
> +#endif
> --
> 2.26.2
>
--
Kees Cook
On Mon, Jun 29, 2020 at 08:48:05AM -0700, Kees Cook wrote:
> On Mon, Jun 29, 2020 at 10:09:22AM -0400, Arvind Sankar wrote:
>
> This is also being done on arm64, and the section was specified slightly
> differently (with INFO) which maybe should be done here too?
I was actually just about to email you to ask what that INFO is for :)
What does it do?
>
> .got.plt (INFO) : { *(.got.plt) }
> ASSERT(SIZEOF(.got.plt) == 0 || SIZEOF(.got.plt) == 0x18, ".got.plt not empty")
>
> Otherwise, yes, looks good.
>
> Reviewed-by: Kees Cook <[email protected]>
>
> > +
> > .data : {
> > _data = . ;
> > *(.data)
> > @@ -77,3 +80,9 @@ SECTIONS
> >
> > DISCARDS
> > }
> > +
> > +#ifdef CONFIG_X86_64
> > +ASSERT(SIZEOF(.got.plt) == 0 || SIZEOF(.got.plt) == 0x18, "Unexpected GOT/PLT entries detected!")
> > +#else
> > +ASSERT(SIZEOF(.got.plt) == 0 || SIZEOF(.got.plt) == 0xc, "Unexpected GOT/PLT entries detected!")
> > +#endif
> > --
> > 2.26.2
> >
>
> --
> Kees Cook
On Mon, Jun 29, 2020 at 05:51:00PM +0200, Ard Biesheuvel wrote:
> On Mon, 29 Jun 2020 at 17:50, Arvind Sankar <[email protected]> wrote:
> >
> > On Mon, Jun 29, 2020 at 08:48:05AM -0700, Kees Cook wrote:
> > > On Mon, Jun 29, 2020 at 10:09:22AM -0400, Arvind Sankar wrote:
> > >
> > > This is also being done on arm64, and the section was specified slightly
> > > differently (with INFO) which maybe should be done here too?
> >
> > I was actually just about to email you to ask what that INFO is for :)
> > What does it do?
> >
>
> It makes the section non-allocatable, so it is dropped from the final image
i.e. takes no disk space, but the ASSERT can still be done.
--
Kees Cook